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

[Fabiano Fidêncio]

On Mon, Feb 15, 2016 at 2:57 PM, Eduardo Lima (Etrunko)
<etrunko at redhat.com> wrote:
> 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.

I'd rather live it as it is.

>
>>
>>
>> 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.

Sure, please, rebase everything on git master and re-send and I'll
take a second look (and someone else also will take a look) :-)

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