[virt-tools-list] [PATCH 2/2] Set Spice display to fullscreen if owning window is pending fullscreen
Jonathon Jongsma
jjongsma at redhat.com
Tue Oct 15 15:01:14 UTC 2013
----- Original Message -----
> From: "Marc-André Lureau" <mlureau at redhat.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>
> Cc: "Marc-André Lureau" <marcandre.lureau at gmail.com>, "virt" <virt-tools-list at 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 at gmail.com>
> > > To: "Jonathon Jongsma" <jjongsma at redhat.com>
> > > Cc: "virt" <virt-tools-list at 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.
Jonathon
> > > On Wed, Oct 9, 2013 at 10:09 PM, Jonathon Jongsma <jjongsma at 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 at redhat.com
> > > > https://www.redhat.com/mailman/listinfo/virt-tools-list
> > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> > >
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
More information about the virt-tools-list
mailing list