[virt-tools-list] --fullscreen=auto-conf issues

[Marc-André Lureau]

On Wed, Oct 9, 2013 at 10:09 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
>
> I don't know if anybody else has noticed, but --fullscreen=auto-conf is broken
> in git. For example, in a two-monitor case, what happens is that two display

It went mostly unnoticed because those changes are recent, and not in
an official release afaik. It's the way development works, you keep
fixing the released version while nobody check the git one - release
often! :)

> windows are created, and they are fullscreened on each monitor, but the
> resolution does not match the window size.  They both tend to be very
> low-resolution displays that are then zoomed up to fill the full screen.
>
> I've been investigating the cause, and it turns out that the cause is a fairly
> complex set of interactions. First of all, these are the commits that broke this
> functionality:
>
> 9da9305 - Use display fullscreen state instead of app state
>  - Causes the secondary displays to be the wrong resolution
> 323d85d - spice: if zoom-level is changed, resize guest, even in fullscreen
>  - Causes the primary display to be the wrong resolution
>
> The problem with the primary monitor is solved by this earlier patch:
>
>   https://www.redhat.com/archives/virt-tools-list/2013-October/msg00047.html

which is already acked, but you should have put the rationale there,
not in a different thread.

> Ideally we should probably make the code less fragile in response to extraneous
> zoom-level change notifications.

You state the obvious.

> The reason that the behavior of the primary display window is different than the
> secondary (and following) display windows is because the first window is always
> shown (and thus full-screened) before any Spice displays are created. All
> following windows are created in response to new monitor notifications from the
> spice server. Or, to state it more simply: the first VirtViewerDisplay object is
> added to an already-mapped and properly-sized fullscreen window, whereas all
> future VirtViewerDisplay objects are added to hidden and unconfigured windows.
> This turns out to be quite important.
>
> So, here is the sequence of events that results in the secondary displays having
> incorrect resolution:
>
> - There is some functionality in VirtViewerDisplaySpice that attempts to prevent
>   itself from sending down a resize command to the guest in response to
>   allocations when we are in fullscreen=auto-conf mode. Unfortunately, this
>   doesn't work properly because it depends on setting a local state variable
>   inside of a "notify::fullscreen" signal handler.
> - unfortunately, this "notify::fullscreen" signal handler doesn't get called
>   before the first allocation, because during the display setup, there's an
>   explicit call to gtk_widget_realize() (which causes an allocation) between
>   calls to g_object_freeze_notify() and g_object_thaw_notify(). Therefore, the
>   first allocation happens before we can synchronize our fullscreen state
>   properly, and as a result, we send a resize command down to the server (with a
>   resolution of 100x198).

ok, can't it be blocked at this setup time? I think the rationale
should be in the commit, if it's fixed by the following commit.

> - When the VirtViewerDisplaySpice object is created, a VirtViewerWindow is
>   created for it, and the display is added to this window. The window sets
>   various properties on the display to match its own state (e.g. zoom level,
>   auto-resize, and fullscreen).  Unfortunately, although we called
>   virt_viewer_window_enter_fullscreen() on the newly-created window, it hasn't
>   taken effect yet.  when you call virt_viewer_window_enter_fullscreen() on a
>   hidden window, it doesn't actually change its priv->fullscreen state variable.
>   Instead, it sets up a "map-event" handler which tries to enter fullscreen
>   after the window is mapped. So, until the window is displayed, it thinks it's
>   not in fullscreen mode.  So, when the window attempts to set display's
>   fullscreen mode to match its own fullscreen mode, it passes priv->fullscreen,
>   which remains FALSE.
>
> The simplest fix for the issue is show by the attached patches, but I'm afraid
> that these small changes could result in regressions in other areas, since this
> startup code is rather fragile.  Any thoughts?

No, any ideas (even high level) to make it "less fragile"? :)


-- 
Marc-André Lureau