[virt-tools-list] [PATCH virt-viewer 3/3] Add new functions to enable/disable a display
jjongsma at redhat.com
Mon Aug 10 21:25:05 UTC 2015
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 ?
> or just on the patch 2/4 of that series
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.
More information about the virt-tools-list