I can confirm the part with the panel

*** Bug 659323 has been marked as a duplicate of this bug. ***

*** Bug 682610 has been marked as a duplicate of this bug. ***

*** Bug 676177 has been marked as a duplicate of this bug. ***

I spent some time looking into a particular issue with fullscreen, reported as:

 https://bugzilla.redhat.com/show_bug.cgi?id=920697

which is supposed to be a simulation of how LibreOffice Impress goes fullscreen. In that case, the problem turned out to be that the window was *both*:

 A) Set fullscreen via the EWMH
 B) Set up to trigger "legacy fullscreen" operation - undecorated and the size of the monitor

A) sets window->fullscreen_after_placement, and in that case, the window gets assigned to the FULLSCREEN layer correctly. In B) the fullscreening happens immediately, and the window is not initially set to the fullscreen layer because it's not the focus window at the point where the layer assignment happens.

Note that fullscreen_after_placement isn't done because of this, but so that we have a "placed" position for a fullscreen window to go back to when it unfullscreens - so it doesn't unfullscreen to the full screen size.

GNOME Shell is looking for the FULLSCREEN layer to determine if it should hide its top panel.

I'm not sure yet if a) the layer is never set to FULLSCREEN or b) the layer is set to FULLSCREEN later but gnome-shell doesn't notice.

So, possible fixes:

 A) Fix up layer tracking so that things work correctly if we fullscreen during placement.

 B) Set fullscreen_after_placement if we find a legacy fullscreen window during placement.

I think A) is probably better - B) feels like working around bugs in layer tracking that will come back to haunt us later.

(In reply to comment #5)
> I'm not sure yet if a) the layer is never set to FULLSCREEN or b) the layer is
> set to FULLSCREEN later but gnome-shell doesn't notice.

I'm pretty sure now that it's b). The problem is that GNOME Shell is updating the fullscreen status by connecting to the MetaScreen::restacked. That is driven off of MetaStackTracker and accurately reflects when windows are actually restacked. But it's not going to notice when a window changes it's layer without changing the position in the stack. (It also probably won't notice if a minimized fullscreen window that was on top of the stack unminimizes without any restacking.)

Created attachment 238730 [details] [review]
Improve tracking of fullscreen windows

It's possible in some corner cases for the status of the topwindow
to change and make it not fullscreen without ::restacked being
changed. One way that it could happen with the old code was if the
layer of the top window changed from NORMAL to FULLSCREEN.

Change the logic not to look at the layer, which is a function of
Mutter's *intended* stacking, rather than the *actual* stacking,
which is what ::restacked gives you. Instead, look at the top
portion of the stack, down to the first non-override-redirect
window, and see if their are any monitor-sized windows there.

Connect to changes on the top portion of the stack, so we know
if conditions change.

Things I'm still not 100% happy with:

 * With my patch, if the top fullscreen window is on another display, and there is a separate fullscreen window on the primary display, then the code considers the fullscreen window on the primary display as not fullscreen, and doesn't hide the chrome. This is sort of in-sync with the code in Mutter that reserves the FULLSCREEN layer for *focused* fullscreen windows, and drops other fullscreen windows into the NORMAL layer, but it's going to look weird to the user. But, the code in _windowsRestacked() to minimize obscured fullscreen windows is going to hide the second fullscreen window anyways.

 * The code in _windowsRestacked() to minimize obscured full-screen windows is not in-sync with the definition here in terms of override-redirect windows - it considers everything but the very topmost window "obscured".

 * The code in _windowsRestacked() to minimize obscured full-screen windows can try to minimize O-R windows.

The second two should probably be fixed, the first maybe can wait until someone complains.

Review of attachment 238730 [details] [review]:

::: js/ui/layout.js
@@ +949,3 @@
+
+                function connect(o, signal, callback) {
+                    connections.push(o);

Just use a record instead of array indexes, and use destructuring to unpack it:

    connections.push({ object: o, signal: o.connect(...) });

    // ...

    connections.forEach(function(record) {
        let { object: object, signal: signal } = record;
        object.disconnect(record);
    });

Although I'm not sure the genericism adds much.

@@ +958,3 @@
+                connect(window.metaWindow, 'notify::fullscreen', this._topActorChanged);
+                connect(window, 'position-changed', this._topActorChanged);
+                connect(window, 'size-changed', this._topActorChanged);

Why position-changed / size-changed? To track when the monitors of a window change? Why not window-left-monitor/window-entered-monitor?

(In reply to comment #9)
> Review of attachment 238730 [details] [review]:
> 
> ::: js/ui/layout.js
> @@ +949,3 @@
> +
> +                function connect(o, signal, callback) {
> +                    connections.push(o);
> 
> Just use a record instead of array indexes, and use destructuring to unpack it:
> 
>     connections.push({ object: o, signal: o.connect(...) });
> 
>     // ...
> 
>     connections.forEach(function(record) {
>         let { object: object, signal: signal } = record;
>         object.disconnect(record);
>     });
> 
> Although I'm not sure the genericism adds much.

Probably a cleaner to do it that way. The generic code is a little questionable - it avoided a bunch of tedious typing to store 4 separate signal handlers and reduced the chance of typos, but does add extra burden to the reader to understand it.

> @@ +958,3 @@
> +                connect(window.metaWindow, 'notify::fullscreen',
> this._topActorChanged);
> +                connect(window, 'position-changed', this._topActorChanged);
> +                connect(window, 'size-changed', this._topActorChanged);
> 
> Why position-changed / size-changed? To track when the monitors of a window
> change? Why not window-left-monitor/window-entered-monitor?

Override redirect windows are monitor-sized or not depending on their position and size.

Review of attachment 238730 [details] [review]:

Looks good to me, maybe we could so something more readable instead of the "even / odd" index thing in the array.

::: js/ui/layout.js
@@ +951,3 @@
+                    connections.push(o);
+                    connections.push(
+                        o.connect(signal, Lang.bind(me, callback, window))

Would be easier to read if you did something like

connections.push({ obj: o, connection: o.connect(...)});

and then 

connections.forEach(function(c) {
    c.obj.disconnect(c.connection);
});

while disconnecting them.

OK too slow ...

Created attachment 238738 [details] [review]
* Clever disconnection removed in favor of just writing it out

long-hand. (-20 +24 for line count, if curious)
* Restricted the connection to resize/reposition to O-R windows.
  Though not 100% correct with the current meta_window_is_monitor_sized()
  it avoids extra code running on the window-dragging path.

Created attachment 238740 [details] [review]
Improve tracking of fullscreen windows

Actually include all the changes described last time.

Review of attachment 238740 [details] [review]:

Looks good.

Comment on attachment 238740 [details] [review]
Improve tracking of fullscreen windows

It's possible this fix fixes everything, but leaving the bug open
for the moment - there's a lot of diversity in the bugs that have
been dup'ed here, which might also include problems with games
that change screen resolution.

Attachment 238740 [details] pushed as 6119b44 - Improve tracking of fullscreen windows

OK found another bug / regression ... I can no longer fullscreen flash videos.
Try to fullscreen a video it will just flicker and go back to the firefox window.

(In reply to comment #17)
> OK found another bug / regression ... I can no longer fullscreen flash videos.
> Try to fullscreen a video it will just flicker and go back to the firefox
> window.

Rverting 6119b44 fixes it.

Created attachment 238947 [details] [review]
MetaScreen: Add tracking of whether there are fullscreen windows

As I tried to deal with edge cases, it became very unclear how to make
this code robust within GNOME shell. As one example, the code in
gnome-shell was looking at fullscreen_window.appears_focused to
know determine whether to auto-minimize it or not, which is roughly
similar to part of the check in mutter/src/core/stack.c:

      else if (window->fullscreen &&
               (focused_transient ||
                window == window->display->expected_focus_window ||
                window->display->expected_focus_window == NULL ||
                (window->display->expected_focus_window != NULL &&
                 windows_on_different_monitor (window,
                                               window->display->expected_focus_window))))

But far from not identical. And it's certainly not the case that the
::windows-restacked handler gets run when the above expression
changes. Unless everything is really perfectly in sync, we'll easily
get situations where we'll run _updateFullscreen at the wrong time,
see a wrong temporary state, and auto-minimize a window that shouldn't
be autominimized.

The two patches here move both tracking of fullscreen windows and
auto-minimization into Mutter, and then use that from GNOME Shell.

Mutter:       8 files changed, 198 insertions(+), 2 deletions(-)
GNOME Shell:  1 file changed, 7 insertions(+), 163 deletions(-)

Things I'm not perfectly happy about (other than the scope of the
changes.)

 * The force-updating of fullscreen window layers in
   check_fullscreen_func. It would be better to just keep them
   correctly updated all the time - basic plan for that:

   Have screen->fullscreen_windows, window->acting_fullscreen,
   meta_screen_update_acting_fullscreen(), and just call that
   extremely frequently when anythign in the above predicate
   could have changed. (Any time focus changes, or a 

 * The hardcoding of auto-minimization - this seems specific
   to GNOME Shell - having a signal when a fullscreen window loses
   "acting fullscreen" might make more sense.

Still, if this version holds up to review and testing, it may be the
best thing to do for 3.8. Alternatives:

 - Revert my patches and just accept that LibreOffice Impress and other
   fullscreening will *still* be broken after all this time in 3.8.

 - Defer auto-minimization to an idle and hope for the best. It probably
   would *usually* work, but window.appears_focused is actually set
   in response to focus events, so their is no particular guarantee
   that it will be present even after idle. So you could maybe switch
   from appears_focused to checking display->expected_focus_window
   (set synchronously), but that isn't exposed, so ... argh!

======

Trying to track the fullscreen status outside of Mutter, as GNOME Shell
was doing previously, was very prone to errors, because Mutter has a
very tricky definition of when a window is set to be fullscreen and
*actually* acting like a fullscreen window.

 * Add meta_screen_get_monitor_in_fullscreen() and an
   ::in-fullscreen-changed signal. This allows an application to
   track when there are fullscreen windows on a monitor.

 * Do the computation of fullscreen status in a "later" function that
   runs after showing, so we properly take focus into account.

 * To get ordering of different phases right, add more values
   to MetaLaterType.

 * Add auto-minimization, similar to what was added to GNOME Shell
   earlier in this cycle - if a window is set to be fullscreen, but
   not actually fullscreen, minimize.

I've tested with ooimpress, gedit (including with dialogs open),
youtube videos, overide-redirect windows. I haven't done
multihead testing, though I'll check that quickly tonight at home
where I have a cable.

I should note that auto-minimization is a bit different than was
implemented in GNOME Shell - it doesn't care about the primary
window. E.g.

  OOImpress on primary with presentation open on second display, 
  Alt-tab to another window on the second display.
 
  Old: presentation stays fullscreen, other window put on top.
  New: presentation minimized

Created attachment 238948 [details] [review]
Use Mutter fullscreen window tracking for hiding chrome and auto-minimize

Getting fullscreen window tracking right in GNOME Shell turned out to
be very hard, because it depended on details both how Mutter handled
fullscreen windows and the exact timing of that. Fullscreen tracking
and auto-minimization of fullscreen windows that lose their fullscreen
status has thus been implemented in Mutter: use that.

I tested with multi-monitors and my patches worked as expected on the second monitor, and the behavior (as described above) seemed like an OK one.

[
auto-minimize in general feels not quite right to me though it's better than nothing. What I'd expect:

 A) App has a fullscreen toggle - when you alt-tab away from the app, it unfullscreens, and then fullscreens back again when you alt-tab back to it.

 B) Separate window full fullscreen. Something like auto-minimize but when the main app regains focus, it reshows the presentation window instead of the user having to search for it and go to it explicitly.

A digression, however.
]

Review of attachment 238947 [details] [review]:

Code looks good and it seems fine in testing (every case I tried worked fine) but I have only tested with one monitor though.

Review of attachment 238948 [details] [review]:

Looks good.

::: js/ui/layout.js
@@ +901,1 @@
+        this.emit('fullscreen-changed');

Can we just get rid of this and just use in-fullscreen-changed everywhere where we need it? Should be a separate patch though.

(In reply to comment #21)
> I tested with multi-monitors and my patches worked as expected on the second
> monitor, and the behavior (as described above) seemed like an OK one.
> 
> [
> auto-minimize in general feels not quite right to me though it's better than
> nothing. What I'd expect:
> 
>  A) App has a fullscreen toggle - when you alt-tab away from the app, it
> unfullscreens, and then fullscreens back again when you alt-tab back to it.
> 
>  B) Separate window full fullscreen. Something like auto-minimize but when the
> main app regains focus, it reshows the presentation window instead of the user
> having to search for it and go to it explicitly.
> 
> A digression, however.
> ]

IRC discussion:

<drago01> owen: re your comment in the fullscreen bug ... can we unfullscreen an app behind its back?
<drago01> owen: not sure it is "legal" (as per spec) to unfullscreen it even though it asks for fullscreen
<halfline> mutter exposes a keybinding to unfullscreen a window
<halfline> so we already do it if the keybinding is configured and the user hits the key
<drago01> halfline: ok, so we could just unfullscreen fullscreen windows when they are no longer on top of the stack instead of minimizing them
* weld (~weld@p5488DBA0.dip.t-dialin.net) hat #gnome-shell betreten
<drago01> halfline: and then restore the state once they get focus again
<halfline> i don't know the context (haven't read the bug), but that sounds like a reasonable behavior to me. i wouldn't be surprised if some apps would break from being minimized and some apps would break from being unfullscreened though
<halfline> i guess minimize is little less risky than it would be with say metacity, since we don't unmap on minimize

*** Bug 696079 has been marked as a duplicate of this bug. ***

The patches do address the issue for me when trying to run maximized Blender window.

Comment on attachment 238947 [details] [review]
MetaScreen: Add tracking of whether there are fullscreen windows

Attachment 238947 [details] pushed as 5ceffe8 - MetaScreen: Add tracking of whether there are fullscreen windows

Created attachment 239183 [details] [review]
Remove LayoutManager::fullscreen-changed

Since we now have global.screen::in-fullscreen-changed, remove the
duplicate signal. To prevent ordering problems in connecting to
this signal, make inFullscreen a property-function of a new Monitor
object rather than a data property we tack on to a Rectangle object.

Comment on attachment 238948 [details] [review]
Use Mutter fullscreen window tracking for hiding chrome and auto-minimize

Attachment 238948 [details] pushed as a6b4d49 - Use Mutter fullscreen window tracking for hiding chrome and auto-minimize

Review of attachment 239183 [details] [review]:

OK.

Review of attachment 239183 [details] [review]:

Looks good.

Not sure this should be on the 3.8.1 list - the reported issue has been fixed, the remaining patch just cleans up some code. (It's also removing JS API, so there's the potential of breaking extensions in a minor release)

fair enough, moving it off

Attachment 239183 [details] pushed as 9ae2440 - Remove LayoutManager::fullscreen-changed