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

x11: Support numpad arrows/Home/End/PageUp/PageDown/Insert/Delete #396

Merged
merged 1 commit into from
Mar 23, 2018

Conversation

forbjok
Copy link
Contributor

@forbjok forbjok commented Feb 4, 2018

Uncommented and corrected x11 key mappings for the numpad keys with num-lock turned off.

@tomaka
Copy link
Contributor

tomaka commented Feb 4, 2018

Seems suspicious that this was commented out, but I don't remember why it was the case.

@forbjok
Copy link
Contributor Author

forbjok commented Feb 4, 2018

There are a lot of commented-out x11 keycodes in there. My guess is they were copied from a list at some point, and only the ones deemed of immediate importance were mapped correctly, while the rest were just left commented-out to be fixed later.

That said, after posting this, I found another pull request (#114) that among other things maps some of these, but to other keycodes.

There is some discussion there about whether the numpad keys should map to the same VirtualKeyCode regardless of numlock state. While for my particular case (the terminal alacritty using it for input) this does not matter, I can see this being useful for other purposes, such as games where you might want to use the numpad keys as an additional input separate from the regular arrow keys. In that case, I guess alacritty would have to be changed to use a different method of mapping them to input depending on numlock state.
It really depends on what the intended purpose of VirtualKeyCodes is - to be able to uniquely identify each physical key on the keyboard, or to identify their functions.

However, either way, they should be mapped to something so that it's possible to use them.

EDIT: Also, the old pull request (#114) maps the incorrect codes for PageUp and PageDown. The numpad PageUp and PageDown keys with numlock off actually produce ffi::XK_KP_Prior and ffi::XK_KP_Next, not ffi::XK_KP_Page_Up and ffi::XK_KP_Page_Down.

EDIT2: Nevermind, it seems those have the same underlying int values, so it should make no difference.

@francesca64
Copy link
Member

I'm still of the belief that the VirtualKeyCodes should be dependent on numlock state, as numlock changes the semantic meaning of those keys. Someone wishing to use them as directional/etc. input in a game would use scancodes, which refer to physical key positions.

@forbjok
Copy link
Contributor Author

forbjok commented Feb 9, 2018

Yeah, I think I agree on that.

@francesca64
Copy link
Member

What's stopping this from being merged? It seems we have a good consensus.

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Needs an entry in the changelog and we're good to go!

@forbjok forbjok force-pushed the fix-x11-keypad-input branch from 400ffc9 to 5d17e62 Compare March 14, 2018 21:17
@forbjok forbjok force-pushed the fix-x11-keypad-input branch from 5d17e62 to 42ad49e Compare March 14, 2018 21:27
@forbjok
Copy link
Contributor Author

forbjok commented Mar 14, 2018

I changed it to use XK_KP_Page_Up and XK_KP_Page_Down instead of Prior and Next, as they have the same underlying code and are better named, and added a changelog entry for the change.

@tomaka tomaka merged commit 7a19465 into rust-windowing:master Mar 23, 2018
@tomaka
Copy link
Contributor

tomaka commented Mar 23, 2018

Thanks!

tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022
madsmtm pushed a commit to madsmtm/winit that referenced this pull request Jun 11, 2022
* publish new versions

* Update Cargo.toml

* Update CHANGELOG.md

Co-authored-by: wusyong <[email protected]>
Co-authored-by: Ngo Iok Ui (Wu Yu Wei) <[email protected]>
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.

3 participants