[virt-tools-list] [PATCH virt-viewer 3/3] Add new functions to enable/disable a display
jjongsma at redhat.com
Thu Sep 10 21:25:58 UTC 2015
OK, I've finally pushed this series and the spice-gtk patches it depends
on. You may need to update your spice-gtk and re-configure the next time
On Tue, 2015-08-18 at 15:57 +0200, Pavel Grunt wrote:
> 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.
More information about the virt-tools-list