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

fix buffer offset handling #1341

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Conversation

cmeissl
Copy link
Collaborator

@cmeissl cmeissl commented Feb 24, 2024

fixes the regression introduced in #1123 (comment)
(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 #1340

@cmeissl
Copy link
Collaborator Author

cmeissl commented Feb 24, 2024

@YaLTeR would be nice to get some feedback from you

@cmeissl
Copy link
Collaborator Author

cmeissl commented Feb 24, 2024

@ids1024 @Drakulix @PolyMeilex would love to get some feedback on this topic, maybe we can find some better solution together.

@YaLTeR
Copy link
Contributor

YaLTeR commented Feb 24, 2024

Test cases:

  1. GTK4: Identity. Open a video, play it, DnD while playing, the DnD icon should be a centered video.
  2. wleird-attach-delta-loop, see what it does in Weston for instance.

@cmeissl
Copy link
Collaborator Author

cmeissl commented Feb 24, 2024

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-commenter
Copy link

codecov-commenter commented Feb 24, 2024

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 ⚠️
anvil/src/shell/mod.rs 46.66% 16 Missing ⚠️
anvil/src/state.rs 0.00% 14 Missing ⚠️
wlcs_anvil/src/main_loop.rs 66.66% 6 Missing ⚠️
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%) ⬆️
wlcs-core 17.64% <32.18%> (+0.01%) ⬆️
wlcs-output 7.54% <13.79%> (+0.01%) ⬆️
wlcs-pointer-input 19.44% <32.18%> (+<0.01%) ⬆️

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.

the buffer offset should be handled in a role specific way.
adding the offset to the surface view to offset the surface
is wrong.
@cmeissl
Copy link
Collaborator Author

cmeissl commented Sep 14, 2024

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.

@cmeissl cmeissl marked this pull request as ready for review September 14, 2024 11:42
@@ -152,6 +152,22 @@ impl<BackendData: Backend> CompositorHandler for AnvilState<BackendData> {
}
if let Some(window) = self.window_for_surface(&root) {
window.0.on_commit();

if &root == surface {
Copy link
Contributor

Choose a reason for hiding this comment

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

For subsurfaces it is supposed to do nothing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not so easy to answer, it might have been intended to also apply to subsurfaces. But as mutter and wlroots always ignored the offset for subsurface weston has been changed to also ignore it. So imo we should follow that and just accept this as the common behavior.

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.

Yes, this seems like a very sane approach, given how role-specific this is.

@Drakulix Drakulix merged commit b29ff8f into Smithay:master Sep 18, 2024
13 checks passed
@cmeissl cmeissl deleted the fix/buffer_offset branch September 19, 2024 16:12
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.

4 participants