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? > > Pavel 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. Jonathon _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
