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

Handle events on NumPad when NumLock is disabled #1188

Closed
wants to merge 3 commits into from

Conversation

jaafarbi
Copy link
Contributor

Hello, I tried to fix the issue #1048.
I mapped the NumPad keys as follow when both SHIFT and NUM aren't set:

  • SDLK_KP_6 --> AKEYCODE_DPAD_RIGHT
  • SDLK_KP_4 --> AKEYCODE_DPAD_LEFT
  • SDLK_KP_2 --> AKEYCODE_DPAD_DOWN
  • SDLK_KP_8 --> AKEYCODE_DPAD_UP
  • SDLK_KP_7 --> AKEYCODE_MOVE_HOME
  • SDLK_KP_1 --> AKEYCODE_MOVE_END
  • SDLK_KP_3 --> AKEYCODE_PAGE_DOWN
  • SDLK_KP_9 --> AKEYCODE_PAGE_UP
  • SDLK_KP_0 --> AKEYCODE_INSERT
  • SDLK_KP_PERIOD --> AKEYCODE_FORWARD_DEL

@npes87184
Copy link
Contributor

Hi,

Why the condition including not shift?

Thank you.


if (!(mod & (KMOD_NUM | KMOD_SHIFT))) {
// handling Numpad events when Num Lock is disabled
switch(from){
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that there are some coding style issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some modifications regarding the coding style issues. Can you tell me what was exactly the issue?

Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! From now, I only notice that we need a space after switch.

@jaafarbi
Copy link
Contributor Author

jaafarbi commented Mar 1, 2020

Hi, I tried to mimic the behavior of the keyboard. If you disable the NumLock, and try the NumPad numbers with SHIFT, it'll work as the NumLock was enabled.

EDIT: At least, on Ubuntu system

@npes87184
Copy link
Contributor

Hi, I tried to mimic the behavior of the keyboard. If you disable the NumLock, and try the NumPad numbers with SHIFT, it'll work as the NumLock was enabled.

EDIT: At least, on Ubuntu system

I try it on my windows 10 ver 1909, the shift + numpad when num lock off, it works like shift + arrow, insert, home, etc.

@jaafarbi
Copy link
Contributor Author

jaafarbi commented Mar 1, 2020

Should we go for Windows behavior? or the Ubuntu's one?

Thanks

@npes87184
Copy link
Contributor

Should we go for Windows behavior? or the Ubuntu's one?

Thanks

The behavior problem should contact @rom1v .
In my opinion, we can use different behavior in windows and ubuntu.

Thank you.

rom1v pushed a commit that referenced this pull request Mar 14, 2020
rom1v pushed a commit that referenced this pull request Mar 14, 2020
rom1v pushed a commit that referenced this pull request Mar 14, 2020
rom1v pushed a commit that referenced this pull request Mar 14, 2020
@rom1v
Copy link
Collaborator

rom1v commented Mar 14, 2020

Thank you for your contribution (and sorry for the delay).

I think checking SHIFT like you do produces the correct behavior: if shift is enabled, a text event will be generated, and managed as a text, so we must absolutely not send key events in that case.

IMO, the block should be moved few lines above so that it works even if --prefer-text is passed.

I squashed your commits, rebased on dev, and applied the changes I suggest: cd69eb4 (I also reordered the lines to sort by number, but it's just personal preference)

Please review and if it's ok I'll merge into dev.

@jaafarbi
Copy link
Contributor Author

Thank you for your answer. Since it is one of my first contributions, yes everything is fine for me.

@rom1v
Copy link
Collaborator

rom1v commented Mar 15, 2020

it is one of my first contributions

Congrats 🎉

Merged in dev. It will be in the next release.

@rom1v rom1v closed this Mar 15, 2020
@rom1v rom1v mentioned this pull request Mar 15, 2020
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.

3 participants