[virt-tools-list] [PATCH virt-viewer 3/3] Add new functions to enable/disable a display

[Pavel Grunt]

On Mon, 2015-08-10 at 16:25 -0500, Jonathon Jongsma wrote:
> On Tue, 2015-08-04 at 10:25 +0200, Pavel Grunt wrote:
> > Hi Jonathon,
> > 
> > On Mon, 2015-08-03 at 15:59 -0500, Jonathon Jongsma wrote:
> > > Previously, there was a single function for controlling the enabled
> > > state of a display: virt_viewer_display_set_enabled(). Unfortunately,
> > > this function is used for two slightly different things:
> > > 
> > >  A. It informs the local display widget that the display has become
> > >     disabled or enabled on the server. In other words, it tries to
> > >     synchronize the 'enabled' state of the local widget with the actual
> > >     state of the remote display.
> > > 
> > > OR
> > > 
> > >  B. It tries to actively enable a currently-disabled display (or vice
> > >     versa) due to some action by the user in the client application.
> > >     This causes the client to send a new configuration down to the
> > >     server. In other words, it tries to change the state of the remote
> > >     display.
> > > 
> > > There is some conflict between these two scenarios. If the change is due
> > > to a notification from the server, there is no need to send a new
> > > configuration back down to the server, so this results in unnecessary
> > > monitor configuration messages and can in fact cause issues that are a
> > > little bit hard to track down. Because of this, I decided that it was
> > > really necessary to have two separate functions for these two different
> > > scenarios. so the existing _set_enabled() function will be used for
> > > scenario A mentioned above. I added two new
> > > functiions (_enable() and _disable()) that are used to send new
> > typo   ^
> > > configurations down to the server.
> > > ---
> > >  configure.ac                    |  2 +-
> > >  src/virt-viewer-display-spice.c | 29 +++++++++++++++++++++++++----
> > >  src/virt-viewer-display.c       | 31 +++++++++++++++++++++++++++++++
> > >  src/virt-viewer-display.h       |  4 ++++
> > >  src/virt-viewer-window.c        |  6 +++---
> > >  5 files changed, 64 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index d37863b..a7b7140 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -19,7 +19,7 @@ GTK2_REQUIRED="2.18.0"
> > >  GTK3_REQUIRED="3.0"
> > >  GTK_VNC1_REQUIRED="0.3.8"
> > >  GTK_VNC2_REQUIRED="0.4.0"
> > > -SPICE_GTK_REQUIRED="0.28"
> > > +SPICE_GTK_REQUIRED="0.29.17"
> > It should be "0.30"
> 
> In theory, perhaps. However, that makes it quite difficult to build the
> git version (since there is no version of spice-gtk yet that has a
> version of 0.30). So I simply found the version of spice-gtk that
> included my new spice-gtk API (running
> 'build-aux/git-version-gen .tarball-version' in the spice-gtk
> repository) and used that. Of course, the spice-gtk repository has since
> changed and that new API has not yet been committed upstream, so this
> version number will need to be updated.
> 
> 
> > So this commit depends on your spice-gtk series ?
> > http://lists.freedesktop.org/archives/spice-devel/2015-July/020873.html
> > or just on the patch 2/4 of that series
> > http://lists.freedesktop.org/archives/spice-devel/2015-July/020875.html
> 
> Yes, it just depends on the patch that adds that new API.
> 
> > 
> > imho it would be better to split this patch otherwise the change
> > from spice_main_set_display_enabled() to spice_main_update_display_enabled()
> > will be "hidden".
> 
> I could do that, but I don't think that it splits up very nicely. The
> first commit would be adding two new api functions (_enable() and
> _disable()) that do exactly the same thing as _set_enabled(): they would
> both call the old API which automatically sends new monitors-config
> messages to the server. Then the second commit would change to calling
> the new API so that we can avoid sending the monitors-config messages.
> But avoiding the messages is the entire point of adding the new API. So
> to me it makes more sense as a single commit. 
> 
I thought that the first commit would just replace
'spice_main_set_display_enabled' by 'spice_main_update_display_enabled' and bump
the spice-gtk version. The second commit would add the new functions to
enable/disable a display. But it is just my opinion and your explanation makes
sense, so

ACK the series.

Pavel