Skip to content

Add alive() method for grabs, and add PopupTouchGrab#1911

Draft
ids1024 wants to merge 10 commits intoSmithay:masterfrom
ids1024:touch-grab
Draft

Add alive() method for grabs, and add PopupTouchGrab#1911
ids1024 wants to merge 10 commits intoSmithay:masterfrom
ids1024:touch-grab

Conversation

@ids1024
Copy link
Copy Markdown
Member

@ids1024 ids1024 commented Jan 22, 2026

https://wayland.app/protocols/xdg-shell#xdg_popup:request:grab says popups that take a grab should also take touch input.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 0% with 94 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.30%. Comparing base (e84a4ca) to head (d213731).
⚠️ Report is 46 commits behind head on master.

Files with missing lines Patch % Lines
src/desktop/wayland/popup/grab.rs 0.00% 84 Missing ⚠️
anvil/src/shell/xdg.rs 0.00% 9 Missing ⚠️
src/desktop/wayland/popup/manager.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1911      +/-   ##
==========================================
- Coverage   18.73%   18.30%   -0.44%     
==========================================
  Files         182      182              
  Lines       28934    29342     +408     
==========================================
- Hits         5421     5370      -51     
- Misses      23513    23972     +459     
Flag Coverage Δ
wlcs-buffer 15.92% <0.00%> (-0.41%) ⬇️
wlcs-core 15.48% <0.00%> (-0.50%) ⬇️
wlcs-output 6.81% <0.00%> (-0.09%) ⬇️
wlcs-pointer-input 17.21% <0.00%> (-0.52%) ⬇️

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.

@ids1024
Copy link
Copy Markdown
Member Author

ids1024 commented Apr 9, 2026

I guess we need an MSRV of 1.88 to use if let chains. No matter, though this is a little neater with that.

This is called whenever the grab is used, and if it returns `false`, the
keyboard/pointer/touch will be treated as not grabbed, and the grab will
be removed automatically.

The actual removal of the grab only happens in
`*Internal`/`*Inner::with_grab`, since `unset()` requires a `data: &mut
D` argument.

This function may be called somewhat frequently, but that's probably
fine as long as the implementation is nothing too complicated. An
alternative would be to not check everywhere and instead do so in some
periodic refresh, but this seems reasonable.
@ids1024 ids1024 changed the title WIP Touch popup grab Add alive() method for grabs, and PopupTouchGrab Apr 9, 2026
@ids1024
Copy link
Copy Markdown
Member Author

ids1024 commented Apr 9, 2026

Okay, I think this is working now.

Previously, running two copies of cosmic-term in anvil or cosmic-comp, opening a menu in one then touching a menu in the second would make the second instance crash with a xdg_popup was not created on the topmost popup protocol error.

Not sure if we should change anything else related to that, but having a touch grab prevents that. Now either the grab is removed, or touch input, like other input, can't go to the other process.

I've added a new .alive() mechanism to grabs, which seems like a better way to make sure removing one of these grabs removes them all.

@ids1024
Copy link
Copy Markdown
Member Author

ids1024 commented Apr 9, 2026

Oh, and I forgot that I meant to use has_ended() for defining alive(), which should also make the separate has_ended() checks unnecessary.

This may warrant a bit more testing, but assuming no unforseen issues, it's a nice cleanup to the popup grab code.

@ids1024 ids1024 force-pushed the touch-grab branch 2 times, most recently from c931f80 to 574e7f2 Compare April 9, 2026 23:20
@ids1024
Copy link
Copy Markdown
Member Author

ids1024 commented Apr 9, 2026

I guess to change how popup grabs work, there are more details to get the behavior right.

Though on cosmic-comp (on the existing version), I see Esc dismisses both the pointer and keyboard grabs but still leaves the popup open, so some changes here could be appropriate regardless.

@ids1024 ids1024 force-pushed the touch-grab branch 2 times, most recently from f242802 to fc065c7 Compare April 10, 2026 02:09
@ids1024 ids1024 changed the title Add alive() method for grabs, and PopupTouchGrab Add alive() method for grabs, and add PopupTouchGrab Apr 10, 2026
@ids1024
Copy link
Copy Markdown
Member Author

ids1024 commented Apr 10, 2026

Well now that issue is fixed too, but closing a menu by touching elsewhere on the window is making the window not open again...

@ids1024
Copy link
Copy Markdown
Member Author

ids1024 commented Apr 10, 2026

I guess not having a refresh method for this does still mean the grabs may not have unset called for a long time if no input of a particular category occurs. (So the pointer grab isn't being unset if only touch input has happened since alive started returning false)

ids1024 added 3 commits April 10, 2026 18:21
This replaces `unset_keyboard_grab` with a single mechanism to make sure
that any of the three grabs (keyboard, pointer, touch) ending results in
all of them being removed.

It also makes it unnecesary to check `has_ended()` in various methods.
If the method is called, `alive()` has already been checked.

We could do this a different way, like using `with_grab` on each type of
input device, downcasting, and checking if it's a matching popup grab,
but this `alive()` mechanism seems cleaner.
This ensures the popup is dismissed when the grab is removed somewhere
other than the `button()` method here. For instance, pressing `esc` on
cosmic-comp.
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.

2 participants