On Wed, May 20, 2015 at 5:28 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote: > 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. > Hmm. I agree that persistent options (the ones present in the settings file) should be exposed in the UI. What I do not agree is that these options are the only ones to be exposed. OTOH, I do not have a good idea about where/how to expose them to the users in a good way. An input from a UX designer would be more than appreciated about all those things. > Jonathon > > _______________________________________________ > virt-tools-list mailing list > virt-tools-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/virt-tools-list Best Regards, -- Fabiano Fidêncio _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
