Hi Jonathon, On Fri, 2015-05-01 at 16:18 -0500, Jonathon Jongsma wrote: > Interesting. This might actually be good enough to fix some of the > problems I was fighting with. But I've not totally convinced myself > yet > that there won't be some unexpected behavior caused by disabling > those > displays. I need to think on it a bit more. Some implementation > comments > below, though. > > On Thu, 2015-04-30 at 09:43 +0200, Pavel Grunt wrote: > > When running in fullscreen it is possible to end up in a situation > > where we have more displays enabled than monitors. This can happen > > if displays that were enabled in the previous connection to the > > guest > > doesn't match displays requested when entering the fullscreen mode. > > > > This commit solves the problem by disabling displays that should > > not > > enabled in the fullscreen mode. > > > > Resolves: rhbz#1212802 > > --- > > v1: https://www.redhat.com/archives/virt-tools-list/2015 > > -April/msg00184.html > > v2: - nth is not used to determine which display should be enabled, > > but the list of fullscreen displays is used > > - the extra display is disabled instead of being ignored > > v3: - added missing check for self->priv->did_auto_conf to fix > > hidden display > > when --reconnect > > --- > > src/virt-viewer-session-spice.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer > > -session-spice.c > > index b69faa6..161c3e0 100644 > > --- a/src/virt-viewer-session-spice.c > > +++ b/src/virt-viewer-session-spice.c > > @@ -58,6 +58,7 @@ struct _VirtViewerSessionSpicePrivate { > > gboolean has_sw_smartcard_reader; > > guint pass_try; > > gboolean did_auto_conf; > > + GList *fullscreen_displays; > > Why did you choose to cache a list of dispays here instead of using > e.g. > virt_viewer_app_get_initial_displays() (which is the function used to > populate the list in the first place? As you mentioned below we are not using ids from monitor-mapping. Maybe the approach I used in v1 is enough (storing the total number of fullscreen displays). > > > }; > > > > #define VIRT_VIEWER_SESSION_SPICE_GET_PRIVATE(o) > > (G_TYPE_INSTANCE_GET_PRIVATE((o), VIRT_VIEWER_TYPE_SESSION_SPICE, > > VirtViewerSessionSpicePrivate)) > > @@ -146,6 +147,7 @@ virt_viewer_session_spice_dispose(GObject *obj) > > spice->priv->audio = NULL; > > > > g_clear_object(&spice->priv->main_window); > > + g_list_free(spice->priv->fullscreen_displays); > > > > G_OBJECT_CLASS(virt_viewer_session_spice_parent_class) > > ->dispose(obj); > > } > > @@ -693,6 +695,15 @@ destroy_display(gpointer data) > > g_object_unref(display); > > } > > > > +static gboolean > > +display_is_in_fullscreen_mode(VirtViewerSessionSpice *self, > > + VirtViewerDisplay *display) > > +{ > > + gconstpointer nth = > > GINT_TO_POINTER(virt_viewer_display_get_nth(display)); > > + > > + return g_list_index(self->priv->fullscreen_displays, nth) != > > -1; > > +} > > + > > static void > > virt_viewer_session_spice_display_monitors(SpiceChannel *channel, > > GParamSpec *pspec > > G_GNUC_UNUSED, > > @@ -702,6 +713,8 @@ > > virt_viewer_session_spice_display_monitors(SpiceChannel *channel, > > GPtrArray *displays = NULL; > > GtkWidget *display; > > guint i, monitors_max; > > + gboolean fullscreen_mode = > > + > > virt_viewer_app_get_fullscreen(virt_viewer_session_get_app(VIRT_VI > > EWER_SESSION(self))); > > > > g_object_get(channel, > > "monitors", &monitors, > > @@ -739,6 +752,17 @@ > > virt_viewer_session_spice_display_monitors(SpiceChannel *channel, > > if (monitor->width == 0 || monitor->height == 0) > > continue; > > > > + if (fullscreen_mode && > > + self->priv->did_auto_conf > > &&m > > + !display_is_in_fullscreen_mode(self, > > VIRT_VIEWER_DISPLAY(display))) { > > + g_debug("display %d should not be enabled, disabling", > > + > > virt_viewer_display_get_nth(VIRT_VIEWER_DISPLAY(display))); > > + > > spice_main_set_display_enabled(virt_viewer_session_spice_get_main_ > > channel(self), > > + > > virt_viewer_display_get_nth(VIRT_VIEWER_DISPLAY(display)), > > + FALSE); > > + continue; > > + } > > + > > > > virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), > > TRUE); > > > > virt_viewer_display_spice_set_desktop(VIRT_VIEWER_DISPLAY(display) > > , > > monitor->x, monitor > > ->y, > > @@ -879,6 +903,8 @@ > > virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice > > *self) > > > > for (i = 0; i < ndisplays; i++) { > > GdkRectangle *rect = &displays[i]; > > + self->priv->fullscreen_displays = g_list_append(self > > ->priv->fullscreen_displays, > > + > > GINT_TO_POINTER(i)); > > the 'i' variable here is not the id of the display, so I don't think > that this will work generically. Up above in > display_is_in_fullscreen_mode() you compare this value to the 'nth' > property of the display, which I don't believe is valid. > > > > > spice_main_set_display(cmain, i, rect->x, rect->y, rect > > ->width, rect->height); > > spice_main_set_display_enabled(cmain, i, TRUE); > > But now I notice that we do in fact use 'i' as the id of the > display... > This seems like a bug. But I guess that probably explains both of my > comments above. > As you said we are requesting to enable the display #i. I tried to change it to use the id according to the user configuration (monitor -mapping), but it caused another problem - an extra display #0 when rebooting. If using 'i' as id is a bug I think it is better to make the monitor mapping more strict, ie mapping like "monitor-mapping=4:2;2:3" should not be valid. Thanks, Pavel _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
