[virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's

[Eduardo Lima (Etrunko)]

On 02/15/2016 11:47 AM, Fabiano Fidêncio wrote:
[snip]

>>  
>>   cleanup:
>> -    g_free(uri);
>> -    if (viewer)
>> -        g_object_unref(viewer);
>> -    g_strfreev(args);
>> -    g_clear_error(&error);
>> -
>> +    g_object_unref(viewer);
> 
> g_object_unref() shouldn't be called with a NULL argument. So, you'll
> need to re-add the check for the viewer before unref'ing the object.
> 

Fixed.

>> +        }
>> +
>> +        g_clear_error(&error);
>> +        g_application_quit(app);
> 
> 
> And then return here ... ?
> 

I don't think it is necessary, g_application_quit() will end the
execution of the program.

>> -    g_free(uri);
>> -    g_strfreev(args);
>> -    g_free(help_msg);
>> -    g_clear_error(&error);
>> -
>> +    g_object_unref(viewer);
> 
> As for remote-viewer, don't remove the check for the viewer before
> calling g_object_unref().
> 

Also fixed.

>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>> index 3a958f0..14549fd 100644
>> --- a/src/virt-viewer-window.c
>> +++ b/src/virt-viewer-window.c
>> @@ -346,8 +346,7 @@ virt_viewer_window_init (VirtViewerWindow *self)
>>      gtk_window_set_has_resize_grip(GTK_WINDOW(priv->window), FALSE);
>>      priv->accel_enabled = TRUE;
>>  
>> -    accels = gtk_accel_groups_from_object(G_OBJECT(priv->window));
>> -    for ( ; accels ; accels = accels->next) {
>> +    for (accels = gtk_accel_groups_from_object(G_OBJECT(priv-
>>> window)); accels; accels = accels->next) {
> 
> This change is not related ....
> 

Yes, I can merge it with previous cleanup patch.

> 
> 
> Didn't have any problem running on Windows and on Linux.
> 
> The code is in a way better shape than the previous versions. Just a
> small set of minor comments from me. Also, please, this whole series is
> breaking "make syntax-check" as pointed out in the #spice channel.
> +1 for having the code in with the fixes suggested.
> 
> Please, I also would like to have at least one more review from someone
> used with the client side (Daniel? Jonathon? Pavel?) before you can
> consider it as an ACK!

Sure, will post a new version with fixes to comments. Thanks for the review.

-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com