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

swaybar: Fix scrolling to switch workspace with precise trackpads #5067

Merged

Conversation

mortie
Copy link
Contributor

@mortie mortie commented Mar 3, 2020

This patch implements some logic to try to emulate scrolling in discrete steps for devices which scroll continuously, such as modern touchpads. It keeps track of whether the axis event is discrete or not, so it shouldn't have any effect on switching workspaces with scroll wheels (assuming an axis_discrete event is always fired before an axis event for devices which scroll wheels, which seems to be the case).

This is related to #1930.

Note that this patch makes swaybar depend on wl_seat v5 instead of v3.

Here's exactly how the decision about whether to switch workspaces or not is done:

bool do_scroll =
	(axis == WL_POINTER_AXIS_VERTICAL_SCROLL && seat->scroll_is_discrete_vertical) ||
	(axis == WL_POINTER_AXIS_HORIZONTAL_SCROLL && seat->scroll_is_discrete_horizontal) ||
	fabs(amt) >= 10 ||
	(fabs(amt) >= 1 && time >= seat->scroll_time + 200);

Switch workspace if:

  • The event is from a device with discrete scroll steps.
  • The movement in this one event is really big. This allows for making a large motion to scroll through lots of workspaces at once, typically to get to the first or last workspace.
  • The time since the last workspace switch is at least 200ms (and the event's delta isn't tiny). This makes a slow, controlled motion first switch workspace instantly, then every 200ms.

This feels pretty good to me on my laptop with my touchpad, but I haven't tested it on others. Maybe some tuning of the values is necessary (or maybe they should even be configurable).

@mortie
Copy link
Contributor Author

mortie commented Mar 3, 2020

I see the FreeBSD CI build failed, but it failed with:

pkg: wrong architecture: FreeBSD:12.0:amd64 instead of FreeBSD:12:amd64
pkg: repository FreeBSD contains packages with wrong ABI: FreeBSD:12.0:amd64
Unable to update repository FreeBSD
Error updating repositories!

@emersion
Copy link
Member

emersion commented Mar 4, 2020

Yeah, don't worry about it, that's a FreeBSD bug. They're working on fixing it.

@ianyfan
Copy link
Contributor

ianyfan commented Mar 4, 2020

It's been a while since I've worked on this, so I may be wrong, but I'm not sure if this is very robust in the case that a seat has both a trackpad and a mousewheel attached. I think a complete solution would utilise discrete axis events separately.

@mortie
Copy link
Contributor Author

mortie commented Mar 4, 2020

We discussed that kind of thing in #sway-devel, I think we concluded that this way of implementing it should be safe. This solution works in the case where we receive an axis_discrete event for axis A, followed by an axis event for axis B, followed by the axis event for axis A; A is correctly treated as discrete, while B is treated as continuous.

The only way for it to fail is if we receive an axis_discrete for axis A, followed by an axis event for the same axis A which should've been treated as continuous, followed by the axis event for axis A which should've been treated as discrete - and I think that sequence is illegal.

I'm not 100% confident in this interpretation though, and I'm ready to be proven wrong if you disagree. Also, if you have a suggestion for a better way to figure out whether an axis event is for a discrete or continuous scroll, I'd be happy to implement it.

@Synthetica9
Copy link
Contributor

If I understand correctly, this would only fire scroll events every 200ms when scrolling continuously? On Waybar, we keep an internal counter to see how far we've scrolled yet, which seems to work very well. (See Alexays/Waybar#381)

@mortie
Copy link
Contributor Author

mortie commented Mar 5, 2020

Yeah, this would trigger a workspace switch every 200ms when scrolling continuously (with a speed of >=1 per axis event). My initial implementation had a counter like you're proposing, but I eventually tweaked it to work like "if abs(counter) >= k or the previous scroll time was more than N milliseconds ago". That second case is really important, because you want to switch workspaces instantly as soon as the user starts scrolling for a snappy user experience.

I kept tuning the various thresholds until I thought they felt good, and after that, I noticed that the "abs(counter) >= k" case was essentially tuned out of existence, so I removed it.

In retrospect, I think I can see these three distinct modes of use of a scrolling workspace switcher:

  1. Scrolling once to switch workspaces one time.
  2. Scroll continuously in a controlled manner to switch workspaces a few times.
  3. Scroll quickly and uncontrolled, to reach the first or last workspace.

Looking at it like that, it seems to make more sense that "switch workspaces every 200ms" in the second mode instead of "switch workspaces every distance X scrolled' might make sense.

Again though, I have only tested this on my own laptop, with my own hands an my own sensibilities. User studies would be necessary to figure out what's actually the best solution in any objective sense. If anyone tries my patch and finds it doesn't work well for them, I'd be very interested to hear about it.

@mortie
Copy link
Contributor Author

mortie commented Mar 16, 2020

What's the status on this? Would people prefer if I removed the timing component and re-introduced the counter, doing something like waybar? Or is this way fine and people are just busy with other things?

Either is ok, but I think I can fairly easily change my patch to use a counter in a way which still feels good. I just need to know if it's worthwhile or not.

swaybar/input.c Outdated Show resolved Hide resolved
@ddevault
Copy link
Member

This code should be reworked to use the frame event instead. We should be accumulating pointer events into a single struct, then defer processing it until we get a frame event.

@mortie mortie force-pushed the fix-swaybar-scroll-with-precise-trackpads branch from bdfa665 to 70e9178 Compare April 23, 2020 21:09
@mortie
Copy link
Contributor Author

mortie commented Apr 23, 2020

Alright, I reworked my patches to accumulate events and then act on them in the 'frame' event. I'm also respecting the 'discrete'' value in 'axis_discrete', and removed my weird timing behaviour in favour of a plain "accumulate scroll distance, act when it crosses some threshold" system.

@mortie mortie force-pushed the fix-swaybar-scroll-with-precise-trackpads branch from 70e9178 to d8426b4 Compare April 23, 2020 21:17
@ddevault
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants