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

Revert "Don't trigger a key event if a key with the same associated char was pressed (#13773)" #13942

Merged

Conversation

erlehmann
Copy link
Contributor

@erlehmann erlehmann commented Oct 29, 2023

This reverts commit d57c936.

The reverted commit prevented recognition of key combinations.

Several keyboard layouts use a key combination to input a “+” (e.g. Neo2);
therefore some users could no longer input “+” to increase the view range.

Goal of the PR

I want to be able to input a “+” again in Minetest, outside of chat.

How does the PR work?

The PR simply reverts the logic that prevents recognition of key combinations.

Does it resolve any reported issue?

This PR resolves two issues actually:

The PR unresolves this issue:

It is probably possible to fix this issue again in a way that does not cause multiple regressions.
In any case, reverting the solution for #13773 just restores what Minetest did for many years.

Does this relate to a goal in the roadmap?

  • Fixing issues
  • General correctness.

Default keybindings being unusable unless you have a layout where “+” can be input without using a modifier key seems incorrect to me.

How to test

  1. Set your keyboard layout to German (Neo2).
  2. Verify that you can not increase the viewing range by pressing ISO Level 3 Shift (the key above the left Shift key) and b together (which inputs a “+”) at the commit previous to this patch.
  3. Verify that you can increase the viewing range by pressing ISO Level 3 Shift (the key above the left Shift key) and b together (which inputs a “+” ) with the patch in this PR.
  4. Verify that you can increase the viewing range by pressing the “+” key on the Numpad with the patch in this PR.

@erlehmann
Copy link
Contributor Author

erlehmann commented Oct 29, 2023

Other keyboard layouts that need a key combination to input a plus sign (without a numpad, i.e. on laptops) are:

  • French AZERTY (Shift =) before AZERTY amélioré
  • Belgian AZERTY (Shift =)
  • USA QWERT (Shift =)
  • US-International (Shift =)
  • UK QWERTY (Shift =)
  • Polish QWERTS (Shift 6)
  • Dvorak simplified (Shift =)
  • Colemak (Shift =)
  • MIT Space Cadet (Shift =)
  • 4800-52 mainframe / dumb terminal keyboard (Shift =)
  • Azerbaijani QÜERTY (Shift =)
  • Dvorak-fr (Shift [)
  • Bulgarian Cyrillic (Shift 3)
  • Japanese QWERTY (Shift ;)

… so I am pretty sure at least some other layout than Neo 2 would be affected negatively without this patch.

Edit: IIRC Shift is bound to sneak by default in Minetest though, so I have no idea how this can work at all.

Edit 2: According to an IRC discussion, Shift works fine. Unclear how that is different from other modifiers.

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

I think it makes sense to revert #13773 for now. It seems unlikely that we will find good alternative solutions for #13890 and #13899 before the release.

After a full migration to the SDL Irrlicht device, a better solution to #13770 might be to use SDL scancodes for user-defined bindings and most default bindings, but SDL keycodes for the default "viewing range" bindings. (Assuming both the Neo2 plus combination and the numpad plus trigger the "SDLK_PLUS" SDL keycode.) (See also minetest/irrlicht#207.)

@sfan5 sfan5 added this to the 5.8.0 milestone Oct 29, 2023
…har was pressed (minetest#13773)"

This partially reverts commit d57c936.

The reverted commit prevented recognition of key combinations.
It correctly changed a test case to no longer use “KEY_NUMPAD_5”.

Several keyboard layouts use a key combination to input a “+” (e.g. Neo2);
therefore some users could no longer input “+” to increase the view range.

Co-authored-by: savilli <78875209+savilli@users.noreply.github.com>
@erlehmann erlehmann force-pushed the maybe-fix-the-plus-sign-input-on-neo2 branch from ac29d74 to e41f836 Compare October 29, 2023 16:31
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Players can still use the the numpad by having NumLock disabled to assign the arrow keys instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants