smithay
smithay copied to clipboard
Smithay
fix buffer offset handling
fixes the regression introduced in https://github.com/Smithay/smithay/pull/1123#issuecomment-1953661036 (not that is was working correctly before, but pointer was broken at bit less)
what I do not like is all the manual plumbing in the Dnd handling, handling this in smithay will require re-structuring a few things. From the spec of DnD start:
The icon surface is an optional (can be NULL) surface that provides an icon to be moved around with the cursor. Initially, the top-left corner of the icon surface is placed at the cursor hotspot, but subsequent wl_surface.attach request can move the relative position.
According to this we need the cursor hotspot at the start of the grab to initialize the DnD icon offset. But as the hotspot is stored in the current pointer surface we can no access it in smithay. On commit of the DnD icon we need to be able to change the current DnD offset by the surface offset if any. But again we can not do this internally as we have no access to the icon.
We can probably do better by either:
- moving some parts that are now in anvil/downstream into smithay
- providing some helpers
- requiring access to some state held by downstream (similar to
SeatState)
based on https://github.com/Smithay/smithay/pull/1340
@ids1024 @Drakulix @PolyMeilex would love to get some feedback on this topic, maybe we can find some better solution together.
Test cases:
- GTK4: Identity. Open a video, play it, DnD while playing, the DnD icon should be a centered video.
- wleird-attach-delta-loop, see what it does in Weston for instance.
Test cases:
1. GTK4: [Identity](https://flathub.org/apps/org.gnome.gitlab.YaLTeR.Identity). Open a video, play it, DnD while playing, the DnD icon should be a centered video. 2. [wleird-attach-delta-loop](https://gitlab.freedesktop.org/emersion/wleird/-/blob/master/attach-delta-loop.c?ref_type=heads), see what it does in Weston for instance.
Number 2 is actually a good example on why and what it is currently broken in smithay. The current implementation offsets all surfaces by the buffer offset, but weston actually modifies the position of the toplevel. Without SSD (which weston does not have) it just looks the same, but it actually does something completely different.
The offset should (and this is how it is done in weston) be handled in a role specify way. For pointer it defines that it should decrement the hotspot, dnd defines it as adding it to the current offset, toplevel will be moved, ... The hotspot for pointer is reset when the client sets the cursor image, this also explains why pointer is currently so broken in the current implementation. (Also a good explanation on why the surface will not move in sway and probably also most other tiling compositors. So not sure what you want to do in niri, but for toplevels it should be compositor policy and nothing smithay should internally handle)
I hope this helps to clarify the motivations behind these changes and why it looked like it was working, while it was actually broken.
Codecov Report
Attention: Patch coverage is 32.18391% with 59 lines in your changes missing coverage. Please review.
Project coverage is 20.55%. Comparing base (
5386601) to head (4ad5199).
| Files | Patch % | Lines |
|---|---|---|
| src/wayland/seat/pointer.rs | 0.00% | 23 Missing :warning: |
| anvil/src/shell/mod.rs | 46.66% | 16 Missing :warning: |
| anvil/src/state.rs | 0.00% | 14 Missing :warning: |
| wlcs_anvil/src/main_loop.rs | 66.66% | 6 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## fix/pointer_image_stuck #1341 +/- ##
========================================================
Coverage 20.55% 20.55%
========================================================
Files 158 158
Lines 25868 25934 +66
========================================================
+ Hits 5317 5332 +15
- Misses 20551 20602 +51
| Flag | Coverage Δ | |
|---|---|---|
| wlcs-buffer | 17.98% <32.18%> (+0.01%) |
:arrow_up: |
| wlcs-core | 17.64% <32.18%> (+0.01%) |
:arrow_up: |
| wlcs-output | 7.54% <13.79%> (+0.01%) |
:arrow_up: |
| wlcs-pointer-input | 19.44% <32.18%> (+<0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Okay, I moved the whole buffer offset handling into anvil. I do not like to have more Mutex stuff for this in smithay itself and only handling the cursor hotspot in smithay is inconsistent.