[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [PATCH 2/2] Set Spice display to fullscreen if owning window is pending fullscreen




----- Original Message -----
> 
> 
> ----- Original Message -----
> > From: "Marc-André Lureau" <mlureau redhat com>
> > To: "Jonathon Jongsma" <jjongsma redhat com>
> > Cc: "Marc-André Lureau" <marcandre lureau gmail com>, "virt"
> > <virt-tools-list redhat com>
> > Sent: Tuesday, October 15, 2013 8:07:04 AM
> > Subject: Re: [virt-tools-list] [PATCH 2/2] Set Spice display to fullscreen
> > if owning window is pending fullscreen
> > 
> > 
> > 
> > ----- Original Message -----
> > > 
> > > ----- Original Message -----
> > > > From: "Marc-André Lureau" <marcandre lureau gmail com>
> > > > To: "Jonathon Jongsma" <jjongsma redhat com>
> > > > Cc: "virt" <virt-tools-list redhat com>
> > > > Sent: Wednesday, October 9, 2013 3:29:22 PM
> > > > Subject: Re: [virt-tools-list] [PATCH 2/2] Set Spice display to
> > > > fullscreen
> > > > if owning window is pending fullscreen
> > > > 
> > > > To avoid introducing a new pending state, we could set
> > > > 
> > > > priv->fullscreen = TRUE;
> > > > 
> > > > before the delayed map-event, and in the handler, set it back to
> > > > FALSE. That really shouldn't be a problem, and since it's a
> > > > special/temporary case, I think that would be simpler.
> > > 
> > > 
> > > I'm not quite sure what you mean by "in the handler, set it back to
> > > FALSE".
> > 
> > static gboolean
> > mapped(GtkWidget *widget, GdkEvent *event G_GNUC_UNUSED,
> >        VirtViewerWindow *self)
> > {
> >     g_signal_handlers_disconnect_by_func(widget, mapped, self);
> >     priv->fullscreen = FALSE;
> >     virt_viewer_window_enter_fullscreen(self,
> >     self->priv->fullscreen_monitor);
> >     return FALSE;
> > }
> > 
> > 
> > > But in general I don't think it will really work to use a single boolean
> > > for this case. virt_viewer_enter_fullscreen() actually has an early
> > > return
> > > at the start of the function:
> > > 
> > >     if (priv->fullscreen)
> > >         return;
> > 
> > That's why we set it back in the map event.
> 
> 
> OK, I understand.  That would work around this particular issue. But in
> general I prefer being slightly more verbose and having the state easily
> understandable. I feel that flipping this variable back and forth makes the
> code just slightly harder to follow. (yes, it's a very minor quibble, but
> these things can add up).
> 
> 
> > 
> > > 
> > > As far as I can tell, this is here because after we enter fullscreen
> > > mode,
> > > we
> > > end up calling gtk_check_menu_item_set_active(check, TRUE), which can
> > > result
> > > in another call to _enter_fullscreen(). So this protects us from running
> > 
> > No, this loop can't happen, as gtk_check_menu_item_set_active() only
> > activates when the value changes.
> 
> Not true.  It can and does happen.  I've hit this breakpoint many times while
> debugging this issue.  Perhaps it only happens during this
> delayed-fullscreen scenario in startup auto-conf, but it does happen.
> 
>  
> > > this handler twice.  I could work around this in a few ways (e.g.
> > > blocking
> > > that signal handler while we call gtk_check_menu_item_set_active(), or
> > > turning priv->fullscreen into a tri-state variable rather than a
> > > boolean),
> > > but neither of those seemed obviously better to me than simply adding a
> > > new
> > > priv->pending_fullscreen state.
> > 
> > I still prefer not adding another object state when this one can be easily
> > confined locally.
> 
> 
> The one thing to consider here is that this could potentially affect
> behavior. Previously, any code that checked whether the window was in
> 'fullscreen' state between the calls to _enter_fullscreen() and mapped()
> would have gotten a FALSE value.  But if we change this state variable as
> you suggest, the value will be TRUE.  Thus any code that checks this state
> might take a different code branch. Perhaps this is harmless, but I'm not
> sure that I can guarantee that.

The delayed fullscreen is a small hack to workaround gnome2 metacity issue, I don't expect this to add up. If it happens, then an additional object state might be required.

The rest of the code should behave as if fullscreen state was TRUE.

> 
> Jonathon
> 
> 
> 
> 
> > > > On Wed, Oct 9, 2013 at 10:09 PM, Jonathon Jongsma <jjongsma redhat com>
> > > > wrote:
> > > > > When you call virt_viewer_window_enter_fullscreen() on a hidden
> > > > > window,
> > > > > it
> > > > > doesn't actually change its fullscreen state.  Instead, it sets up a
> > > > > map-event
> > > > > handler to enter fullscreen after it is shown. When _set_display() is
> > > > > called on
> > > > > a window that is pending fullscreen status, it initially sets the
> > > > > fullscreen
> > > > > state of the display to FALSE, which can cause an unwanted resize to
> > > > > be
> > > > > sent
> > > > > down to the guest.
> > > > > ---
> > > > >  src/virt-viewer-window.c | 6 +++++-
> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> > > > > index 0f62feb..7108aa0 100644
> > > > > --- a/src/virt-viewer-window.c
> > > > > +++ b/src/virt-viewer-window.c
> > > > > @@ -96,6 +96,7 @@ struct _VirtViewerWindowPrivate {
> > > > >      GSList *accel_list;
> > > > >      gboolean enable_mnemonics_save;
> > > > >      gboolean grabbed;
> > > > > +    gboolean fullscreen_pending;
> > > > >      gint fullscreen_monitor;
> > > > >      gboolean desktop_resize_pending;
> > > > >      gboolean kiosk;
> > > > > @@ -294,6 +295,7 @@ virt_viewer_window_init (VirtViewerWindow *self)
> > > > >      self->priv = GET_PRIVATE(self);
> > > > >      priv = self->priv;
> > > > >
> > > > > +    priv->fullscreen_pending = FALSE;
> > > > >      priv->fullscreen_monitor = -1;
> > > > >      priv->auto_resize = TRUE;
> > > > >      g_value_init(&priv->accel_setting, G_TYPE_STRING);
> > > > > @@ -533,11 +535,13 @@
> > > > > virt_viewer_window_enter_fullscreen(VirtViewerWindow
> > > > > *self, gint monitor)
> > > > >      priv->fullscreen_monitor = monitor;
> > > > >
> > > > >      if (!gtk_widget_get_mapped(priv->window)) {
> > > > > +        priv->fullscreen_pending = TRUE;
> > > > >          g_signal_connect(priv->window, "map-event",
> > > > >          G_CALLBACK(mapped),
> > > > >          self);
> > > > >          return;
> > > > >      }
> > > > >
> > > > >      priv->fullscreen = TRUE;
> > > > > +    priv->fullscreen_pending = FALSE;
> > > > >
> > > > >      gtk_check_menu_item_set_active(check, TRUE);
> > > > >      gtk_widget_hide(menu);
> > > > > @@ -1232,7 +1236,7 @@ virt_viewer_window_set_display(VirtViewerWindow
> > > > > *self, VirtViewerDisplay *displa
> > > > >          virt_viewer_display_set_zoom_level(VIRT_VIEWER_DISPLAY(priv->display),
> > > > >          priv->zoomlevel);
> > > > >          virt_viewer_display_set_auto_resize(VIRT_VIEWER_DISPLAY(priv->display),
> > > > >          priv->auto_resize);
> > > > >          virt_viewer_display_set_monitor(VIRT_VIEWER_DISPLAY(priv->display),
> > > > >          priv->fullscreen_monitor);
> > > > > -
> > > > > virt_viewer_display_set_fullscreen(VIRT_VIEWER_DISPLAY(priv->display),
> > > > > priv->fullscreen);
> > > > > +
> > > > > virt_viewer_display_set_fullscreen(VIRT_VIEWER_DISPLAY(priv->display),
> > > > > priv->fullscreen_pending || priv->fullscreen);
> > > > >
> > > > >          gtk_widget_show_all(GTK_WIDGET(display));
> > > > >          gtk_notebook_append_page(GTK_NOTEBOOK(priv->notebook),
> > > > >          GTK_WIDGET(display), NULL);
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > > > > _______________________________________________
> > > > > virt-tools-list mailing list
> > > > > virt-tools-list redhat com
> > > > > https://www.redhat.com/mailman/listinfo/virt-tools-list
> > > > 
> > > > 
> > > > 
> > > > --
> > > > Marc-André Lureau
> > > > 
> > > 
> > > _______________________________________________
> > > virt-tools-list mailing list
> > > virt-tools-list redhat com
> > > https://www.redhat.com/mailman/listinfo/virt-tools-list
> >
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list redhat com
> https://www.redhat.com/mailman/listinfo/virt-tools-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]