xdg-shell, layer-shell, session-lock: Track last acked configure in MultiCache #1817

YaLTeR · 2025-09-05T13:58:37Z

Corresponding niri update: YaLTeR/niri@5a4d794

Fixes my fullscreen anim bugs that I found with mpv and adwaita-1-demo.

The last-acked configure is part of the committed state, and should be tracked as such. Before this commit, it would be possible to observe a newer "current state" in a commit handler if the surface acked the next state while the compositor was waiting on a blocker set in a pre-commit handler.

As part of the change, the current_state() getter was removed, because it's confusing and likely not what you actually want. For instance, several uses in Anvil actually needed to access a different state.

The WmCapabilities checks in Anvil were removed because they are serverside and don't change; not sure what the intention was with those.

Also, added forced configures in unmaximize() and unfullscreen() because they too are required by the protocol.

If this is good I'll do a similar refactor for layer-shell and session-lock.

One kinda weird part is current_server_state() that I had to change to owned return, tbh I would change it to return an Option<&State>, but that would involve changing a bunch of callers. I can do that though if that's better.

Also, there's a question of whether any other state needs to be moved into Cached?

YaLTeR · 2025-09-05T14:27:17Z

Another question: should this last_acked cached state be reset somewhere upon unmapping? I guess a pre-commit hook would be needed here? (The configured bool in the role should also be reset at that point. Is that bool needed, or is ToplevelCachedState::last_acked.is_some() the more correct version of it?)

Drakulix

The WmCapabilities checks in Anvil were removed because they are serverside and don't change; not sure what the intention was with those.

Also, added forced configures in unmaximize() and unfullscreen() because they too are required by the protocol.

If that is not too cumbersome, I'd love to split off fixes like this into separate commits.

One kinda weird part is current_server_state() that I had to change to owned return, tbh I would change it to return an Option<&State>, but that would involve changing a bunch of callers. I can do that though if that's better.

It seems like we already cloned it after requesting it in a bunch of places anyway? Seems fine to me honestly.

Another question: should this last_acked cached state be reset somewhere upon unmapping? I guess a pre-commit hook would be needed here?

We had this logic introduced not too (in the grand scheme of things) recently. I guess that logic simply needs to be extended a bit? #1571

anvil/src/shell/xdg.rs

YaLTeR · 2025-09-05T17:00:25Z

If that is not too cumbersome, I'd love to split off fixes like this into separate commits.

Done.

YaLTeR · 2025-09-06T08:24:29Z

Pushed a commit that also tracks the configured state through MultiCache last_acked.

Also:

Removes dependency on renderer abstractions for tracking unmaps.
Removes manual reset_initial_configure() because it's now always automatic.
Makes the check for map before initial configure automatic: fixes Map before initial configure does not disconnect the client #1717.
Adds missing role attributes reset on unmap (it only happened on role destroy previously).

Currently testing it on my machine with niri, so far so good.

YaLTeR · 2025-09-06T08:26:46Z

I removed popup_handle because it wasn't actually set. It would be good to have popup_handle (and a corresponding toplevel_handle in the toplevel) for the error emitting path, but I'm not sure off the bat if setting them would cause a refcycle, so I opted for a loop over known toplevels for now.

YaLTeR · 2025-09-06T08:28:28Z

Would be good to also move popup parent into MultiCache and get rid of committed (and along the way fix #1761) but that sounds like a much deeper dive than I'm prepared to do at the moment :p

YaLTeR · 2025-09-06T08:46:08Z

Uhm so apparently wlcs doesn't bother to ack the initial configure?

$ WAYLAND_DEBUG=client ./wlcs/wlcs ./target/debug/libwlcs_anvil.so --gtest_filter="FrameSubmission.post_one_frame_at_a_time"
(...)
[4128002.287] {Default Queue}  -> wl_compositor#4.create_surface(new id wl_surface#11)
[4128002.325] {Default Queue}  -> xdg_wm_base#7.get_xdg_surface(new id xdg_surface#9, wl_surface#11)
[4128002.352] {Default Queue}  -> xdg_surface#9.get_toplevel(new id xdg_toplevel#13)
[4128002.360] {Default Queue}  -> wl_surface#11.commit()
[4128002.378] {Default Queue}  -> wl_shm#6.create_pool(new id wl_shm_pool#14, fd 16, 160000)
[4128002.384] {Default Queue}  -> wl_shm_pool#14.create_buffer(new id wl_buffer#15, 0, 200, 200, 800, 0)
[4128002.388] {Default Queue}  -> wl_shm_pool#14.destroy()
[4128002.394] {Default Queue}  -> wl_surface#11.attach(wl_buffer#15, 0, 0)
[4128002.400] {Default Queue}  -> wl_surface#11.frame(new id wl_callback#16)
[4128002.405] {Default Queue}  -> wl_surface#11.commit()
error in client communication (pid 105972)
[4128002.829] {Display Queue} wl_display#1.delete_id(14)
[4128002.835] {Display Queue} wl_display#1.error(xdg_surface#9, 3, "must ack the initial configure before attaching buffer")

After creating a role-specific object and setting it up (e.g. by sending the title, app ID, size constraints, parent, etc), the client must perform an initial commit without any buffer attached. The compositor will reply with initial wl_surface state such as wl_surface.preferred_buffer_scale followed by an xdg_surface.configure event. The client must acknowledge it and is then allowed to attach a buffer to map the surface.

(and the server debug for completeness)

[4251864.961] wl_compositor#4.create_surface(new id wl_surface#11)
[4251864.986] xdg_wm_base#7.get_xdg_surface(new id xdg_surface#9, wl_surface#11)
[4251865.020] xdg_surface#9.get_toplevel(new id xdg_toplevel#13)
[4251865.100] wl_surface#11.commit()
[4251865.187]  -> xdg_toplevel#13.configure_bounds(800, 600)
[4251865.201]  -> xdg_toplevel#13.wm_capabilities(array[16])
[4251865.211]  -> xdg_toplevel#13.configure(0, 0, array[4])
[4251865.220]  -> xdg_surface#9.configure(1)
[4251865.229] wl_shm#6.create_pool(new id wl_shm_pool#14, fd 15, 160000)
[4251865.251] wl_shm_pool#14.create_buffer(new id wl_buffer#15, 0, 200, 200, 800, 0)
[4251865.265] wl_shm_pool#14.destroy()
[4251865.272]  -> wl_display#1.delete_id(14)
[4251865.276] wl_surface#11.attach(wl_buffer#15, 0, 0)
[4251865.284] wl_surface#11.frame(new id wl_callback#16)
[4251865.291] wl_surface#11.commit()
[4251865.302]  -> wl_display#1.error(xdg_surface#9, 3, "must ack the initial configure before attaching buffer")

YaLTeR · 2025-09-07T07:22:28Z

Did the same refactor in layer-shell and session-lock.
Exported more of the usual types from session-lock.
Fixed an edge case (introduced in the refactor) with clients attaching a null buffer for the initial configure—Waybar does this.
Added with_cached_state() to all three (layer-shell had a similar function so might as well).

Tested on:

xdg: everything running on my desktop
layer-shell: waybar, sfwbar, lxqt-panel, xfce4-panel, quickshell, xfdesktop, pcmanfm-qt --desktop
session-lock: swaylock, hyprlock, gtklock

Would be good to also move popup parent into MultiCache and get rid of committed

Added a FIXME comment mentioning this.

YaLTeR · 2025-09-07T07:39:01Z

I think newer WLCS fixes the ack_configure issue? At least from a quick glance at its repo. How is it usually updated?

codecov-commenter · 2025-09-07T07:40:38Z

Codecov Report

❌ Patch coverage is 0% with 423 lines in your changes missing coverage. Please review.
✅ Project coverage is 6.81%. Comparing base (ec35248) to head (4bd0c6f).
⚠️ Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
src/wayland/shell/xdg/mod.rs	0.00%	143 Missing ⚠️
src/wayland/session_lock/surface.rs	0.00%	106 Missing ⚠️
anvil/src/shell/xdg.rs	0.00%	69 Missing ⚠️
src/wayland/shell/wlr_layer/mod.rs	0.00%	66 Missing ⚠️
src/wayland/shell/xdg/handlers/surface.rs	0.00%	9 Missing ⚠️
src/desktop/wayland/layer.rs	0.00%	7 Missing ⚠️
src/desktop/wayland/popup/manager.rs	0.00%	6 Missing ⚠️
anvil/src/input_handler.rs	0.00%	4 Missing ⚠️
src/wayland/shell/wlr_layer/handlers.rs	0.00%	4 Missing ⚠️
src/wayland/shell/xdg/handlers/surface/popup.rs	0.00%	3 Missing ⚠️
... and 3 more

❗ There is a different number of reports uploaded between BASE (ec35248) and HEAD (4bd0c6f). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (ec35248) HEAD (4bd0c6f)

wlcs-buffer 1 0

wlcs-core 1 0

wlcs-pointer-input 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #1817       +/-   ##
==========================================
- Coverage   18.89%   6.81%   -12.09%     
==========================================
  Files         176     177        +1     
  Lines       27672   27839      +167     
==========================================
- Hits         5230    1896     -3334     
- Misses      22442   25943     +3501

Flag	Coverage Δ
wlcs-buffer	`?`
wlcs-core	`?`
wlcs-output	`6.81% <0.00%> (-0.04%)`	⬇️
wlcs-pointer-input	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Drakulix

Looks all good to me, thanks a lot for working on this.

However I would like another review before merging this. @ids1024 @cmeissl one of you maybe up for the task?

Mandated by the protocol, same as fullscreen/maximize.

Unclear what they were for. This is compositor-side state that Anvil doesn't change after startup.

The last-acked configure is part of the committed state, and should be tracked as such. Before this commit, it would be possible to observe a newer "current state" in a commit handler if the surface acked the next state while the compositor was waiting on a blocker set in a pre-commit handler. As part of the change, the current_state() getter was replaced by with_committed_state(). Since the "current" state wasn't really a correct thing most of the time, most existing uses of current_state() actually wanted something different, and were corrected in previous commits.

Also: - Removes dependency on renderer abstractions for tracking unmaps. - Removes manual reset_initial_configure() because it's now always automatic. - Makes the check for map before initial configure automatic. - Adds missing role attributes reset on unmap (it only happened on role destroy previously).

Same idea as with xdg-shell in the previous commit.

In theory, allows reusing the wl_surface, consistent with other shells.

Same idea as with xdg-shell in the previous commit.

Contains fixes required for enforcing ack before commit as mandated by the protocols.

YaLTeR · 2025-09-12T14:30:34Z

Rebased, wrote changelog.

cmeissl

Looks good to me, thanks for working on this!

Qt layer-shell calls grab() first, only then sets popup parent via zwlr_layer_surface_v1::get_popup(). Before this commit, grab() would call the handler immediately, which would see that the popup has no parent, and fail. However, it works on other compositors. The xdg_popup docs don't mention the desired behavior explicitly, though this line could be interpreted in favor of "applying" the grab request at commit time: > The client must call wl_surface.commit on the corresponding wl_surface for > the xdg_popup state to take effect. Fixes grabs from first-level popups on layer-shell Qt clients such as pcmanfm-qt --desktop and lxqt-panel.

YaLTeR · 2025-09-15T11:32:45Z

Added one commit on top that resolves the popup FIXME and delays the grab() handler until pre-commit. This fixes #1761.

I kept the parent and the new field in the role attributes because they can only be set during the initial commit and cannot change afterward (not sure this is currently enforced in Smithay), so I figured there's no need to put them in the cached state.

Tested with: pcmanfm-qt --desktop, lxqt-panel, Quickshell with Menu { popupType: Popup.Native, a bunch of random apps and tools with popups that did not have issues before

Drakulix reviewed Sep 5, 2025

View reviewed changes

anvil/src/shell/xdg.rs Outdated Show resolved Hide resolved

YaLTeR force-pushed the toplevel-cached-state branch from 386db48 to 688cd1b Compare September 5, 2025 16:58

YaLTeR force-pushed the toplevel-cached-state branch from 688cd1b to dec7479 Compare September 6, 2025 08:21

YaLTeR force-pushed the toplevel-cached-state branch from dec7479 to 3ee763d Compare September 7, 2025 07:11

YaLTeR changed the title ~~wayland/shell/xdg: Track last acked configure in MultiCache~~ xdg-shell, layer-shell, session-lock: Track last acked configure in MultiCache Sep 7, 2025

YaLTeR force-pushed the toplevel-cached-state branch from 3ee763d to 8b93492 Compare September 7, 2025 07:19

YaLTeR force-pushed the toplevel-cached-state branch from 8b93492 to 4bd0c6f Compare September 7, 2025 07:31

YaLTeR marked this pull request as ready for review September 7, 2025 07:38

YaLTeR mentioned this pull request Sep 7, 2025

Refactor layout fullscreen handling, add un/fullscreen animations YaLTeR/niri#2333

Merged

Drakulix approved these changes Sep 8, 2025

View reviewed changes

YaLTeR added 11 commits September 12, 2025 17:29

anvil: Send configure unconditionally in unfullscreen & unmaximize

d17d750

Mandated by the protocol, same as fullscreen/maximize.

anvil: Remove WmCapabilities checks

82d95a8

Unclear what they were for. This is compositor-side state that Anvil doesn't change after startup.

anvil: Correct checks of "current" to pending state

e47acea

wayland/shell/layer: Fix comment

023e2ed

wayland/shell/layer: Add and use with_cached_state() helper

c64f5dd

wayland/shell/layer: Move last_acked tracking to cached state

5a89fb5

Same idea as with xdg-shell in the previous commit.

wayland/session_lock: Reset attributes on destroy

e6d3633

In theory, allows reusing the wl_surface, consistent with other shells.

wayland/session_lock: Move last_acked tracking to cached state

215726f

Same idea as with xdg-shell in the previous commit.

Add /wlcs to .gitignore

44f881a

YaLTeR added 2 commits September 12, 2025 17:29

Bump WLCS to v1.8.1

3b51612

Contains fixes required for enforcing ack before commit as mandated by the protocols.

Add changelog entries for the protocol changes

1efb695

YaLTeR force-pushed the toplevel-cached-state branch from 3281377 to 1efb695 Compare September 12, 2025 14:30

Drakulix requested review from ids1024 and cmeissl September 12, 2025 16:25

cmeissl approved these changes Sep 14, 2025

View reviewed changes

Drakulix approved these changes Sep 17, 2025

View reviewed changes

Drakulix merged commit 91c1d36 into Smithay:master Sep 17, 2025
13 checks passed

This was referenced Sep 17, 2025

xdg_popup.grab() and parent vs. commit ordering #1761

Closed

Reset 'initial_configure_sent' upon unmapping a layer surface #1735

Closed

Map before initial configure does not disconnect the client #1717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

xdg-shell, layer-shell, session-lock: Track last acked configure in MultiCache #1817

xdg-shell, layer-shell, session-lock: Track last acked configure in MultiCache #1817

Uh oh!

YaLTeR commented Sep 5, 2025 •

edited

Loading

Uh oh!

YaLTeR commented Sep 5, 2025

Uh oh!

Drakulix left a comment

Uh oh!

Uh oh!

YaLTeR commented Sep 5, 2025

Uh oh!

YaLTeR commented Sep 6, 2025

Uh oh!

YaLTeR commented Sep 6, 2025

Uh oh!

YaLTeR commented Sep 6, 2025

Uh oh!

YaLTeR commented Sep 6, 2025 •

edited

Loading

Uh oh!

YaLTeR commented Sep 7, 2025 •

edited

Loading

Uh oh!

YaLTeR commented Sep 7, 2025

Uh oh!

codecov-commenter commented Sep 7, 2025

Uh oh!

Drakulix left a comment •

edited

Loading

Uh oh!

YaLTeR commented Sep 12, 2025

Uh oh!

cmeissl left a comment

Uh oh!

YaLTeR commented Sep 15, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

xdg-shell, layer-shell, session-lock: Track last acked configure in MultiCache #1817

xdg-shell, layer-shell, session-lock: Track last acked configure in MultiCache #1817

Uh oh!

Conversation

YaLTeR commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YaLTeR commented Sep 5, 2025

Uh oh!

Drakulix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YaLTeR commented Sep 5, 2025

Uh oh!

YaLTeR commented Sep 6, 2025

Uh oh!

YaLTeR commented Sep 6, 2025

Uh oh!

YaLTeR commented Sep 6, 2025

Uh oh!

YaLTeR commented Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YaLTeR commented Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YaLTeR commented Sep 7, 2025

Uh oh!

codecov-commenter commented Sep 7, 2025

Codecov Report

Uh oh!

Drakulix left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YaLTeR commented Sep 12, 2025

Uh oh!

cmeissl left a comment

Choose a reason for hiding this comment

Uh oh!

YaLTeR commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YaLTeR commented Sep 5, 2025 •

edited

Loading

YaLTeR commented Sep 6, 2025 •

edited

Loading

YaLTeR commented Sep 7, 2025 •

edited

Loading

Drakulix left a comment •

edited

Loading

YaLTeR commented Sep 15, 2025 •

edited

Loading