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

Pointer constraints protocol #872

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Jan 24, 2023

The protocol implementation should generally work, though there's some more work still needed to get the implementation in Anvil fully functional. And testing, docs, etc.

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

Looks good for a first draft!

@ids1024 ids1024 force-pushed the pointer-constrant branch from 9616c04 to d99e3ef Compare June 1, 2023 03:37
@ids1024 ids1024 force-pushed the pointer-constrant branch from d99e3ef to a5efd34 Compare July 7, 2023 02:39
@ids1024 ids1024 force-pushed the pointer-constrant branch 2 times, most recently from f6c0263 to b7c081b Compare July 22, 2023 01:03
@ids1024 ids1024 force-pushed the pointer-constrant branch from eb9f8fc to 265a296 Compare August 9, 2023 20:45
@ids1024 ids1024 force-pushed the pointer-constrant branch 3 times, most recently from 54e03ea to 4504e1e Compare September 14, 2023 02:45
@ids1024 ids1024 changed the title WIP Pointer constraints Pointer constraints protocol Sep 14, 2023
@ids1024
Copy link
Member Author

ids1024 commented Sep 14, 2023

I've implemented pointer confinement in Anvil; confining to just the window boundary, ignoring the region.

Handling this correctly seems complicated. The logic in wlroots is slightly complicated (https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/master/util/region.c#L190-256), but more significantly seems to depend on the pixman_region32_t data structure. I don't see any crates providing something similar. (I guess other compositors are all in C and using pixman, so it's not an issue for them.)

I think this is good enough to merge now, at least given the part that's incomplete is the Anvil implementation.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023

Codecov Report

Patch coverage: 14.77% and project coverage change: -0.13% ⚠️

Comparison is base (d8600f2) 22.88% compared to head (d978a2b) 22.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
- Coverage   22.88%   22.76%   -0.13%     
==========================================
  Files         143      144       +1     
  Lines       23050    23365     +315     
==========================================
+ Hits         5275     5319      +44     
- Misses      17775    18046     +271     
Flag Coverage Δ
wlcs-buffer 19.79% <4.40%> (-0.21%) ⬇️
wlcs-core 19.45% <4.40%> (-0.21%) ⬇️
wlcs-output 8.05% <4.40%> (-0.05%) ⬇️
wlcs-pointer-input 21.56% <14.77%> (-0.11%) ⬇️

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

Files Changed Coverage Δ
anvil/src/input_handler.rs 7.33% <ø> (ø)
src/input/pointer/mod.rs 33.09% <0.00%> (-0.25%) ⬇️
anvil/src/state.rs 37.62% <8.33%> (-0.60%) ⬇️
src/wayland/pointer_constraints.rs 14.09% <14.09%> (ø)
src/wayland/seat/pointer.rs 17.40% <80.00%> (+0.77%) ⬆️

... and 1 file with indirect coverage changes

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

@ids1024 ids1024 marked this pull request as ready for review September 14, 2023 03:17
ids1024 added a commit to pop-os/cosmic-comp that referenced this pull request Sep 14, 2023
Depends on Smithay/smithay#872.

For some reason, pointer confinment isn't working correctly here at the
moment, even though the code matches Anvil...

May need solution to Smithay/smithay#1126 for
behavior to be correct.
@Drakulix
Copy link
Member

Handling this correctly seems complicated. The logic in wlroots is slightly complicated (https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/master/util/region.c#L190-256), but more significantly seems to depend on the pixman_region32_t data structure. I don't see any crates providing something similar. (I guess other compositors are all in C and using pixman, so it's not an issue for them.)

I am slightly confused, because I am pretty sure we have to deal with this for input_regions of surfaces already. So I am not sure, why the same code wouldn't work here?
It probably needs logic to properly clamp to the region, but a cheap implementation could just discard cursor movements, that would result in a place outside the region. And we already have a "is point in wl_region" test for certain.

@Drakulix
Copy link
Member

@ids1024
Copy link
Member Author

ids1024 commented Sep 14, 2023

Yeah, I was thinking of the clamping part. Just ignoring movement if the new position would be outside the region might work alright. Technically less pixel-perfect. Also with a really weird region that could jump over a tiny area that isn't in the region to another that is, but that's even less likely to be an issue (I'm not sure exactly how other implementations handle these cases).

pixman_region32_t should be a more efficient data structure in general, but hopefully applications aren't adding and subtracting 500 rectangles to a wl_region...

@Drakulix
Copy link
Member

pixman_region32_t should be a more efficient data structure in general, but hopefully applications aren't adding and subtracting 500 rectangles to a wl_region...

Oh absolutely and yes the issues above can in theory happen. But I think implementing it this way in anvil is better, than not implementing it at all. Especially because compositors are likely going to copy&paste anvil's implementation. 😅

Similar to implementations for `Seat`. `PartialEq` was already
implemented.
A pointer constraint is unique per surface and pointer. Creating a
constraint when one already exists is a protocol error. Technically the
spec says it should be a protocol error to add a constraint for the same
"seat", but as things are implemented in Smithay it's easier to tie it
to the `PointerHandle`.

This provides `activated`/`deactivated` helpers, which is somewhat
similar to the API wlroots provides.

Adds implementation in anvil. This implementation doesn't currently
handle regions, which requires a non-trivial algorithm / data structure.
But that should be good enough for an initial implementation in Anvil.
@ids1024
Copy link
Member Author

ids1024 commented Sep 18, 2023

This isn't doing any fancy pointer warping, but setting a region seems to work now, with Smithay/client-toolkit#407.

Just testing if the pointer ends up outside the region and dropping the event, instead of clamping, seems to be working alright.

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

LGTM

@Drakulix Drakulix merged commit 58d5bdc into Smithay:master Sep 19, 2023
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.

3 participants