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

Allow characters as hotkey instead of assuming everyone uses a Latin keyboard #5679

Closed
DorpsGek opened this issue Jul 28, 2013 · 11 comments
Closed

Comments

@DorpsGek
Copy link

@DorpsGek DorpsGek commented Jul 28, 2013

adf88 opened the ticket and wrote:

Inside _keycode_to_name table (hothey.cpp) wrong keykode for comma key causes failures. Currently it's ',' which is 44 in decimal. This keycode (44) is assigned to F12 key. When reading a 'COMMA' from .cfg file it gets read as F12 key. When saving a WKC_COMMA an assertion pops up.

Bug exists since the begging of _keycode_to_name table existence, when save/loading code has been added (r20055).
Fix patch attached - changes ',' to WKC_COMMA.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5679
@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Jul 30, 2013

sbr wrote:

Plus (+) is also unusable as an hotkey. For the same reason than comma is assigned to F12, it's assigned to F11.
Moreover there is no WKC_PLUS to map 'PLUS' from the hotkey.cfg file.

-- plus-hotkey_00add-plus-windowkeycode_370db44761d.patch
This patch defines a new WindowKeyCode WKC_PLUS and map it to the video driver keycodes for SDL, Allegro and Win32.

* SDL provides SDLK_PLUS as scancode for the '+' key (http://www.libsdl.org/docs/html/sdlkey.html).
* No scancode for '+' exists in the Allegro driver (https://www.allegro.cc/manual/4/api/keyboard-routines/key). The unicode representation of '+' is tested before the scancode mapping loop.
* Win32 provide 0xBB (VK_OEM_PLUS) as scancode for the '+' key (http://www.kbdedit.com/manual/low_level_vk_list.html). I have no Windows computer and thus haven't tested the Win32 mapping.

-- plus-hotkey_01fix-map-hotkey-plus-to-key-plus_370db44761d.patch
This patch should be applied on top of both adf88's patch and the 00 patch above.
It map 'PLUS' from hotkeys.cfg to WKC_PLUS allowing 'PLUS' to be used as an hotkey.

Please find attached these two patches against r25634.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12450

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Jul 30, 2013

adf88 wrote:

Allegro doesn't have it probably because most keyboards don't have a '+' key. You can't "fix" it the way you did. Allegro's ureadkey returns translated char code rather then key code. When you press [SHIFT]+[=] on a regular PC keyboard you will get '+' char code. With your patch pressing [SHIFT]+[=] will be interpreted as WKC_SHIFT + WKC_PLUS rather then expected WKC_SHIFT + WKC_EQUALS.

I think that the 'PLUS' should be just removed from the _keycode_to_name table because that key is really rare.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12451

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Jul 30, 2013

frosch wrote:

Hotkeys are assigned to scancodes, not to characters. A "+" character hotkey makes no sense wrt. several keyboard layouts. I guess it should be "EQUAL" or so (i.e. sdl and stuff uses US layout).


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12452

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Jul 31, 2013

sbr wrote:

adf88:
I had hesitate to post my patches in this issue. I thought it was related with yours and added them here. I hope that doesn't mess up too much with your bug report and patch.

Allegro doesn't have it probably because most keyboards don't have a '+' key.

Granted for US keyboard layout and most of its derivative. Nevertheless, of the three keyboard layout I use regularly (dvorak-dvp, Latin American & French) only French has no '+' key.

You can't "fix" it the way you did. Allegro's ureadkey returns translated char code rather then key code. When you press [SHIFT]+[=] on a regular PC keyboard you will get '+' char code. With your patch pressing [SHIFT]+[=] will be interpreted as WKC_SHIFT + WKC_PLUS rather then expected WKC_SHIFT + WKC_EQUALS.

Effectively. I should have returned WKC_PLUS as soon as it matched the unicode test. The modifiers tests make only sense when testing the scancodes.
But for example, SHIFT+= has no meaning in the Latin American layout as there is no '=' key, '=' being in fact SHIFT+0.

I think that the 'PLUS' should be just removed from the _keycode_to_name table because that key is really rare.

The 'PLUS' entry in _keycode_to_name is buggy and should be fixed. As long as hotkeys could only be bound to US layout scancodes, the sane path is to remove the 'PLUS' entry in _keycode_to_name.

frosch:

Hotkeys are assigned to scancodes, not to characters. A "+" character hotkey makes no sense wrt. several keyboard layouts. I guess it should be "EQUAL" or so (i.e. sdl and stuff uses US layout).

As wrote above, that make sense from a US layout point of view. I was mixing two things, the fix to an incorrect _keycode_to_name entry and the localization of the hotkeys.cfg file.

OpenTTD is translated in many languages but we can only use US layout scancodes as hotkeys. I think that the localization of hotkeys is desirable to give the users the ability to define as hotkey whatever keys they have in their keyboards. Its an improvement I would like to investigate in another issue.

Please find attached a patch against r25637 to be applied on top of the one of adf88.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12456

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Jul 31, 2013

adf88 wrote:

Actually there is a way to deal with '+' and other keys too. Reassign F1-F12 and WKC_PAUSE to other codes. Why do they interfere with printable ASCII range (0x20..0x7E)?! That makes problems not only with '+' key but also with other keys. We don't need these constants: WKC_SLASH, WKC_SEMICOLON, WKC_EQUALS ... WKC_MINUS. It would be simpler and it would not cause conflicts if using ASCII representation: '/', ';', '=', ..., '-'. I think that all keycodes in range 0x20..0x7E should be considered as keys with corresponding ASCII representation (keycode == charcode).

Sébastien, your Allegro hack could work. Test against '+' after scancode test, not before. That makes conflicts only with unknown scancodes, thus no conflict at all.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12457

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Nov 12, 2013

Rubidium wrote:

To me it looks like the PLUS should never been added to the hotkey options. Furthermore the ',' should be replaced by WKC_COMMA and then it should work properly, right?

Basing it on the entered characters might not be such a good idea. Assume the % is above the 5. How do I write that down in the configuration? SHIFT+5, or the actually pressed character %... but is that with or without shift? In the former case the character is %, but it doesn't know that it means SHIFT+5 was pressed... so it isn't triggered. In the latter case, the character is % again, but the SHIFT is pressed as well... so it doesn't match the character.

It will also do weird things with composite keys; they won't be useful for hotkeys as you have to press them multiple times. On the other hand, you can use it to construct a huge number of extra hotkeys by using dead keys that two keys to construct characters.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12750

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Nov 13, 2013

adf88 wrote:

This is exactly what I'm talking about. That would fix issues.

But to extend current functionality to a wide range of keyboard layouts, I think there is one solution. Split keys and chars so there is no interference. Allow to define hotkeys by a key or by a character.

To set up a character hotkey, user would have to put it's Unicode representation into the cfg. A certain character hotkey would be fired up when user presses key combination that translates to the given character. Characters would be allowed only alone, no combinations.

Key hotkeys would have their mnemonics ('A_KEY', 'B_KEY', ..., 'COMMA', 'EQUEALS', ..., 'SHIFT', 'CTRL', ...). Only limited number may be allowed because of video drives i.e. Allegro doesn't know the [+] key so probably there would be no 'PLUS'. Any key combination would have consist of a single non-special key and any number of special keys.

Above ideas are similar to what we have currently. But currently keys interfere with chars.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12753

@DorpsGek
Copy link
Author

@DorpsGek DorpsGek commented Nov 13, 2013

Rubidium wrote:

The bug is fixed in r25973. The feature request remains open.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5679#comment12757

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Apr 13, 2018

See also #6101

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 5, 2019

Thanks for this. There's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. Since OpenTTD moved to GitHub, we use pull requests rather than patches, as they are a much more productive workflow.

I'm planning to close this soon (in 7 days), as we try to keep the issue count low for OpenTTD, it helps us focus on things that are important and fun.

If you would like to continue with this patch, the best way would be to move the patch to your own GitHub fork, update it for the current OpenTTD master, and then create a pull request. For more information, please see our CONTRIBUTING.md.

We are also happy to discuss directly on the issue, or in #openttd irc, including help to get this into a pull request. Thanks for your contribution!

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 24, 2019

Per previous comment, closing. Thanks!

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

Successfully merging a pull request may close this issue.

None yet
3 participants