[virt-tools-list] [PATCH virt-viewer v3] session-spice: Disable extra displays in fullscreen mode
Pavel Grunt
pgrunt at redhat.com
Mon May 4 06:55:05 UTC 2015
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
More information about the virt-tools-list
mailing list