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

[Jonathon Jongsma]

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. 

Jonathon