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 duplicate bit masks for caps lock and num lock #863

Merged
merged 1 commit into from
Jun 16, 2024

Conversation

timstr
Copy link
Contributor

@timstr timstr commented Feb 20, 2024

I'm using the Kitty terminal emulator with crossterm's raw mode, DISAMBIGUATE_ESCAPE_CODES and REPORT_ALL_KEYS_AS_ESCAPE_CODES. I noticed that caps lock was always being reported as active via event.state.contains(KeyEventState::CAPS_LOCK), even when toggling the key many times and restarting my program. Looking into crossterm's source, I found what looks like a simple copy/paste bug: CAPS_LOCK and NUM_LOCK both use the bit mask 0b000_1000. 0b0000_0001 is used but 0b0000_0010 and 0b0000_0100 aren't, so I went ahead and assigned those sequentially instead. With this change, caps lock is being detected as expected. I haven't tested numlock (my current keyboard doesn't have it clearly labeled) but I expect this can only help.

Copy link
Collaborator

@joshka joshka left a comment

Choose a reason for hiding this comment

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

This is a breaking change for anyone that has serialized values that they expect to deserialize. That said, the scenario where this actually breaks a real use case is pretty unlikely as they'd also have to be using the kitty disambiguation and not currently care about the difference between these two key states.

There's no meaningful usages of either CAPS_LOCK or NUM_LOCK in any repo in github that would be impacted by this change.

https://github.com/search?q=crossterm+num_lock&type=code
https://github.com/search?q=crossterm+caps_lock&type=code

This is reasonable to merge, but I'd suggest adding a note about breaking changes in into the commit message.

@TimonPost
Copy link
Member

Yeah -- just a change log entry will be sufficient and we can maintain semantic versioning.

@OvermindDL1
Copy link

I also just ran in to this, I can't imagine a case where this was intended and would actually be hitting someone as-it-is-written, possibly though it may be?

Copy link
Member

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

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

Nice catch!

@TimonPost TimonPost merged commit 33b4e37 into crossterm-rs:master Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants