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.

Sign up for GitHub

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

Jump to bottom

Fix x11 mouse scroll wheel #218

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Fix x11 mouse scroll wheel #218

merged 1 commit into from
Jul 12, 2017

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Jul 9, 2017

Fixes: #217

  • Direction issue was resolved by swapping the order of the operands in (unsafe { *value } - info.position)
  • "Stored up" scrolls issue was resolved by updating the scroll position, stored in the relevant Device, every XI_Enter event

@Ralith
Copy link
Contributor

Ralith commented Jul 9, 2017

Direction issue

Scroll direction seems correct to me--positive is down and right. What behavior are you observing? What do other platforms do? Bear in mind that some platforms (e.g. OSX) have a reversed direction convention.

"Stored up" scrolls issue was resolved by updating the scroll position, stored in the relevant Device, every XI_Enter event

That part looks good, thanks.

@rukai
Copy link
Contributor Author

rukai commented Jul 10, 2017

Well this is weird.

Found a file in /etc/X11/xorg.conf.d

Section "InputClass"
 Identifier "Joystick hat mapping"
 Option "StartKeysEnabled" "True"
 #MatchIsJoystick "on"
 Option "MapAxis5" "keylow=113 keyhigh=114"
 Option "MapAxis6" "keylow=111 keyhigh=116"
EndSection

I removed the file and rebooted.
The scroll direction was fixed!

Then I readded the file and rebooted.
Issue still fixed! wat.

The file has probably been there for years. (I dont actually know why I added it or what it was for)
I had previously checked out commit 15aafc2 and the previous commit and verified that was when the issue was introduced.

I guess its just an X11 bug that didnt occur in the previous way winit used the API.
And the bug was reset by clearing some sort of cache when the configuration was altered.

@rukai
Copy link
Contributor Author

rukai commented Jul 10, 2017 •
edited
Loading

I have removed my incorrect change to direction.

The scroll position update should be good to merge.

Ralith
Ralith reviewed Jul 11, 2017
View reviewed changes
Copy link
Contributor

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Looks good, just one quibble.

src/platform/linux/x11/mod.rs Outdated
@@ -406,7 +406,7 @@ impl EventsLoop {
for i in 0..xev.valuators.mask_len*8 {
if ffi::XIMaskIsSet(mask, i) {
if let Some(&mut (_, ref mut info)) = physical_device.scroll_axes.iter_mut().find(|&&mut (axis, _)| axis == i) {
let delta = (unsafe { *value } - info.position) / info.increment;
let delta = (unsafe { *value } - info.position) / info.increment; // TODO: Doesn't handle underflow/overflow
Copy link
Contributor

Choose a reason for hiding this comment

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

Is overflow a realistic concern here? The range of double is astronomical, and we're only computing differences between successive values delivered by X.

@rukai
Copy link
Contributor Author

rukai commented Jul 11, 2017

Didnt pay attention to the type, good point!
Fixed.

Ralith
Ralith approved these changes Jul 12, 2017
View reviewed changes
Copy link
Contributor

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Changes look good to me, though @tomaka might want you to drop the version bump.

Cargo.toml
@@ -1,6 +1,6 @@
[package]
name = "winit"
version = "0.7.2"
version = "0.7.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think bumping this is usually left for the maintainer to do immediately prior to making a release.

@tomaka
Copy link
Contributor

tomaka commented Jul 12, 2017

I don't mind the version bump.

@tomaka tomaka merged commit e1fba7d into rust-windowing:master Jul 12, 2017
@Ralith Ralith mentioned this pull request Jul 17, 2017
Closed
tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022
@pcwalton
Merge pull request rust-windowing#218 from s3bk/master
842348c 
fixes and a possible canvas addition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ralith Ralith Ralith approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mouse scroll issues on X11
3 participants
@rukai @Ralith @tomaka