-
Notifications
You must be signed in to change notification settings - Fork 159
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
xdg: handle initially maximized xdg-shell views better #1956
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, initially maximized (or fullscreen) xdg-shell views exhibit one of two issues: - some (e.g. GTK and Qt apps) paint an initial frame un-maximized (before the "map" event) and only maximize in a later commit - others (e.g. foot) maximize immediately without flicker, but never store a valid natural size, so we end up using a fallback (640x480) Under KWin, neither of these issues occur, so I looked into what labwc is doing wrong. It seems that: - wlroots internally sends an initial configure event with a size of 0x0 to all xdg-shell views. This requests the client to set its own preferred (a.k.a. natural) size. - For an initially maximized/fullscreen view, the initial configure event should contain the maximized/fullscreen size rather than 0x0. In labwc, this means we have to call wlr_xdg_toplevel_set_size() earlier, i.e. from the new_surface event. Tracing with WAYLAND_DEBUG shows that the initial configure event now has the correct geometry, matching KWin behavior. With this change, GTK and Qt apps no longer paint an incorrect un-maximized frame. - However, this means that all xdg-shell views now suffer from the same issue as foot, where we never receive a commit with the un-maximized (natural) geometry. The correct way to get the natural geometry seems to be to wait until we want to un-maximize, and send a configure event of 0x0 at that point. Sending a configure event of 0x0 when un-maximizing is a bit annoying as it breaks some assumptions in labwc code. In particular: - view->natural_geometry may now be unknown (0x0), requiring various wlr_box_empty() checks sprinkled around. I added these in all the obvious places, but there could be some code paths that I missed. - Positioning the newly un-maximized view within view_maximize() no longer works since we don't know the natural size. Instead we have to run the positioning logic from the surface commit handler. This results in some extra complexity, especially for interactive move. See the new do_late_positioning() function in xdg.c. Some TODOs/FIXMEs (non-blocking in my opinion): - The view_wants_decorations() check is now duplicated in both the new_surface and map event handlers. I'm not sure if this is necessary but it seemed like the safest approach for now. More testing would be nice, particularly with various combinations of config and client SSD preferences. - Aside from the interactive move case, the "late positioning" logic always centers the view when un-maximizing, and does not invoke any of the smart placement logic. If we want to invoke smart placement here, I'd appreciate someone with more knowledge of that code to take a look and figure out how to do that correctly.
This is technically a "fix", but due to how invasive it is, probably not 0.7.3 material. |
jlindgren90
added a commit
to jlindgren90/labwc
that referenced
this pull request
Jul 2, 2024
Currently, initially maximized (or fullscreen) xdg-shell views exhibit one of two issues: - some (e.g. GTK and Qt apps) paint an initial frame un-maximized (before the "map" event) and only maximize in a later commit - others (e.g. foot) maximize immediately without flicker, but never store a valid natural size, so we end up using a fallback (640x480) Under KWin, neither of these issues occur, so I looked into what labwc is doing wrong. It seems that: - wlroots internally sends an initial configure event with a size of 0x0 to all xdg-shell views. This requests the client to set its own preferred (a.k.a. natural) size. - For an initially maximized/fullscreen view, the initial configure event should contain the maximized/fullscreen size rather than 0x0. In labwc, this means we have to call wlr_xdg_toplevel_set_size() earlier, i.e. from the new_surface event. Tracing with WAYLAND_DEBUG shows that the initial configure event now has the correct geometry, matching KWin behavior. With this change, GTK and Qt apps no longer paint an incorrect un-maximized frame. - However, this means that all xdg-shell views now suffer from the same issue as foot, where we never receive a commit with the un-maximized (natural) geometry. The correct way to get the natural geometry seems to be to wait until we want to un-maximize, and send a configure event of 0x0 at that point. Sending a configure event of 0x0 when un-maximizing is a bit annoying as it breaks some assumptions in labwc code. In particular: - view->natural_geometry may now be unknown (0x0), requiring various wlr_box_empty() checks sprinkled around. I added these in all the obvious places, but there could be some code paths that I missed. - Positioning the newly un-maximized view within view_maximize() no longer works since we don't know the natural size. Instead we have to run the positioning logic from the surface commit handler. This results in some extra complexity, especially for interactive move. See the new do_late_positioning() function in xdg.c. Some TODOs/FIXMEs (non-blocking in my opinion): - The view_wants_decorations() check is now duplicated in both the new_surface and map event handlers. I'm not sure if this is necessary but it seemed like the safest approach for now. More testing would be nice, particularly with various combinations of config and client SSD preferences. - Aside from the interactive move case, the "late positioning" logic always centers the view when un-maximizing, and does not invoke any of the smart placement logic. If we want to invoke smart placement here, I'd appreciate someone with more knowledge of that code to take a look and figure out how to do that correctly. labwc#1956
jlindgren90
added a commit
to jlindgren90/labwc
that referenced
this pull request
Jul 2, 2024
Currently, initially maximized (or fullscreen) xdg-shell views exhibit one of two issues: - some (e.g. GTK and Qt apps) paint an initial frame un-maximized (before the "map" event) and only maximize in a later commit - others (e.g. foot) maximize immediately without flicker, but never store a valid natural size, so we end up using a fallback (640x480) Under KWin, neither of these issues occur, so I looked into what labwc is doing wrong. It seems that: - wlroots internally sends an initial configure event with a size of 0x0 to all xdg-shell views. This requests the client to set its own preferred (a.k.a. natural) size. - For an initially maximized/fullscreen view, the initial configure event should contain the maximized/fullscreen size rather than 0x0. In labwc, this means we have to call wlr_xdg_toplevel_set_size() earlier, i.e. from the new_surface event. Tracing with WAYLAND_DEBUG shows that the initial configure event now has the correct geometry, matching KWin behavior. With this change, GTK and Qt apps no longer paint an incorrect un-maximized frame. - However, this means that all xdg-shell views now suffer from the same issue as foot, where we never receive a commit with the un-maximized (natural) geometry. The correct way to get the natural geometry seems to be to wait until we want to un-maximize, and send a configure event of 0x0 at that point. Sending a configure event of 0x0 when un-maximizing is a bit annoying as it breaks some assumptions in labwc code. In particular: - view->natural_geometry may now be unknown (0x0), requiring various wlr_box_empty() checks sprinkled around. I added these in all the obvious places, but there could be some code paths that I missed. - Positioning the newly un-maximized view within view_maximize() no longer works since we don't know the natural size. Instead we have to run the positioning logic from the surface commit handler. This results in some extra complexity, especially for interactive move. See the new do_late_positioning() function in xdg.c. Some TODOs/FIXMEs (non-blocking in my opinion): - The view_wants_decorations() check is now duplicated in both the new_surface and map event handlers. I'm not sure if this is necessary but it seemed like the safest approach for now. More testing would be nice, particularly with various combinations of config and client SSD preferences. - Aside from the interactive move case, the "late positioning" logic always centers the view when un-maximizing, and does not invoke any of the smart placement logic. If we want to invoke smart placement here, I'd appreciate someone with more knowledge of that code to take a look and figure out how to do that correctly. labwc#1956
John. Thanks for the research+patch.
It'd be really nice to get this fixed, but yeah, shall we look to merge
after the release.
Should we discuss/consider changing that upstream initial 0x0?
…On Tue, 2 Jul 2024, 23:35 John Lindgren, ***@***.***> wrote:
Currently, initially maximized (or fullscreen) xdg-shell views exhibit one
of two issues:
- some (e.g. GTK and Qt apps) paint an initial frame un-maximized
(before the "map" event) and only maximize in a later commit
- others (e.g. foot) maximize immediately without flicker, but never
store a valid natural size, so we end up using a fallback (640x480)
Under KWin, neither of these issues occur, so I looked into what labwc is
doing wrong. It seems that:
-
wlroots internally sends an initial configure event with a size of 0x0
to all xdg-shell views. This requests the client to set its own preferred
(a.k.a. natural) size.
-
For an initially maximized/fullscreen view, the initial configure
event should contain the maximized/fullscreen size rather than 0x0. In
labwc, this means we have to call wlr_xdg_toplevel_set_size() earlier, i.e.
from the new_surface event. Tracing with WAYLAND_DEBUG shows that the
initial configure event now has the correct geometry, matching KWin
behavior. With this change, GTK and Qt apps no longer paint an incorrect
un-maximized frame.
-
However, this means that all xdg-shell views now suffer from the same
issue as foot, where we never receive a commit with the un-maximized
(natural) geometry. The correct way to get the natural geometry seems to be
to wait until we want to un-maximize, and send a configure event of 0x0 at
that point.
Sending a configure event of 0x0 when un-maximizing is a bit annoying as
it breaks some assumptions in labwc code. In particular:
-
view->natural_geometry may now be unknown (0x0), requiring various
wlr_box_empty() checks sprinkled around. I added these in all the obvious
places, but there could be some code paths that I missed.
-
Positioning the newly un-maximized view within view_maximize() no
longer works since we don't know the natural size. Instead we have to run
the positioning logic from the surface commit handler. This results in some
extra complexity, especially for interactive move. See the new
do_late_positioning() function in xdg.c.
Some TODOs/FIXMEs (non-blocking in my opinion):
-
The view_wants_decorations() check is now duplicated in both the
new_surface and map event handlers. I'm not sure if this is necessary but
it seemed like the safest approach for now. More testing would be nice,
particularly with various combinations of config and client SSD preferences.
-
Aside from the interactive move case, the "late positioning" logic
always centers the view when un-maximizing, and does not invoke any of the
smart placement logic. If we want to invoke smart placement here, I'd
appreciate someone with more knowledge of that code to take a look and
figure out how to do that correctly.
------------------------------
You can view, comment on, or merge this pull request online at:
#1956
Commit Summary
- 23b08ca
<23b08ca>
xdg: handle initially maximized xdg-shell views better
File Changes
(6 files <https://github.com/labwc/labwc/pull/1956/files>)
- *M* include/labwc.h
<https://github.com/labwc/labwc/pull/1956/files#diff-21f0a58e6b42e5d58fa90be002287bfc19d1865132db90575c80c06069b1309e>
(9)
- *M* include/view.h
<https://github.com/labwc/labwc/pull/1956/files#diff-56f6953833ef00fc3d98067561e4102da0731720a4f25a24d000b3f260bc6fb8>
(1)
- *M* src/interactive.c
<https://github.com/labwc/labwc/pull/1956/files#diff-61e80078fadc2d1cb4cbee03d2cbbc5fd9af6101e0e0b1c780573b17cfd1fbc8>
(27)
- *M* src/view.c
<https://github.com/labwc/labwc/pull/1956/files#diff-662ec1e8c57906cca33cf41b09c520a818a03e8c7454538e3529869b8c2b24b4>
(41)
- *M* src/xdg.c
<https://github.com/labwc/labwc/pull/1956/files#diff-4f2afbd6177320f6a3d49cfe49f71ce9a2a9fa69d0e8417cfd3933262564ea7b>
(95)
- *M* src/xwayland.c
<https://github.com/labwc/labwc/pull/1956/files#diff-4f89603972abcd15cc677cf88a8cec80a241a824fd6c3a41963fe7b6796ea2f3>
(16)
Patch Links:
- https://github.com/labwc/labwc/pull/1956.patch
- https://github.com/labwc/labwc/pull/1956.diff
—
Reply to this email directly, view it on GitHub
<#1956>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVCCWMQV7WVD6JV4ZIB57TZKMMKDAVCNFSM6AAAAABKIKY2TKVHI2DSMVQWIX3LMV43ASLTON2WKOZSGM4DOMRQG4ZDCOA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
I think the current wlroots behavior is fine (though it could be documented better). The 0x0 is correct for floating views to query the natural geometry, and the compositor can override it (as in this PR). |
jlindgren90
added a commit
to jlindgren90/labwc
that referenced
this pull request
Jul 18, 2024
Currently, initially maximized (or fullscreen) xdg-shell views exhibit one of two issues: - some (e.g. GTK and Qt apps) paint an initial frame un-maximized (before the "map" event) and only maximize in a later commit - others (e.g. foot) maximize immediately without flicker, but never store a valid natural size, so we end up using a fallback (640x480) Under KWin, neither of these issues occur, so I looked into what labwc is doing wrong. It seems that: - wlroots internally sends an initial configure event with a size of 0x0 to all xdg-shell views. This requests the client to set its own preferred (a.k.a. natural) size. - For an initially maximized/fullscreen view, the initial configure event should contain the maximized/fullscreen size rather than 0x0. In labwc, this means we have to call wlr_xdg_toplevel_set_size() earlier, i.e. from the new_surface event. Tracing with WAYLAND_DEBUG shows that the initial configure event now has the correct geometry, matching KWin behavior. With this change, GTK and Qt apps no longer paint an incorrect un-maximized frame. - However, this means that all xdg-shell views now suffer from the same issue as foot, where we never receive a commit with the un-maximized (natural) geometry. The correct way to get the natural geometry seems to be to wait until we want to un-maximize, and send a configure event of 0x0 at that point. Sending a configure event of 0x0 when un-maximizing is a bit annoying as it breaks some assumptions in labwc code. In particular: - view->natural_geometry may now be unknown (0x0), requiring various wlr_box_empty() checks sprinkled around. I added these in all the obvious places, but there could be some code paths that I missed. - Positioning the newly un-maximized view within view_maximize() no longer works since we don't know the natural size. Instead we have to run the positioning logic from the surface commit handler. This results in some extra complexity, especially for interactive move. See the new do_late_positioning() function in xdg.c. Some TODOs/FIXMEs (non-blocking in my opinion): - The view_wants_decorations() check is now duplicated in both the new_surface and map event handlers. I'm not sure if this is necessary but it seemed like the safest approach for now. More testing would be nice, particularly with various combinations of config and client SSD preferences. - Aside from the interactive move case, the "late positioning" logic always centers the view when un-maximizing, and does not invoke any of the smart placement logic. If we want to invoke smart placement here, I'd appreciate someone with more knowledge of that code to take a look and figure out how to do that correctly. labwc#1956
Excellent. Thanks. |
jlindgren90
added a commit
to jlindgren90/labwc
that referenced
this pull request
Jul 19, 2024
The initial configure event is now sent explicitly by labwc rather than by wlroots. We need to move the maximize/fullscreen logic to the initial commit handling accordingly. Updates labwc#1956, fixes labwc#1994, replaces labwc#1995.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, initially maximized (or fullscreen) xdg-shell views exhibit one of two issues:
Under KWin, neither of these issues occur, so I looked into what labwc is doing wrong. It seems that:
wlroots internally sends an initial configure event with a size of 0x0 to all xdg-shell views. This requests the client to set its own preferred (a.k.a. natural) size.
For an initially maximized/fullscreen view, the initial configure event should contain the maximized/fullscreen size rather than 0x0. In labwc, this means we have to call wlr_xdg_toplevel_set_size() earlier, i.e. from the new_surface event. Tracing with WAYLAND_DEBUG shows that the initial configure event now has the correct geometry, matching KWin behavior. With this change, GTK and Qt apps no longer paint an incorrect un-maximized frame.
However, this means that all xdg-shell views now suffer from the same issue as foot, where we never receive a commit with the un-maximized (natural) geometry. The correct way to get the natural geometry seems to be to wait until we want to un-maximize, and send a configure event of 0x0 at that point.
Sending a configure event of 0x0 when un-maximizing is a bit annoying as it breaks some assumptions in labwc code. In particular:
view->natural_geometry may now be unknown (0x0), requiring various wlr_box_empty() checks sprinkled around. I added these in all the obvious places, but there could be some code paths that I missed.
Positioning the newly un-maximized view within view_maximize() no longer works since we don't know the natural size. Instead we have to run the positioning logic from the surface commit handler. This results in some extra complexity, especially for interactive move. See the new do_late_positioning() function in xdg.c.
Some TODOs/FIXMEs (non-blocking in my opinion):
The view_wants_decorations() check is now duplicated in both the new_surface and map event handlers. I'm not sure if this is necessary but it seemed like the safest approach for now. More testing would be nice, particularly with various combinations of config and client SSD preferences.
Aside from the interactive move case, the "late positioning" logic always centers the view when un-maximizing, and does not invoke any of the smart placement logic. If we want to invoke smart placement here, I'd appreciate someone with more knowledge of that code to take a look and figure out how to do that correctly.