Hi Jonathon,
It seems I got a bit lost with the all comments on the previous series
and some of them became were not addressed. Once again, thanks for the
review. My comments follow inline. I am working on the new version of
this series.
On 12/14/2015 08:46 PM, Jonathon Jongsma wrote:
>> @@ -238,11 +275,11 @@ remote_viewer_init(RemoteViewer *self)
>> }
>>
>> RemoteViewer *
>> -remote_viewer_new(const gchar *uri)
>> +remote_viewer_new(void)
>> {
>> return g_object_new(REMOTE_VIEWER_TYPE,
>> - "guri", uri,
>> - "open-recent-dialog", uri == NULL,
>> + "application-id", "org.fedorahosted.remote-viewer",
>
> Still using org.fedorahosted. Does anybody else have an opinion on this? I
>
I think we could be using "virt-manager" instead of fedorahosted. But it
is simple enough to change it.
>> @@ -634,11 +651,12 @@ remote_viewer_activate(VirtViewerApp *app, GError
>> **error)
>> }
>>
>> static void
>> -remote_viewer_window_added(VirtViewerApp *app,
>> - VirtViewerWindow *win)
>> +remote_viewer_window_added(GtkApplication *app,
>> + G_GNUC_UNUSED GtkWindow *win)
>> {
>> - spice_menu_update(REMOTE_VIEWER(app), win);
>> - spice_foreign_menu_update(REMOTE_VIEWER(app), win);
>> + VirtViewerWindow *window =
>> virt_viewer_app_get_main_window(VIRT_VIEWER_APP(app));
>> + spice_menu_update(REMOTE_VIEWER(app), window);
>> + spice_foreign_menu_update(REMOTE_VIEWER(app), window);
>
>
> This seems wrong to me. When we add a new window to the appliation, I think we
> should be updating the menu, etc for the new window, rather than the main
> window.
Yes, you are right, this function is updating the menu on the main
window only. I am thinking about adding the VirtViewerWindow pointer as
a data to the GtkWindow so we can access it here and keep the
menu_update functions unchanged.
> @@ -1185,6 +1203,76 @@ cleanup:
>> return ret;
>> }
>>
>> +static gint
>> +remote_viewer_handle_local_options(GApplication *gapp, GVariantDict *options)
>> +{
>> + VirtViewerApp *app = VIRT_VIEWER_APP(gapp);
>> + gint ret = -1;
>> + gchar *title = NULL;
>> + gchar **args = NULL;
>> + gboolean controller = FALSE;
>> +
>> + if (g_variant_dict_contains(options, OPT_VERSION)) {
>> + g_print(_("%s version %s"), g_get_prgname(), VERSION BUILDID);
>> +#ifdef REMOTE_VIEWER_OS_ID
>> + g_print(" (OS ID: %s)", REMOTE_VIEWER_OS_ID);
>> +#endif
>> + g_print("\n");
>> + return 0;
>> + }
>> +
>> + if (g_variant_dict_lookup(options, G_OPTION_REMAINING, "^as", &args)) {
>> + if (args && (g_strv_length(args) != 1)) {
>> + g_printerr(_("Error: can't handle multiple URIs\n"));
>> + ret = 1;
>> + goto end;
>> + }
>> +
>> + g_object_set(app, "guri", args[0], NULL);
>> + }
>> +
>> + g_variant_dict_lookup(options, OPT_TITLE, "s", &title);
>
> This didn't occur to me during the last review, but: why did you decide to pass
> NULL for arg_data in the GOptionEntry array instead of continuing to set
> arg_data to a pointer to e.g. a string variable? Setting arg_data to NULL there
> means that the argument value will be stuffed into a GVariantDict which you can
> inspect in this vfunc. But that just seems like more work to me. If you had left
> it the old way, you could just use the (already-populated) variable here instead
> of needing to extract the value from the variant dict here. It also means that
> you wouldn't need to create string variables for each of the option names
> (OPT_TITLE, etc) since you wouldn't need to query the variant dict by name here.
>
Yes, this was intended. I tried to make use of the new functionalities
provided by glib as much as possible. Now, knowing that copying that
huge amount of compatibility code from glib is not desired, I will get
back to the original approach and keep the variables for handling the
options.
>> +
>> +#ifdef HAVE_SPICE_GTK
>> + if (g_variant_dict_lookup(options, OPT_CONTROLLER, "b", &controller)) {
>> + if (args) {
>> + g_printerr(_("Error: extra arguments given while using Spice
>> controller\n\n"));
>> + ret = 1;
>> + goto end;
>> + } else {
>> + RemoteViewer *self = REMOTE_VIEWER(app);
>> + SpiceCtrlController *ctrl = spice_ctrl_controller_new();
>> + SpiceCtrlForeignMenu *menu = spice_ctrl_foreign_menu_new();
>> +
>> + g_object_set(self, "guest-name", "defined by Spice controller",
>> + "controller", ctrl,
>> + "foreign-menu", menu,
>> + NULL);
>> +
>> + g_signal_connect(menu, "notify::title",
>> + G_CALLBACK(foreign_menu_title_changed),
>> + self);
>> +
>> + g_object_unref(ctrl);
>> + g_object_unref(menu);
>> + }
>> + }
>> +#endif
>> +
>> + if (title && !controller)
>> + g_object_set(app, "title", title, NULL);
>> +
>> + ret = G_APPLICATION_CLASS(remote_viewer_parent_class)
>> ->handle_local_options(gapp, options);
>> +
>> +end:
>> + if (ret == 1)
>
> I know you only use -1, 0, and 1 above, but technically any positive number is
> an error condition, so I think this test should be >0 instead of ==1
>
Fixed.
>> @@ -298,7 +298,7 @@ virt_viewer_app_quit(VirtViewerApp *self)
>> }
>> }
>>
>> - gtk_main_quit();
>> + g_application_quit(G_APPLICATION(self));
>> }
>>
>> static gint
>> @@ -926,12 +926,11 @@ virt_viewer_app_window_new(VirtViewerApp *self, gint
>> nth)
>> virt_viewer_session_get_has_usbredir(self->priv
>> ->session));
>> }
>>
>> - g_signal_emit(self, signals[SIGNAL_WINDOW_ADDED], 0, window);
>> -
>> if (self->priv->fullscreen)
>> app_window_try_fullscreen(self, window, nth);
>>
>> w = virt_viewer_window_get_window(window);
>> + gtk_application_add_window(GTK_APPLICATION(self), w);
>
> Copying comments from my first review:
>
> This is a slight change in behavior. It will now emit the window-added signal
> after the call to app_window_try_fullscreen() rather than before. Not sure
> whether this is important, just noting it. Perhaps move this where the original
> signal was emitted?
>
Fixed too.
>
>> g_signal_connect(w, "hide", G_CALLBACK(viewer_window_visible_cb), self);
>> g_signal_connect(w, "show", G_CALLBACK(viewer_window_visible_cb), self);
>> g_signal_connect(w, "focus-in-event",
>> G_CALLBACK(viewer_window_focus_in_cb), self);
>> @@ -1046,8 +1045,6 @@ static void
>> virt_viewer_app_remove_nth_window(VirtViewerApp *self,
>> g_debug("Remove window %d %p", nth, win);
>> self->priv->windows = g_list_remove(self->priv->windows, win);
>>
>> - g_signal_emit(self, signals[SIGNAL_WINDOW_REMOVED], 0, win);
>> -
>> g_object_unref(win);
>> }
>>
>> @@ -1401,7 +1398,7 @@ virt_viewer_app_default_deactivated(VirtViewerApp *self,
>> gboolean connect_error)
>> }
>>
>> if (self->priv->quit_on_disconnect)
>> - gtk_main_quit();
>> + g_application_quit(G_APPLICATION(self));
>> }
>>
>> static void
>> @@ -1479,7 +1476,7 @@ virt_viewer_app_disconnected(VirtViewerSession *session
>> G_GNUC_UNUSED, const gch
>> virt_viewer_app_hide_all_windows(self);
>>
>> if (priv->quitting)
>> - gtk_main_quit();
>> + g_application_quit(G_APPLICATION(self));
>
>
>
> Copying comments from my first review:
>
> I'd need to test to make sure, but I think it's possible that the call to
> virt_viewer_app_hide_all_windows() a few lines above might result in the
> application quitting because the application no longer has any associated
> windows. In other words, even if priv->quitting is false, the application might
> quit here. I could be wrong though. Have you tested this path?
>
> (Just want to make sure this has been tested)
Also fixed.
--
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
