linux: Various X11 scroll improvements #18484
Conversation
cla-bot
bot
added
the
cla-signed
mgsloan
force-pushed
the
x11-scroll-improvements
branch
5 times, most recently
from
66566cb
to
7288186
Compare
mgsloan
force-pushed
the
x11-scroll-improvements
branch
from
7288186
to
bda572f
Compare
mgsloan
force-pushed
the
x11-scroll-improvements
branch
2 times, most recently
from
6cbe2f3
to
a7ab256
Compare
|
Hey, thanks for this! I'm going to take a look, try this out, and review code. @apricotbucket28 I'd appreciate a second set of eyes, since you know this area pretty well — if you have the time :) |
mrnugget
changed the title
There was a problem hiding this comment.
Code looks good to me (left some improvement suggestions). Appreciate the tests and pulling out into functions.
I also tried it locally on my Linux/X11 machine, and it didn't break anything, but I also can't confirm/deny that it fixed anything. I trust you here.
So, let me know what you think about the suggested changes and then, pending input from @apricotbucket28, I think we can merge this.
| delta_scroll | ||
| } | ||
|
|
||
| fn get_valuator_axis_index(valuator_mask: &Vec<u32>, valuator_number: u16) -> Option<usize> { |
There was a problem hiding this comment.
This and the following 3-4 methods made me wonder whether we shouldn't introduce a ValuatorMask type (that might just wrap Vec<u32>) and define the methods on that type.
There was a problem hiding this comment.
I moved these to event.rs since that seems to be where event-related utilities go. It does seem weird to even put them in the platform crate, though. I imagine there is probably a cleaner way to do them with some library.
Could do the ValuatorMask newtype thing (I'm a big fan of newtypes!), but I'm leaning towards not making that change. Rationale is:
-
The other 2 methods aren't really specific to valuators, these are operations that are general bitmask operations on
Vec<u32>. -
Would only be constructed in one spot and not passed around, so there are no type safety benefits
There was a problem hiding this comment.
The code looks good and it seems to work well on my machine! Don't have a trackpad right now to retest it there, but I don't see why it would break.
The behavior also seems to be more stable than before, since I haven't had the mouse not work randomly.
Great work! 😄
mgsloan
force-pushed
the
x11-scroll-improvements
branch
2 times, most recently
from
c006f4a
to
2052be9
Compare
mgsloan
force-pushed
the
x11-scroll-improvements
branch
from
2052be9
to
1b872a9
Compare
|
@mrnugget Thanks for reviewing!! Made some changes based on your feedback. @apricotbucket28 Thanks for taking a look, glad it's working well for you!! |
mgsloan
force-pushed
the
x11-scroll-improvements
branch
from
1b872a9
to
4c9bc3f
Compare
* Now uses valuator info from slave pointers rather than master. This hopefully fixes remaining cases where scrolling is fully broken. zed-industries#14089, zed-industries#15970, zed-industries#17230 * Per-device recording of "last scroll position" used to calculate deltas. This meant that swithing scroll devices would cause a sudden jump of scroll position, often to the beginning or end of the file (zed-industries#14416). * Re-queries device metadata when devices change, so that newly plugged in devices will work, and re-use of device-ids don't use old metadata with a new device. * xinput 2 documentation describes support for multiple master devices. I believe this implementation will support that, since now it just uses `DeviceInfo` from slave devices. The concept of master devices is only used in registering for events. * Uses popcount+bit masking to resolve axis indexes, instead of iterating bit indices.
mgsloan
force-pushed
the
x11-scroll-improvements
branch
from
4c9bc3f
to
8bdf9cb
Compare
u32 can't be less than 0.
|
Thank you! Hopped on the branch to fix a clippy issue |
|
Just grabbed 0.156.0 build with this fix... On my openSUSE 15.6 system it's working well, and it's so nice to scroll with my mouse wheel! Thx @mgsloan 😃 |
Closes #14089, #14416, #15970, #17230, #18485
Release Notes:
Change details:
Also includes the commit from PR #18317 so I don't have to deal with merge conflicts.
Now uses valuator info from slave pointers rather than master. This
hopefully fixes remaining cases where scrolling is fully
broken. Scroll completely broken (Linux Mint) #14089, Unable to scroll the code space #15970, Scrolling doesn't work with trackpad and mouse wheel #17230
Per-device recording of "last scroll position" used to calculate
deltas. This meant that swithing scroll devices would cause a sudden
jump of scroll position, often to the beginning or end of the
file (Changing scroll devices will cause the scrollbar to snap to the top or bottom. #14416).
Re-queries device metadata when devices change, so that newly
plugged in devices will work, and re-use of device-ids don't use old
metadata with a new device.
xinput 2 documentation describes support for multiple master
devices. I believe this implementation will support that, since now it
just uses
DeviceInfofrom slave devices. The concept of masterdevices is only used in registering for events.
Uses popcount+bit masking to resolve axis indexes, instead of
iterating bit indices.