Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#3274) When a window is fullscreen AND it is not a background AND it is focused, then we don't render any layer above it #3337

Closed
wants to merge 1 commit into from

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Apr 19, 2024

fixes #3274

What's new?

  • If a window is fullscreen AND it is not a background AND it is focused, then we don't render any layer above it.

Question

While this is somewhat of a natural place to put this, I'm wondering if a better solution would be to move it between layers. But that would be kind of tricky in my opinion.

@mattkae mattkae changed the title WIP on surface stack (#3274) Windows marked "fullscreen" will now be above all other windows when they are focused Apr 19, 2024
@mattkae mattkae marked this pull request as ready for review April 19, 2024 18:17
@mattkae mattkae requested a review from a team as a code owner April 19, 2024 18:17
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.34%. Comparing base (f629688) to head (bbf3d52).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3337      +/-   ##
==========================================
- Coverage   77.35%   77.34%   -0.02%     
==========================================
  Files        1072     1062      -10     
  Lines       68216    67825     -391     
==========================================
- Hits        52771    52458     -313     
+ Misses      15445    15367      -78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this overall approach, but that's because I'm not sure what the correct behaviour is here.

For what it's worth, the windowing doc says:

When a window is full-screen:

  • It should exist with any descendants in its own temporary workspace and be visible only when you are in that workspace. (Any tips or glosses should not survive the mode change to full screen, and so should not be moved with the surface.)
  • It should normally occupy all of the display. If the window’s maximum width or maximum height is less than the width or height respectively of the display, the window should be centered and letterboxed and/or pillarboxed.
  • When the window is active, the Launcher should be forced into auto-hide mode.

That doesn't necessarily translate 100% to the modern world, but is roughly compatible with your approach here.

I'm happy for this to be an improvement on the status quo, but I'll let others weigh in too.

@AlanGriffiths
Copy link
Contributor

* If a window is fullscreen AND it is not a background AND it is focused, then we don't render any layer above it.

This is what the code does, but not what the headline says.

But I'm not sure this is correct: if we encounter a focussed, fullscreen surface in "below", then we don't paint a window in "overlay".

It can also cause confusion: while things above any fullscreen surface are not rendered, they are still above the surface and not transparent to input: there's no logic to ensure they do not receive pointer or touch events.

I suspect a better approach is to place fullscreen surfaces in a layer between "above" and "overlay".

@Saviq
Copy link
Collaborator

Saviq commented Apr 22, 2024

I suspect a better approach is to place fullscreen surfaces in a layer between "above" and "overlay".

The problem is changing focus to another, non-fullscreen window, then?

@AlanGriffiths
Copy link
Contributor

The problem is changing focus to another, non-fullscreen window, then?

Only if you think z-order & layers control changing focus

@Saviq
Copy link
Collaborator

Saviq commented Apr 22, 2024

Only if you think z-order & layers control changing focus

Sorry, wasn't clear.

If you have a fullscreen window between top and overlay, and then you Alt+Tab to a non-fullscreen window. You'd need to move the unfocused fullscreen window back to the regular layer to show the shell components again.

That is, unless we actually implemented what the windowing document calls for - placing the fullscreen window on a separate workspace.

@mattkae mattkae changed the title (#3274) Windows marked "fullscreen" will now be above all other windows when they are focused (#3274) When a window is fullscreen AND it is not a background AND it is focused, then we don't render any layer above it Apr 22, 2024
@mattkae
Copy link
Contributor Author

mattkae commented Apr 22, 2024

Alan is correct in this being a faulty implementation. Panels and whatnot will still receive input even though they are not rendered.

According to the design doc, the fullscreen window sounds like it is to be considered as its own layer (disregarding the workspace bit). While looking at a fullscreen window, it should take up the whole screen and hide all others. So perhaps the following is appropriate:

  • When a window is requested to be fullscreen, it remains in the same layer
  • When a fullscreen window is focused, it moves to a new layer (between mir_depth_layer_above and mir_depth_layer_overlay)
  • When the window loses focus, it returns to its previous layer, in the position after the now-focused window (if they are in the same layer, otherwise the topmost positon)

We would most likely require some new notifications here, but this would be more correct.

@Saviq
Copy link
Collaborator

Saviq commented Apr 23, 2024

Alan is correct in this being a faulty implementation. Panels and whatnot will still receive input even though they are not rendered.

According to the design doc, the fullscreen window sounds like it is to be considered as its own layer (disregarding the workspace bit). While looking at a fullscreen window, it should take up the whole screen and hide all others.

It sounds to me like the layer shell protocol does not have enough to serve us here. Consider a shell that wants to pull the shell components in on "pushing" against an edge. I can't really see any way other than notifying them that the foreground app is in fullscreen and let them hide on their own. Separate workspace or not.

I suppose that behaviour could be implemented in-compositor for edge-anchored layer-shell surfaces, by moving them to the overlay layer and pushing them off-screen. That would be fighting against the protocol, though. Could also keep the layers as they are and hide/deactivate any non-edge-anchored surfaces?

So perhaps the following is appropriate:
--8<--

  • When a fullscreen window is focused, it moves to a new layer (between mir_depth_layer_above and mir_depth_layer_overlay)

It could be, if we conceded that you never get access to panels when in fullscreen. At least for now.

  • When the window loses focus, it returns to its previous layer, in the position after the now-focused window

*below

@RAOF
Copy link
Contributor

RAOF commented Apr 23, 2024

Hm. Would that be easier than actually implementing the design as documented? Switch to an alternate stack when a fullscreen window is focused?

The bits that I think we might care about would be:

  • OSKs need to be visible when necessary (but if I understand it correctly they don't take input focus anyway)
  • (Optional) New notifications need to be visible over the top

I don't think it's worth trying to handle the combination of fullscreen & panels that want to slide in, even over a fullscreen surface. (For a point of reference, GNOME Shell does not try to do this over fullscreen windows)

@AlanGriffiths
Copy link
Contributor

I think we need to clarify the requirements before discussing solutions.

The linked issue shows that there are usecases where panels should be obscured by fullscreen apps (at least when focussed).

But that is not the full story. We have, for example, hacked "fullscreen" in Frame so that the OSK forces the application to resize. That is a usecase where "fullscreen" intentionally doesn't overlay a panel.

As far as layer-shell goes there's not much in the way of semantic information to go on. But panels in background or below are clearly obscured by a fullscreen surface: Maybe configuring waybar to be "below" is enough for the bug?

@Saviq
Copy link
Collaborator

Saviq commented Apr 23, 2024

Maybe configuring waybar to be "below" is enough for the bug?

But then if you move a restored window up, it will cover the panel…

@AlanGriffiths
Copy link
Contributor

Maybe configuring waybar to be "below" is enough for the bug?

But then if you move a restored window up, it will cover the panel…

Which is why the requirements need nailing down

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[this comment intentionally blank]

@mattkae
Copy link
Contributor Author

mattkae commented Apr 23, 2024

I agree with @AlanGriffiths that this needs nailing down. I will make a proper design doc for this one so that we can dissect the requirements and the possible solutions in a good manner. That sounds like something for next pulse though, as it might eat up a lot of time. I'll have it ready for when we see each other during the sprint.

@mattkae
Copy link
Contributor Author

mattkae commented Apr 23, 2024

Or rather, I will do the design doc this pulse

@mattkae
Copy link
Contributor Author

mattkae commented Apr 23, 2024

Closing this for now, as we need some designing :)

@mattkae mattkae closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows marked mir_window_state_fullscreen appear behind shell components
4 participants