[virt-tools-list] [PATCH virt-viewer v3] session-spice: Disable extra displays in fullscreen mode

[Jonathon Jongsma]

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?

>  };
>  
>  #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_VIEWER_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.

Jonathon