[virt-tools-list] [virt-viewer][PATCH 1/4 v3] virt-viewer: Moved preferences from app to window
jjongsma at redhat.com
Wed May 20 15:28:33 UTC 2015
On Wed, 2015-05-20 at 08:41 +0200, Pavel Grunt wrote:
> 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?
Hmm, indeed it would be weird to change a persistent setting when the
user started the application with the --reconnect switch. But I think
it's also weird to have a non-persistent setting in the preferences
dialog. I personally still think that this option probably doesn't need
to be exposed in the UI.
More information about the virt-tools-list