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

[Linux] Send mapped wchar characters to internal key press #608

Merged
merged 2 commits into from Mar 19, 2021
Merged

[Linux] Send mapped wchar characters to internal key press #608

merged 2 commits into from Mar 19, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 18, 2021

Previously event loop would ignore any non-english characters.

Now it triggers key_press with values, accordingly mapped with X's keyboard mapping.
This works by itself, but i noticed a couple of options to improve handling of KeyPress/KeyRelease events

  1. Right now, we are guarding against key press repeats by storing state of whole keyboard, this can be improved by guarding against retrigger by just storing previous key press, this can enable us to ditch keyPressed array and simplify KEY() macro
  2. Is it intended to not ignore repeat in trigger_key_pressed? it might be better to let user handle repeats in the way they want, not tie it to what system thinks of it, but on the other hand using system's repeat key presses would integrate better with system's settings

Best of both worlds would be to send isRepeat with that event, but it would require matching implementations on all of the platforms, break compatibility, and might not even apply to some targets

@ghost
Copy link
Author

ghost commented Mar 19, 2021

I didn't make clear that I'm asking for advice.

About 1) should this be done, and if so, in this pr or is it better to make another one?
About 2) just wondering if we should prevent repeats for trigger_key_pressed?

@RobDangerous RobDangerous merged commit 3a7564d into Kode:master Mar 19, 2021
@RobDangerous
Copy link
Member

I'm undecided about preventing key repeats. That goes back to a time when keyPress didn't yet exist - only key up and down. I think we don't need it for keyPress but I need to have a closer look at all the targets first - please open an issue about it so I don't forget.

@Sanva
Copy link
Contributor

Sanva commented Mar 21, 2021

[ @RobDangerous @dmitry-ant ]

it might be better to let user handle repeats in the way they want, not tie it to what system thinks of it, but on the other hand using system's repeat key presses would integrate better with system's settings

If possible to implement, I'll be in favor of respecting system autorepeat, since it's not only about user preferences, but also user needs (accessibility).

Also, there are some problems now with the we are guarding against key press repeats by storing state of whole keyboard approach. For example, I've personally experienced a problem analogous to this one while playing with Kha and Dear ImGUI [1] last months, but with Ctrl instead of Alt → ocornut/imgui#3532 — to be specific, you can break keyboard input in Kha/Kinc Linux apps just by holding Ctrl and moving system's window focus to another window without releasing Ctrl.

Also, the thing about obtaining the keysym without current modifiers (those two zeros in XkbKeycodeToKeysym call) ignores NumLock status, which is that causes problems like this one → armory3d/armortools#713 . By the way, the keysym originally obtained with XLookupString had the correct keysym (I assume XwcLookupString behaves the same) with modifiers and all... but using it would change how Kinc Linux keyboard works, issuing key presses using the virtual key instead of trying to guess the physical one. Realizing this was what made me try to start a disscussion about Kinc keyboard, here → Kode/Kha#1291 .

[1] Working like a charm, by the way, but with a slightly modified Kinc.

@ghost
Copy link
Author

ghost commented Mar 21, 2021

@Sanva

If possible to implement, I'll be in favor of respecting system autorepeat, since it's not only about user preferences, but also user needs (accessibility).

Isn't key_pressed already behaves that way? it certainly is on my machine. With key_down autorepeat would interfere with videogamey way of tracking input. really depends on the use case.

Also, there are some problems now with the we are guarding against key press repeats by storing state of whole keyboard approach. For example, I've personally experienced a problem analogous to this one while playing with Kha and Dear ImGUI [1] last months, but with Ctrl instead of Alt → ocornut/imgui#3532 — to be specific, you can break keyboard input in Kha/Kinc Linux apps just by holding Ctrl and moving system's window focus to another window without releasing Ctrl.

I'm working on a PR for this one, wouldn't notice this issue with controlDown without you mentioning it! Since its really only used for generating cut/copy/paste events, just controlDown = false; in FocusOut event should fix this.

By the way, the keysym originally obtained with XLookupString had the correct keysym (I assume XwcLookupString behaves the same) with modifiers and all...

It shouldn't affect anything in switch statement that makes key_down call, it's a different KeySym variable there, it doesn't take modifiers into account, but we can't use keysym from XwcLookupString because it would ignore any alphabet characters when using non-english keymap. i don't really know how to attack that, but i'll look into that. Maybe just using keycodes (for alphabet characters only) would be safe there?

[1] Working like a charm, by the way, but with a slightly modified Kinc.

Sorry, i don't understand, is it not working without modification?

@Sanva
Copy link
Contributor

Sanva commented Mar 21, 2021

Isn't key_pressed already behaves that way? it certainly is on my machine. With key_down autorepeat would interfere with videogamey way of tracking input. really depends on the use case.

I completely agree with you. I thought that was about changing current behavior of key_press to something managed by Kha/Kinc instead of by the system.

I'm working on a PR for this one, wouldn't notice this issue with controlDown without you mentioning it! Since its really only used for generating cut/copy/paste events, just controlDown = false; in FocusOut event should fix this.

👍

Sorry, i don't understand, is it not working without modification?

Sorry about that note if it caused confusion, was only to show how I've got to know about the Ctrl issue. My Kinc modification in that case is completely unrelated to this.

[...] i don't really know how to attack that, but i'll look into that. Maybe just using keycodes (for alphabet characters only) would be safe there?

I'm not really sure about that either... 😕 but well, I'll be more than happy to test any change to see if I can see weird behavior or something.

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.

2 participants