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 gtk key repeat logic #1081

Merged
merged 4 commits into from
Jul 12, 2020
Merged

Fix gtk key repeat logic #1081

merged 4 commits into from
Jul 12, 2020

Conversation

raphlinus
Copy link
Contributor

Keep track of which keys are pressed and use that to determine whether
a key is a repeat. The previous logic of comparing the keyval to the
last value fails in the case of multiple keys.

Also add lock modifiers, a followup from #1079.

Fixes #1080

Keep track of which keys are pressed and use that to determine whether
a key is a repeat. The previous logic of comparing the keyval to the
last value fails in the case of multiple keys.

Also add lock modifiers, a followup from #1079.

Fixes #1080
@raphlinus
Copy link
Contributor Author

For more context on the logic for modifiers: Mozilla doesn't use gdk for this, but reaches through and calls xkb_keymap_mod_get_index to map the modifiers (see KeymapWrapper::SetModifierMasks in nsGtkKeyUtils.cpp). The name it looks up for num-lock is XKB_MOD_NAME_NUM, which I believe is "Mod2". Calling this function directly is not appealing, as ideally we'd like to preserve the ability to build and test the gtk port on non-X11 systems.

I also have a comment in there about using Keymap as a more principled way of getting the numlock state. Oddly, Mozilla does this for nsBidiKeyboard.cpp, but not for numlock. Maybe somebody who knows this better than me can shed some light. In the meantime, I'm pretty satisfied that the logic in this PR is good enough, and doing something else might cause more problems.

@raphlinus
Copy link
Contributor Author

Also, Mozilla's logic is a bit different. Instead of having a hashmap, they have a "last key pressed" (which is a hardware keycode), but don't clear it on keyup if the value doesn't match. I also confirm that Mozilla works here. I think maybe that logic might be a touch more robust, as it's less likely that a key will be stuck in the down position on something like focus change.

Use single optional keycode rather than hashset as it's more robust
against a key getting "stuck" due to focus or whatnot.
@azymohliad
Copy link

I can confirm that it fixes #1080.

Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Seems to be working!
Once the changlog entry is fixed, we can merge this.

Comment on lines +847 to +849
// Note: this is the usual value on X11, not sure how consistent it is.
// Possibly we should use `Keymap::get_num_lock_state()` instead.
(ModifierType::MOD2_MASK, Modifiers::NUM_LOCK),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tested this on Wayland and I'm getting MOD2_MASK as well.

CHANGELOG.md Outdated
@@ -15,6 +15,7 @@ You can find its changes [documented below](#060---2020-06-01).
- `Scale::from_scale` to `Scale::new`, and `Scale` methods `scale_x` / `scale_y` to `x` / `y`. ([#1042] by [@xStrom])
- Major rework of keyboard event handling ([#1049] by [@raphlinus])
- `Container::rounded` takes `KeyOrValue<f64>` instead of `f64`. ([#1054] by [@binomial0])
- Fix Gtk key repeat logic ([#1081] by [@raphlinus])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go into the Fixed section.

- GTK: Don't interrupt `KeyEvent.repeat` when releasing another key. ([#1081] by [@raphlinus])

Copy link
Collaborator

Choose a reason for hiding this comment

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

@raphlinus Did you miss my suggestion for the changlog entry or did you not like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed it, I only caught the part about it being in the wrong section. That's what I get for trying to do this stuff before I've had my coffee.

@raphlinus raphlinus merged commit 1d6a30f into master Jul 12, 2020
@raphlinus raphlinus deleted the gtk-kb-fix branch July 12, 2020 17:17
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.

Event::KeyDown reports incorrect repeat value after holding multiple keys at specific order
3 participants