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

[Pavel Grunt]

Hi Jonathon,
> 
> 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.
> 

I started working on it, because your patches for rhbz#1200750 didn't affect this bug.

> > ---
> >  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.
> 

My bad, thank you.

> > +            }
> >              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.
> 

Right, I should make sure that the display will be disabled on the guest side.
I probably didn't have the problem you described because the display
was set disabled when the monitor-geometry-changed.

> 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.
> 

My assumption was that if we know which displays should be disabled,
we can keep them disabled.



Thanks a lot for your comments,
Pavel