On Mon, Feb 15, 2016 at 2:57 PM, Eduardo Lima (Etrunko)
<etrunko@xxxxxxxxxx> 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@xxxxxxxxxx
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
