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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
