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

[Jonathon Jongsma]

NACK. I'm quite sure that this is essentially the same problem I've been
trying to solve in https://bugzilla.redhat.com/show_bug.cgi?id=1200750.
Comments below.


On Fri, 2015-04-24 at 17:12 +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 we request when entering the fullscreen mode.
> In other words when running in the fullscreen mode we request enabling
> displays #1, #2 if we have two monitors. However at the moment the
> first "notify::monitors" is recieved, the displays that were used in
> the previous session (e.g. #1, #3) are enabled. As we leave fullscreen
> the display #3 shows up.
> 
> This commit solves the problem by marking requested displays as
> fullscreen and only these can be enabled in the fullscreen mode.
> 
> Resolves: rhbz#1212802
> ---
> I'm not sure wheter this can have impact on rhbz#1200750
> Thanks in advance for any comments.

It might be interesting to test this bug with my (rejected) patches for
rhbz#1200750 applied to see if it fixes this issue. If it does, then I
think that it's safe to assume that the root cause is the same, even
though we don't have an acceptable fix for that bug yet.

> ---
>  src/virt-viewer-session-spice.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c
> index b69faa6..326dfd9 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;
> +    guint fullscreen_displays;
>  };
>  
>  #define VIRT_VIEWER_SESSION_SPICE_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o), VIRT_VIEWER_TYPE_SESSION_SPICE, VirtViewerSessionSpicePrivate))
> @@ -702,6 +703,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,
> @@ -726,6 +729,11 @@ virt_viewer_session_spice_display_monitors(SpiceChannel *channel,
>              display = virt_viewer_display_spice_new(self, channel, i);
>              g_debug("creating spice display (#:%d)", i);
>              g_ptr_array_index(displays, i) = g_object_ref_sink(display);
> +            if (fullscreen_mode) {
> +                gint nth = virt_viewer_display_get_nth(VIRT_VIEWER_DISPLAY(display));
> +                virt_viewer_display_set_fullscreen(VIRT_VIEWER_DISPLAY(display),
> +                                                   nth < self->priv->fullscreen_displays);

This is not correct. Due to user-specified fullscreen configuration,
it's possible to have e.g. 2 fullscreen displays enabled that are not
sequential: #1 and #3. So checking that nth is less than the number of
displays is not a valid solution.

> +            }
>              virt_viewer_session_add_display(VIRT_VIEWER_SESSION(self),
>                                              VIRT_VIEWER_DISPLAY(display));
>          }
> @@ -739,6 +747,12 @@ virt_viewer_session_spice_display_monitors(SpiceChannel *channel,
>          if (monitor->width == 0 || monitor->height == 0)
>              continue;
>  
> +        if (fullscreen_mode && !virt_viewer_display_get_fullscreen(VIRT_VIEWER_DISPLAY(display))) {
> +            g_debug("display #%d is not fullscreen, ignoring",
> +                    virt_viewer_display_get_nth(VIRT_VIEWER_DISPLAY(display)));
> +            continue;
> +        }
> +

I don't think that this is really an acceptable solution. What you seem
to be doing here is acknowledging that the guest display N is enabled,
but you're just ignoring it rather than showing it to the user. That
leaves an extra display (which may contain application windows, etc)
enabled on the guest but inaccessible to the user. This would be very
confusing to the user who wonders why they can't access one of their
applications that they know is open. It's even possible that the hidden
display might be the one with the start menu on it.

I think that we need the comprehensive fix that I've been struggling to
find for rhbz#1200750, assuming that these are related. But even if
these bugs are not related, I'm afraid this is not the right solution
here.


>          virt_viewer_display_set_enabled(VIRT_VIEWER_DISPLAY(display), TRUE);
>          virt_viewer_display_spice_set_desktop(VIRT_VIEWER_DISPLAY(display),
>                                                monitor->x, monitor->y,
> @@ -889,6 +903,7 @@ virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice *self)
>  
>      spice_main_send_monitor_config(cmain);
>      self->priv->did_auto_conf = TRUE;
> +    self->priv->fullscreen_displays = ndisplays;
>      return TRUE;
>  }
>