Change `PointerHandle.motion` and friends to take relative coords

[colinmarc]

trafficstars

The existing API is helpful in that it makes it harder to mess up the math, but it's not flexible enough to support having different windows with different scaling factors. That's relevant if, for example, you want to force a scale factor of 1 for Xwayland windows.

The existing API takes Option<(<D as SeatHandler>::PointerFocus, Point<i32, Logical>)>, with the point being the surface origin, rather than the relative coordinates of the point on that surface. Then, internally, it converts into relative coordinates to generate the wayland event. A user familiar with wayland but not reading the documentation closely might think that the point should be the relative coords, not the surface origin (this happened to me).

Additionally, if you have different scale factors for different windows, it's impossible (or very difficult) to get the calculation right, since smithay is assuming that the coordinate space for the surface is the same as the global compositor space.

[colinmarc]

I can't get anvil to run on my machine for (it appears unrelated) reasons, so I haven't tested that part.

It occurs to me that this will probably result in confusing behavior for existing compositors who update without knowing. I don't really know how to mitigate that.

[colinmarc]

Hm, this is kinda lackluster. Taking another stab at this with a newtype to flush out cases I missed.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 34.17722% with 104 lines in your changes missing coverage. Please review.

Project coverage is 21.76%. Comparing base (003ca51) to head (e3a9927). Report is 325 commits behind head on master.

Files	Patch %	Lines
src/input/pointer/mod.rs	36.53%	33 Missing :warning:
src/wayland/tablet_manager/tablet_tool.rs	0.00%	16 Missing :warning:
anvil/src/input_handler.rs	34.78%	15 Missing :warning:
src/desktop/wayland/popup/grab.rs	0.00%	8 Missing :warning:
anvil/src/shell/element.rs	0.00%	6 Missing :warning:
anvil/src/shell/grabs.rs	0.00%	6 Missing :warning:
src/wayland/selection/data_device/dnd_grab.rs	0.00%	6 Missing :warning:
...c/wayland/selection/data_device/server_dnd_grab.rs	0.00%	6 Missing :warning:
anvil/src/shell/xdg.rs	0.00%	2 Missing :warning:
src/desktop/wayland/layer.rs	0.00%	2 Missing :warning:
... and 2 more

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1223    +/-   ##
========================================
  Coverage   21.76%   21.76%            
========================================
  Files         155      156     +1     
  Lines       24373    24490   +117     
========================================
+ Hits         5305     5331    +26     
- Misses      19068    19159    +91     

Flag	Coverage Δ
wlcs-buffer	18.74% <0.00%> (-0.05%)	:arrow_down:
wlcs-core	18.40% <0.00%> (-0.05%)	:arrow_down:
wlcs-output	7.64% <0.00%> (+<0.01%)	:arrow_up:
wlcs-pointer-input	20.60% <34.17%> (+0.06%)	: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.

[colinmarc]

Ok, I think this is much better now. I introduced a type, RelativePoint, to add semantic meaning to Option<(<D as SeatHandler>::PointerFocus, Point<i32, Logical>)>, which was a bit of a mouthful.

I find the new API is a bit more intuitive (I got confused that motion wanted the surface origin), but I know it's opinionated, so please tell me if you'd prefer a different approach. One disadvantage is that Window.surface_under still returns Option((surface, surface_origin)), but I added RelativePoint::on_focused_surface(pos, surface, surface_origin) to make it easy to follow the happy path without doing any math yourself.

This has the extra benefit that the calculation pos - surface_origin.to_f64() is only done there, instead of all over the library.

[Drakulix]

That's relevant if, for example, you want to force a scale factor of 1 for Xwayland windows.

Before giving this a proper review, note that I would love to have proper API in smithay to support overriding scale factors of windows and/or globally for Xwayland.

[colinmarc]

Sorry for the delay! Updated to address comments.

[colinmarc]

Renaming MotionEvent.location made me realize that this line may no longer be correct:

https://github.com/Smithay/smithay/blob/859ed44a34ea4fbb66349b3ae55aa7c67bc3d443/src/desktop/wayland/layer.rs#L729

But I'm not sure.

[Drakulix]

Renaming MotionEvent.location made me realize that this line may no longer be correct:

https://github.com/Smithay/smithay/blob/859ed44a34ea4fbb66349b3ae55aa7c67bc3d443/src/desktop/wayland/layer.rs#L729

But I'm not sure.

It is correct, the issue is that PointerTarget::enter/motion both get a MotionEvent with relative coordinates, while the PointerHandle and grabs don't. But both take the same MotionEvent-type, which means the last commit made the situation both better and worse, depending on which part of the API you look at.

And I think this the core issue, that makes this api feel so weird. We probably don't want to use the same type here, as this is confusing at best. This fits quite nicely with a discussion we recently had of introducing Local or Relative coordinates as opposed to Global coordinates in addition to the Logical/Physical coordinate spaces. I'll get back to you on that one.

[colinmarc]

I took a stab at making MotionEvent generic over relative/global coordinate spaces:

#[derive(Debug)]
pub struct GlobalOrigin;

#[derive(Debug)]
pub struct SurfaceOrigin;

/// Pointer motion event
#[derive(Debug)]
pub struct MotionEvent<Origin> {
    /// Location of the pointer. If `Origin` is `SurfaceOrigin`, the coordinates
    /// are relative to the origin of the surface. If `Origin` is `GlobalOrigin`, it is a
    /// location in global compositor space.
    pub location: Point<f64, Logical>,
    /// Serial of the event
    pub serial: Serial,
    /// Timestamp of the event, with millisecond granularity
    pub time: u32,

    _origin: std::marker::PhantomData<Origin>,
}

The problem is still that lots of functions take an optional focus, but the MotionEvent is not optional:

    /// Notify that the pointer moved
    ///
    /// You provide the new location of the pointer, in the form of:
    ///
    /// - The surface that the cursor is on top of, and
    /// - The coordinates of the cursor relative to the origin of that surface
    ///
    /// A value of None indicates that no surface is focused, and the
    /// coordinates of the MotionEvent will be ignored.
    ///
    /// This will internally take care of notifying the appropriate client
    /// objects of enter/motion/leave events.
    #[instrument(level = "trace", parent = &self.span, skip(self, data, focus))]
    pub fn motion(&self, data: &mut D, focus: Option<D::PointerFocus>, event: &MotionEvent<SurfaceOrigin>) {
        // ...
    }

Here, if focus is None, then the location field of the event is completely ignored. It's the same problem, but subtly different, and even more finicky, because you need to construct a MotionEvent in the backend code of your compositor (anvil example), and it's not clear what should be set for location if it's a relative one and there's no focus.

I see two possibilities:

1. Ditching MotionEvent altogether, adding serial etc as arguments in their own right, and keeping RelativePoint (from the current version of this PR)
2. Removing location from MotionEvent but otherwise keeping this PR as-is.

I think I like 2 the best. MotionEvent keeps its semantic meaning with a timestamp and serial, which is to say it's semi-opaque and gets passed through everywhere. But the RelativePoint gets more relative as we go down the tree.

[Drakulix]

I see two possibilities:

1. Ditching `MotionEvent` altogether, adding `serial` etc as arguments in their own right, and keeping `RelativePoint` (from the [current version](https://github.com/Smithay/smithay/pull/1223/commits/9b5411ac287e595b18444ffcfa574700a5d5bed6#diff-4d2f1d428c6ceedf69db74c16fda79aa7ce3e8317c08456336a67f67b7ebcd13) of this PR)

2. Removing `location` from `MotionEvent` but otherwise keeping this PR as-is.

I think I like 2 the best. MotionEvent keeps its semantic meaning with a timestamp and serial, which is to say it's semi-opaque and gets passed through everywhere. But the RelativePoint gets more relative as we go down the tree.

I agree, that 2. sounds like a good solution.

[ryanabx]

Removing location from MotionEvent but otherwise keeping this PR as-is.

I think I like 2 the best. MotionEvent keeps its semantic meaning with a timestamp and serial, which is to say it's semi-opaque and gets passed through everywhere. But the RelativePoint gets more relative as we go down the tree.

Naively trying to understand this -- If MotionEvent doesn't have location, how do we gather where the pointer moved in fn motion...

[colinmarc]

Naively trying to understand this -- If MotionEvent doesn't have location, how do we gather where the pointer moved in fn motion...

The RelativePoint has a location, but it's not in the global compositor space, it's relative to the surface. Turns out some of the APIs (grab intercepting for example) rely on getting the global cursor location out of the PointerHandle, so I got stuck.

[chrisduerr]

I don't think RelativePoint contributes much here, especially since every point is relative.

With MotionEvent things are still just as confusing anyway, since what you want is probably position relative to the touch down surface, while the focus will potentially be on a separate surface. So I'd say location should only be in the event and not coupled to the focus.