If we agree on this,On Tue, 2015-05-19 at 11:40 -0500, Jonathon Jongsma wrote:
> General comment on the approach: it seems to me that it would be
> cleaner
> to implement this using virtual functions. For example, in patch #3,
> you
> add a "can-reconnect" property to VirtViewerApp. This means that it's
> technically possible to construct a RemoteViewer object with
> can-reconnect=TRUE, even though RemoteViewer cannot reconnect.
>
> There are two basic ways to achieve this with virtual functions:
>
> 1) make virt_viewer_app_get_preferences() a virtual function. Each
> class
> (RemoteViewer and VirtViewer) would implement this virtual function
> and
> return the preferences that it supports. There may be a fair amount
> of
> code duplication in this approach, but it might be possible to
> overcome
> that.
>
> 2) make virt_viewer_app_can_reconnect() a virtual function.
> RemoteViewer
> would implement this function by returning FALSE, and VirtViewer
> would
> return TRUE (look at virt_viewer_session_can_share_folder() for
> inspiration)
>
> In addition, "preferences" to me indicates a persistent setting, but
> it
> doesn't appear that these settings are stored anywhere. Should we
> hook
> them up to the existing config infrastructure in VirtViewerApp so
> they
> end up stored in e.g. ~/.local/virt-viewer/settings? Furthermore,
> should
> we add the existing "ask-quit" setting to this preferences dialog?
> Opinions?
>
I think it would be beneficial for the user to have "ask-quit" in the
"preferences" - changing the default configuration by editing a file
is not user-friendly.
About making "reconnect" the persistent setting - I am ok with it, it
makes sense to let the user define some default behavior for it. But I
think that with this change the synchronization between the
commandline option and the gui should be removed (to avoid making the
permanent setting just because it was once used with '--reconnect').
Maybe we should add the '--no-reconnect' commandline option to
override the default setting?
Pavel
> Jonathon
>
>
> On Wed, 2015-05-13 at 16:51 +0200, Lukas Venhoda wrote:
> > Moved preferences from virt-viewer-app to virt-viewer-window.
> > Makes more sense to have all of the GUI in virt-viewer-window.
> > This also makes it possible to implement virt-viewer/remote-viewer
> > specific settings in preferences.
> > ---
> > New commit in version 3
> > Moving preferences into window is better then ifdefs all over
> > virt-viewer-app
> > ---
> > src/virt-viewer-app.c | 89 -----------------------------------
> > -----------
> > src/virt-viewer-app.h | 1 -
> > src/virt-viewer-window.c | 92
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 91 insertions(+), 91 deletions(-)
> >
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index b22a876..2570b8c 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -112,8 +112,6 @@ struct _VirtViewerAppPrivate {
> > GHashTable *displays;
> > GHashTable *initial_display_map;
> > gchar *clipboard;
> > - GtkWidget *preferences;
> > - GtkFileChooser *preferences_shared_folder;
> > gboolean direct;
> > gboolean verbose;
> > gboolean enable_accel;
> > @@ -1668,10 +1666,6 @@ virt_viewer_app_dispose (GObject *object)
> > VirtViewerApp *self = VIRT_VIEWER_APP(object);
> > VirtViewerAppPrivate *priv = self->priv;
> >
> > - if (priv->preferences)
> > - gtk_widget_destroy(priv->preferences);
> > - priv->preferences = NULL;
> > -
> > if (priv->windows) {
> > GList *tmp = priv->windows;
> > /* null-ify before unrefing, because we need
> > @@ -2442,89 +2436,6 @@ virt_viewer_app_get_windows(VirtViewerApp
> > *self)
> > return self->priv->windows;
> > }
> >
> > -static void
> > -share_folder_changed(VirtViewerApp *self)
> > -{
> > - gchar *folder;
> > -
> > - folder = gtk_file_chooser_get_filename(self->priv
> > ->preferences_shared_folder);
> > -
> > - g_object_set(virt_viewer_app_get_session(self),
> > - "shared-folder", folder, NULL);
> > -
> > - g_free(folder);
> > -}
> > -
> > -static GtkWidget *
> > -virt_viewer_app_get_preferences(VirtViewerApp *self)
> > -{
> > - VirtViewerSession *session =
> > virt_viewer_app_get_session(self);
> > - GtkBuilder *builder = virt_viewer_util_load_ui("virt-viewer
> > -preferences.xml");
> > - gboolean can_share_folder =
> > virt_viewer_session_can_share_folder(session);
> > - GtkWidget *preferences = self->priv->preferences;
> > - gchar *path;
> > -
> > - if (preferences)
> > - goto end;
> > -
> > - gtk_builder_connect_signals(builder, self);
> > -
> > - preferences = GTK_WIDGET(gtk_builder_get_object(builder,
> > "preferences"));
> > - self->priv->preferences = preferences;
> > -
> > - g_object_set (gtk_builder_get_object(builder,
> > "cbsharefolder"),
> > - "sensitive", can_share_folder, NULL);
> > - g_object_set (gtk_builder_get_object(builder,
> > "cbsharefolderro"),
> > - "sensitive", can_share_folder, NULL);
> > - g_object_set (gtk_builder_get_object(builder,
> > "fcsharefolder"),
> > - "sensitive", can_share_folder, NULL);
> > -
> > - if (!can_share_folder)
> > - goto end;
> > -
> > - g_object_bind_property(virt_viewer_app_get_session(self),
> > - "share-folder",
> > - gtk_builder_get_object(builder,
> > "cbsharefolder"),
> > - "active",
> > -
> > G_BINDING_BIDIRECTIONAL|G_BINDING_SYNC_CREATE);
> > -
> > - g_object_bind_property(virt_viewer_app_get_session(self),
> > - "share-folder-ro",
> > - gtk_builder_get_object(builder,
> > "cbsharefolderro"),
> > - "active",
> > -
> > G_BINDING_BIDIRECTIONAL|G_BINDING_SYNC_CREATE);
> > -
> > - self->priv->preferences_shared_folder =
> > - GTK_FILE_CHOOSER(gtk_builder_get_object(builder,
> > "fcsharefolder"));
> > -
> > - g_object_get(virt_viewer_app_get_session(self),
> > - "shared-folder", &path, NULL);
> > -
> > - gtk_file_chooser_set_filename(self->priv
> > ->preferences_shared_folder, path);
> > - g_free(path);
> > -
> > - virt_viewer_signal_connect_object(self->priv
> > ->preferences_shared_folder,
> > - "file-set",
> > -
> > G_CALLBACK(share_folder_changed), self,
> > - G_CONNECT_SWAPPED);
> > -
> > -end:
> > - g_object_unref(builder);
> > -
> > - return preferences;
> > -}
> > -
> > -void
> > -virt_viewer_app_show_preferences(VirtViewerApp *self, GtkWidget
> > *parent)
> > -{
> > - GtkWidget *preferences =
> > virt_viewer_app_get_preferences(self);
> > -
> > - gtk_window_set_transient_for(GTK_WINDOW(preferences),
> > - GTK_WINDOW(parent));
> > -
> > - gtk_window_present(GTK_WINDOW(preferences));
> > -}
> > -
> > static gboolean
> > option_kiosk_quit(G_GNUC_UNUSED const gchar *option_name,
> > const gchar *value,
> > diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h
> > index f53fa73..ab1d7a9 100644
> > --- a/src/virt-viewer-app.h
> > +++ b/src/virt-viewer-app.h
> > @@ -100,7 +100,6 @@ void
> > virt_viewer_app_clear_hotkeys(VirtViewerApp *app);
> > GList* virt_viewer_app_get_initial_displays(VirtViewerApp* self);
> > gint
> > virt_viewer_app_get_initial_monitor_for_display(VirtViewerApp*
> > self, gint display);
> > void virt_viewer_app_set_enable_accel(VirtViewerApp *app,
> > gboolean enable);
> > -void virt_viewer_app_show_preferences(VirtViewerApp *app,
> > GtkWidget *parent);
> > void virt_viewer_app_set_menus_sensitive(VirtViewerApp *self,
> > gboolean sensitive);
> >
> > G_END_DECLS
> > diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> > index d67fbc1..f66bd50 100644
> > --- a/src/virt-viewer-window.c
> > +++ b/src/virt-viewer-window.c
> > @@ -71,6 +71,7 @@ static void
> > virt_viewer_window_toolbar_setup(VirtViewerWindow *self);
> > static GtkMenu*
> > virt_viewer_window_get_keycombo_menu(VirtViewerWindow *self);
> > static void
> > virt_viewer_window_get_minimal_dimensions(VirtViewerWindow *self,
> > guint *width, guint *height);
> > static gint
> > virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self);
> > +static void virt_viewer_window_show_preferences(VirtViewerWindow
> > *self, GtkWidget *parent);
> >
> > G_DEFINE_TYPE (VirtViewerWindow, virt_viewer_window,
> > G_TYPE_OBJECT)
> >
> > @@ -97,6 +98,8 @@ struct _VirtViewerWindowPrivate {
> > GtkAccelGroup *accel_group;
> > VirtViewerNotebook *notebook;
> > VirtViewerDisplay *display;
> > + GtkWidget *preferences;
> > + GtkFileChooser *preferences_shared_folder;
> >
> > gboolean accel_enabled;
> > GValue accel_setting;
> > @@ -171,6 +174,10 @@ virt_viewer_window_dispose (GObject *object)
> > VirtViewerWindowPrivate *priv = VIRT_VIEWER_WINDOW(object)
> > ->priv;
> > GSList *it;
> >
> > + if (priv->preferences)
> > + gtk_widget_destroy(priv->preferences);
> > + priv->preferences = NULL;
> > +
> > if (priv->display) {
> > g_object_unref(priv->display);
> > priv->display = NULL;
> > @@ -1030,7 +1037,7 @@ G_MODULE_EXPORT void
> > virt_viewer_window_menu_preferences_cb(GtkWidget *menu
> > G_GNUC_UNUSED,
> > VirtViewerWindow *self)
> > {
> > - virt_viewer_app_show_preferences(self->priv->app,
> > GTK_WIDGET(self->priv->window));
> > + virt_viewer_window_show_preferences(self, GTK_WIDGET(self
> > ->priv->window));
> > }
> >
> > G_MODULE_EXPORT void
> > @@ -1582,6 +1589,89 @@
> > virt_viewer_window_get_minimal_zoom_level(VirtViewerWindow *self)
> > return CLAMP(zoom * ZOOM_STEP, MIN_ZOOM_LEVEL,
> > NORMAL_ZOOM_LEVEL);
> > }
> >
> > +static void
> > +share_folder_changed(VirtViewerWindow *self)
> > +{
> > + gchar *folder;
> > +
> > + folder = gtk_file_chooser_get_filename(self->priv
> > ->preferences_shared_folder);
> > +
> > + g_object_set(virt_viewer_app_get_session(self->priv->app),
> > + "shared-folder", folder, NULL);
> > +
> > + g_free(folder);
> > +}
> > +
> > +static GtkWidget *
> > +virt_viewer_window_get_preferences(VirtViewerWindow *self)
> > +{
> > + VirtViewerSession *session = virt_viewer_app_get_session(self
> > ->priv->app);
> > + GtkBuilder *builder = virt_viewer_util_load_ui("virt-viewer
> > -preferences.xml");
> > + gboolean can_share_folder =
> > virt_viewer_session_can_share_folder(session);
> > + GtkWidget *preferences = self->priv->preferences;
> > + gchar *path;
> > +
> > + if (preferences)
> > + goto end;
> > +
> > + gtk_builder_connect_signals(builder, self);
> > +
> > + preferences = GTK_WIDGET(gtk_builder_get_object(builder,
> > "preferences"));
> > + self->priv->preferences = preferences;
> > +
> > + g_object_set (gtk_builder_get_object(builder,
> > "cbsharefolder"),
> > + "sensitive", can_share_folder, NULL);
> > + g_object_set (gtk_builder_get_object(builder,
> > "cbsharefolderro"),
> > + "sensitive", can_share_folder, NULL);
> > + g_object_set (gtk_builder_get_object(builder,
> > "fcsharefolder"),
> > + "sensitive", can_share_folder, NULL);
> > +
> > + if (!can_share_folder)
> > + goto end;
> > +
> > + g_object_bind_property(virt_viewer_app_get_session(self->priv
> > ->app),
> > + "share-folder",
> > + gtk_builder_get_object(builder,
> > "cbsharefolder"),
> > + "active",
> > +
> > G_BINDING_BIDIRECTIONAL|G_BINDING_SYNC_CREATE);
> > +
> > + g_object_bind_property(virt_viewer_app_get_session(self->priv
> > ->app),
> > + "share-folder-ro",
> > + gtk_builder_get_object(builder,
> > "cbsharefolderro"),
> > + "active",
> > +
> > G_BINDING_BIDIRECTIONAL|G_BINDING_SYNC_CREATE);
> > +
> > + self->priv->preferences_shared_folder =
> > + GTK_FILE_CHOOSER(gtk_builder_get_object(builder,
> > "fcsharefolder"));
> > +
> > + g_object_get(virt_viewer_app_get_session(self->priv->app),
> > + "shared-folder", &path, NULL);
> > +
> > + gtk_file_chooser_set_filename(self->priv
> > ->preferences_shared_folder, path);
> > + g_free(path);
> > +
> > + virt_viewer_signal_connect_object(self->priv
> > ->preferences_shared_folder,
> > + "file-set",
> > +
> > G_CALLBACK(share_folder_changed), self,
> > + G_CONNECT_SWAPPED);
> > +
> > +end:
> > + g_object_unref(builder);
> > +
> > + return preferences;
> > +}
> > +
> > +void
> > +virt_viewer_window_show_preferences(VirtViewerWindow *self,
> > GtkWidget *parent)
> > +{
> > + GtkWidget *preferences =
> > virt_viewer_window_get_preferences(self);
> > +
> > + gtk_window_set_transient_for(GTK_WINDOW(preferences),
> > + GTK_WINDOW(parent));
> > +
> > + gtk_window_present(GTK_WINDOW(preferences));
> > +}
> > +
> > /*
> > * Local variables:
> > * c-indent-level: 4
> > --
> > 2.4.0
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
