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

Don't trigger a key event if a key with the same associated char was pressed #13773

Merged
merged 2 commits into from Sep 22, 2023

Conversation

savilli
Copy link
Contributor

@savilli savilli commented Sep 1, 2023

Fix #13770
See #13770 (comment)

To do

This PR is Ready for Review.

How to test

Check that pressing a numpad key doesn't change the active item

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.

Confirmed the bug on Linux with non-SDL Irrlicht. The fix works and makes sense.

The fix also results in being unable to change your viewing range using the numpad + and - keys with the default keymap, but I don't think that's a problem (this is expected, the non-numpad versions of the keys still work).

I can't reproduce the bug on Linux with SDL Irrlicht. If you press a numpad key there, Char always seems to be set to 0.

@sfan5
Copy link
Member

sfan5 commented Sep 5, 2023

Consider adding a short unit test for operator==, I believe we had some keycode test somewhere already.

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 13, 2023
@savilli
Copy link
Contributor Author

savilli commented Sep 13, 2023

Consider adding a short unit test for operator==, I believe we had some keycode test somewhere already.

Done

@@ -111,7 +111,7 @@ void TestKeycode::testCompare()
{
// Basic comparison
UASSERT(KeyPress("5") == KeyPress("KEY_KEY_5"));
UASSERT(!(KeyPress("5") == KeyPress("KEY_NUMPAD_5")));
UASSERT(!(KeyPress("5") == KeyPress("KEY_NUMPAD5")));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was broken for a while

@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 22, 2023
@grorp grorp requested a review from sfan5 September 22, 2023 19:10
@sfan5 sfan5 merged commit d57c936 into minetest:master Sep 22, 2023
13 checks passed
erlehmann added a commit to erlehmann/minetest that referenced this pull request Oct 29, 2023
…har was pressed (minetest#13773)"

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.
erlehmann added a commit to erlehmann/minetest that referenced this pull request 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>
sfan5 pushed a commit that referenced this pull request Oct 30, 2023
…har was pressed (#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>
cx384 pushed a commit to cx384/minetest that referenced this pull request Dec 9, 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>
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 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>
cosin15 pushed a commit to cosin15/minetest that referenced this pull request Feb 24, 2024
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.

Keypad input doubles as digit input
4 participants