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

Fix incorrect keycodes when using a non-US keyboard layout. #755

Merged
merged 5 commits into from
Feb 23, 2019
Merged

Conversation

Toqozz
Copy link
Contributor

@Toqozz Toqozz commented Jan 8, 2019

This commit fixes the issue described in #752, and uses the advised
method to fix it.

I'm relatively new to contributing and Rust (this is my first real contribution), and I'm sure my lack of experience is shining through here. Regardless, I hope that this PR fixes/assists in fixing the issue adequately!

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created an example program if it would help users understand this functionality

This commit fixes the issue described in #752, and uses the advised
method to fix it.
Copy link
Member

@francesca64 francesca64 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! I was on a business trip in Portland for the past week.

Thanks so much for this PR! From testing it, things work great, so the review just addresses code structure and style.

src/platform/macos/events_loop.rs Outdated Show resolved Hide resolved
src/platform/macos/events_loop.rs Outdated Show resolved Hide resolved
src/platform/macos/events_loop.rs Show resolved Hide resolved
src/platform/macos/events_loop.rs Outdated Show resolved Hide resolved
src/platform/macos/events_loop.rs Outdated Show resolved Hide resolved
src/platform/macos/view.rs Outdated Show resolved Hide resolved
src/platform/macos/view.rs Outdated Show resolved Hide resolved
src/platform/macos/view.rs Outdated Show resolved Hide resolved
src/platform/macos/view.rs Outdated Show resolved Hide resolved
src/platform/macos/view.rs Outdated Show resolved Hide resolved
Co-Authored-By: Toqozz <toqoz@hotmail.com>
@Toqozz
Copy link
Contributor Author

Toqozz commented Jan 15, 2019

Thanks for the in-depth reply! I'll get to work on these issues now.

@Toqozz
Copy link
Contributor Author

Toqozz commented Jan 23, 2019

I've applied changes which (I believe) resolve most of what we've talked about here. There were some minor details that I was uncertain about (i.e. where to put get_scancode()), but these should be able to be easily resolved.

I was quite busy last week -- hence the delay -- but I should be able to resolve anything more quite quickly, so please let me know.

Thanks :)

@chrisduerr chrisduerr mentioned this pull request Jan 25, 2019
@chrisduerr
Copy link
Contributor

I just wanted to chime in and say that I've been using Alacritty with your winit branch for a while (on macOS) and it seems to be working great, haven't run into any issues.

@chrisduerr
Copy link
Contributor

I don't think the remaining two 'TODO's are relevant for this PR so it might be a bit misleading to keep them in. Build also is just failing because there's a merge conflict in the CHANGELOG file.

Is there anything left to be done other than fixing the merge conflict? The functionality seems good to go and it would be a shame to forget about a QoL change like this.

@Toqozz
Copy link
Contributor Author

Toqozz commented Feb 16, 2019

What are the remaining two TODO's that you refer to? Did I miss something?

I would definitely still like to push this; things can be very annoying/confusing for non-qwerty users without it. I might even consider it crucial, since so many projects rely on winit.

@chrisduerr
Copy link
Contributor

@Toqozz I'm talking about the list in the original comment of this PR. This is part of winit's PR template, but unfortunately if you look at the list of PRs, it shows up as '2 of 4' tasks done. So that can easily confuse maintainers to think there's still progress the author needs to complete.

@francesca64
Copy link
Member

Sorry for the delay! Thanks so much for this great PR!

@francesca64 francesca64 merged commit 9a23ec3 into rust-windowing:master Feb 23, 2019
elinorbgr pushed a commit to elinorbgr/winit that referenced this pull request Apr 7, 2019
…dowing#755)

* Fix incorrect keycodes when using a non-US keyboard layout.

This commit fixes the issue described in rust-windowing#752, and uses the advised
method to fix it.

* Style fixes

Co-Authored-By: Toqozz <toqoz@hotmail.com>

* Refactoring of macOS `virtualkeycode` fix (rust-windowing#752)

* Applies requested changes as per pull request discussion (rust-windowing#755).
elinorbgr pushed a commit to elinorbgr/winit that referenced this pull request Apr 7, 2019
…dowing#755)

* Fix incorrect keycodes when using a non-US keyboard layout.

This commit fixes the issue described in rust-windowing#752, and uses the advised
method to fix it.

* Style fixes

Co-Authored-By: Toqozz <toqoz@hotmail.com>

* Refactoring of macOS `virtualkeycode` fix (rust-windowing#752)

* Applies requested changes as per pull request discussion (rust-windowing#755).
felixrabe pushed a commit to felixrabe/winit that referenced this pull request Jun 30, 2019
…dowing#755)

* Fix incorrect keycodes when using a non-US keyboard layout.

This commit fixes the issue described in rust-windowing#752, and uses the advised
method to fix it.

* Style fixes

Co-Authored-By: Toqozz <toqoz@hotmail.com>

* Refactoring of macOS `virtualkeycode` fix (rust-windowing#752)

* Applies requested changes as per pull request discussion (rust-windowing#755).
kosyak pushed a commit to kosyak/winit that referenced this pull request Jul 10, 2019
…dowing#755)

* Fix incorrect keycodes when using a non-US keyboard layout.

This commit fixes the issue described in rust-windowing#752, and uses the advised
method to fix it.

* Style fixes

Co-Authored-By: Toqozz <toqoz@hotmail.com>

* Refactoring of macOS `virtualkeycode` fix (rust-windowing#752)

* Applies requested changes as per pull request discussion (rust-windowing#755).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants