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

High res scrolling support #1246

Merged
merged 1 commit into from
Dec 12, 2023
Merged

High res scrolling support #1246

merged 1 commit into from
Dec 12, 2023

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Dec 4, 2023

I've been thinking about how to handle high res scroll. Might as well hack together an initial implementation. Very WIP, but shows what is still needed here.

Testing things, it seems PointerScrollWheelEvent is emitted whenever a scroll is sent from the device, while PointerAxisEvent is only sent after accumulating change. So this impacts even clients not using the new protocol version, so they can get more precise scrolling from wl_pointer::axis.

In particular, this works in Firefox.

Issues:

  • For clients not using the new protocol version, we need to accumulate axis_value120 events into a axis_discrete event
    • Where can this be handed? Probably somewhere in Smithay and not the compositor?
      • Not obviously something libinput backend can deal with. Handle in PointerTarget<D> for WlSurface somehow?
  • Instead of one PointerAxisEvent with an AxisSource enum, there are multiple PointerScroll* event types
    • Currently this only handles PointerScrollWheelEvent
    • smithay::backend::input currently models its API here on libinput. So do we want to remove our AxisSource and split type PointerAxisEvent into 3 types?
  • Requires libinput 1.19
    • May be okay? Otherwise, have feature flag, and fallback to generating events from PointerAxisEvent
  • Needs implementation for X11/winit backend. Ideally using high-res scroll when available.

@ids1024
Copy link
Member Author

ids1024 commented Dec 4, 2023

I guess accumulation for axis_discrete could be stored in SurfaceData::data_map, and reset to 0 on axis_stop or leave.

@Drakulix
Copy link
Member

Drakulix commented Dec 5, 2023

* For clients not using the new protocol version, we need to accumulate `axis_value120` events into a `axis_discrete` event
  
  * Where can this be handed? Probably somewhere in Smithay and not the compositor?
    
    * Not obviously something libinput backend can deal with. Handle in `PointerTarget<D> for WlSurface` somehow?

I guess accumulation for axis_discrete could be stored in SurfaceData::data_map, and reset to 0 on axis_stop or leave.

Yeah, that seems like a good approach to me. I would love to handle of the backwards compatibility inside the specific pointer targets (e.g. WlSurface's implementation) and just use the more precise values all out through smithay. That way we also don't have to replicate the _120 naming.

This might be a bit annoying for toolkit integrations like smithay-egui or cosmic-comp's iced code, but they should be able to copy the WlSurface implementation, if they need to. Maybe we can make this conversion/caching into a helper function?

* Instead of one `PointerAxisEvent` with an `AxisSource` enum, there are multiple `PointerScroll*` event types
  
  * Currently this only handles `PointerScrollWheelEvent`
  * `smithay::backend::input` currently models its API here on libinput. So do we want to remove our `AxisSource` and split `type PointerAxisEvent` into 3 types?

Yes probably.

* Requires libinput 1.19

Totally fine imo, even debian stable already has libinput 1.22.

* Needs implementation for X11/winit backend. Ideally using high-res scroll when available.

Given not all compositors support this (and I am not sure if Xwayland does and will ever), we need a way to generate the new events from the old values. I guess that shouldn't be too difficult though, no?

@ids1024
Copy link
Member Author

ids1024 commented Dec 5, 2023

Given not all compositors support this (and I am not sure if Xwayland does and will ever), we need a way to generate the new events from the old values. I guess that shouldn't be too difficult though, no?

Conversion in that direction is much easier, I think. Nothing needs to be accumulated, and the discrete values can just be multiplied by 120 to get the "v120" value.

@ids1024 ids1024 force-pushed the scroll-v120 branch 4 times, most recently from cb4fb28 to a2fc9c6 Compare December 11, 2023 21:08
This adds support for `wl_pointer` version 8, calling
`wl_pointer::axis_value120` with the v120 values from libinput. For
clients not binding version 8, values are accumulated to multiples of
120 to send the old `axis_discrete` event.

This helps even for clients using `wl_pointer::axis` with old versions
of `wl_pointer`, since the old libinput event only was emitted where
there was a whole discrete scroll step.

Raises libinput requirement to 1.19, which should be widely available
now. Removes handling of versions before that.

It should be possible to support high res scrolling in the X11 and Winit
backends, where the platform supports it, but for now this just converts
the low-res value to v120.
@ids1024 ids1024 changed the title WIP high res scrolling High res scrolling support Dec 11, 2023
@ids1024 ids1024 marked this pull request as ready for review December 11, 2023 22:25
@ids1024
Copy link
Member Author

ids1024 commented Dec 11, 2023

This should be working well now.

Instead of adding 2 more associated types, I used an enum to wrap the different types of scroll events in the libinput backend. Otherwise it would complicate the input handling in the compositor unnecessarily.

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.

I am not super sure, we want the renaming from discrete->v120 now. Is this intended to introduce breakage on purpose?

I don't find the name particular descriptive and the 120 values are still discrete, so if we want to rename, we might as well do amount_discrete_v120 at least, no?

@ids1024
Copy link
Member Author

ids1024 commented Dec 12, 2023

Is this intended to introduce breakage on purpose?

Leaving the name the same could/will break behavior. At a minimum the code in Anvil/Smallvil/Cosmic-Comp tries to use amount_discrete * 3 if amount isn't defined. Which definitely isn't right after this without multiplying that by 120, though I'm not sure that fallback is generally needed. (Are there circumstances where amount_v120 is defined, by amount isn't?)

Changing the name seems appropriate if users are going to have to change the code using it anyway, to have correct behavior after this change.

As for the specific naming, Wayland named the event axis_value120. https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/72 originally called it axis_v120, but changed it after some discussion about the name. Libinput uses the name scroll_value_v120. So amount_v120 seems most consistent with libinput. amount_value120 would be more consistent with Wayland, but I'm not sure that's particularly better.

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.

Sounds good. Given I got nothing on the actual code, I am going to merge this then.

@Drakulix Drakulix merged commit d5782e1 into Smithay:master Dec 12, 2023
9 of 13 checks passed
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