Skip to content

Fix x11 mouse scroll wheel#218

Merged
tomaka merged 1 commit intorust-windowing:masterfrom
rukai:master
Jul 12, 2017
Merged

Fix x11 mouse scroll wheel#218
tomaka merged 1 commit intorust-windowing:masterfrom
rukai:master

Conversation

@rukai
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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
Copy Markdown
Contributor

@Ralith Ralith Jul 11, 2017

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
Copy Markdown
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
Copy Markdown
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
[package]
name = "winit"
version = "0.7.2"
version = "0.7.3"
Copy link
Copy Markdown
Contributor

@Ralith Ralith Jul 12, 2017

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
Copy Markdown
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

1 more reviewer

@Ralith Ralith Ralith approved these changes

Reviewers whose approvals may not affect merge requirements

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