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 mapping special keys #1928

Merged
merged 10 commits into from
Sep 14, 2019
Merged

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Aug 26, 2019

Closes #1919.
The function is from https://stackoverflow.com/questions/8263618/convert-virtual-key-code-to-unicode-string.

keyCodeToString() translates a key code to an NSString. It would be nice to completely replace translateKey() so that we don't need to use a hardcoded mapping anymore but I don't think that's possible because for example pressing any function key results in a String with the UTF-8 representation having a value of 16 in the first and only byte. This does not allow distinguishing between those keys so we need to come up with another solution, possibly combining keyCodeToString() and translateKey().

@kovidgoyal
Copy link
Owner

You cant use strings from UCKeyTranslate to convert keys to key codes. You have no idea how to convert those strings back into key codes. Also since keyDown already calls UCKeyTranslate do we really need to call it again? And is we do why cant we use _glfw.ns.unicodeData?

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 26, 2019

You cant use strings from UCKeyTranslate to convert keys to key codes. You have no idea how to convert those strings back into key codes.

Can't we just use something like

NSNumber* charToKeyCode(NSString *keyChar)
{
    static NSMutableDictionary* dict = nil;

    if (dict == nil) {
        dict = [NSMutableDictionary dictionary];

        // For every keyCode
        for (CGKeyCode i = 0; i < 128; i++) {
            NSString* str = keyCodeToString(i);
            if (str != nil && ![str isEqualToString:@""]) {
                [dict setObject:[NSNumber numberWithInt:i] forKey:str];
            }
        }
    }

    return [dict objectForKey:keyChar];
}

to build a reverse mapping?

Also since keyDown already calls UCKeyTranslate do we really need to call it again?

We most likely don't need to call it again, I just wanted to be somewhat confident that this idea is even possible before doing any kind of optimisation.

@kovidgoyal
Copy link
Owner

I dont think I understand, wont that just end up mapping the special
char bak to semi-colon?

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 26, 2019

It shouldn't since the reverse mapping function uses keyCodeToString() and not translateKey(). And keyCodeToString() properly maps the key code 0x29 to the character ö with my keyboard.
Maybe I didn't really understand. What do we actually need the reverse mapping for? I assumed that we needed it when parsing keyboard mappings in the config file. Is that true? And do we need it for anything else?
I'm also not actually sure what you mean by "special char".

@kovidgoyal
Copy link
Owner

keyCodeToString() will map 0x29 to odiaresis and so the reverse map will
map odiaresis back to 0x29 and 0x29 is mapped to GLFW_KEY_SEMICOLON

The way key mapping works is that when the key event reaches kitty, if
the glfw key code is not GLFW_KEY_UNKNOWN then the glfw key code is
used. Only if the key in unknown is the native code used.

So what you would need to do is detect when keyCodeToString() is mapping
keys to something other than their canonical value, then either set the
glfw key to unknown or change the keyboard api to pass the key name as a
string and have kity match against that preferentially. And when using
keyCodeToString() you have to be very careful about IME events when
normal keys get used to generate arbitrary text, which will totally mess
up key mapping if it is based on strings.

Basically, this is a bug in Apple's API. They should not have reused key
codes for different keys. Any workaround we can come up with will not be
robust as a result.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 29, 2019

I implemented something that I thought would work but it doesn't yet. I tried adding new ä, ö and ü GLFW keys. Do you know what I could have missed?
At least the Shortcut: ctrl+ö has unknown key, ignoring error message isn't printed anymore, so I must have gotten something right.
Is there a way to declare a unicode character constant like 'ä' with UTF-8 encoding and uint32_t type or something in C? Right now I just use the hexadecimal values but that's not very readable.

@kovidgoyal
Copy link
Owner

There is a bunch of stuff to do when adding a new key, check the previous commits for adding the plus key for example.

@kovidgoyal
Copy link
Owner

No there is no way to use anythin other than a numeric constant in a switch statement. You can make it symbolic with a #define if you like.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 30, 2019

What do the constants in ENCODING in kitty/key_encoding.py mean?

@kovidgoyal
Copy link
Owner

They are used to handle the kitty extended keyboard protocol

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 30, 2019

How can I figure out what to map ä, ö, ü and ß to?

@kovidgoyal
Copy link
Owner

You dont need to figure it out, just run update_encoding() it will
update the file automatically.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 30, 2019

Why does translateKey() in glfw/cocoa_window.m contain upper case characters? At least on my keyboard only lower case characters are passed to the switch statement when pressing shift and a letter.

@kovidgoyal
Copy link
Owner

It contains both, since there is no reason not too and who knows what
apple will do in the future.

@Luflosi Luflosi force-pushed the keyboard_improvement branch 2 times, most recently from 2698351 to 5dbf40c Compare August 30, 2019 23:30
@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 30, 2019

It's quite painful to add new keys tbh.
So far I added ÄäÖöÜüß<#.
Don't merge this yet please, I'm not done yet.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 30, 2019

18000 lines changed in kitty/keys.h 😳. So many changes for just a few new keys.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 31, 2019

If I add dead keys, they won't work as keyboard shortcuts, right?

@kovidgoyal
Copy link
Owner

Whether a key is a dead key or not depends on the currently active
keyboard layout, and yes dead keys are not passed on to kitty fom glfw.

@Luflosi Luflosi force-pushed the keyboard_improvement branch 2 times, most recently from 57873dc to 5af371b Compare August 31, 2019 19:27
@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 31, 2019

Do you know what the test error message means?

@kovidgoyal
Copy link
Owner

It means interpret_key_evet() is not giving an expected results for the
F1 key

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 1, 2019

What do the SHIFTED_PRINTABLE and UN_SHIFTED_PRINTABLE variables in kitty/keys.py do? This is keyboard layout dependent and there are a couple mistakes in the definition of those variables.

@kovidgoyal
Copy link
Owner

they are used to interpret what alt+shift+key should become in the
traditional keyboard terminal modes which dont really support alt+shift

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 1, 2019

When you added UNDERSCORE to UN_SHIFTED_PRINTABLE in c2fd700, you didn't add it to SHIFTED_PRINTABLE. Was that intentional?

@kovidgoyal
Copy link
Owner

SHIFTED_PRINTABLE is a superset of UNSHIFTED_PRINTABLE, it start as a
copy of it and gets stuff added to it.

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 1, 2019

Yeah but that means, that UNDERSCORE is mapped to _ in both variables. All the other ones except SPACE have a different mapping for the shifted variant.

@kovidgoyal
Copy link
Owner

yes

@Luflosi Luflosi force-pushed the keyboard_improvement branch 2 times, most recently from 394c6b8 to 2cfdc25 Compare September 7, 2019 20:14
b3b830b did not actually make `update_encoding()` filter `GLFW_KEY_LAST_PRINTABLE` because `name` contained the key name after applying `symbolic_name()`, which replaces underscores with spaces. Instead of replacing the underscore in `LAST_PRINTABLE` with a space, I moved the check above the call to `symbolic_name()`. This is more readable and future-proof in my opinion.
@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 9, 2019

After rebasing onto the latest master, I got the following traceback when regenerating the automatically generated files:

Traceback (most recent call last):
  File "/nix/store/4i0nln542psiic6hcbj99x3r3rsrg1zg-python3-3.7.4/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/nix/store/4i0nln542psiic6hcbj99x3r3rsrg1zg-python3-3.7.4/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "./__main__.py", line 117, in <module>
    main()
  File "./__main__.py", line 108, in main
    namespaced(['+', first_arg[1:]] + sys.argv[2:])
  File "./__main__.py", line 68, in namespaced
    func(args[1:])
  File "./__main__.py", line 27, in runpy
    exec(args[1])
  File "<string>", line 1, in <module>
  File "./kitty/keys.py", line 8, in <module>
    from .key_encoding import KEY_MAP
  File "./kitty/key_encoding.py", line 356, in <module>
    config_key_map.update({k: g[v] for k, v in key_name_aliases.items()})
  File "./kitty/key_encoding.py", line 356, in <dictcomp>
    config_key_map.update({k: g[v] for k, v in key_name_aliases.items()})
KeyError: 'EXCLAM'

Do you know why this happens? What is the purpose of the g variable? It looks like it just maps a bunch of key names to the same name, e.g. {'MINUS': 'MINUS'}. What is the difference to the config_key_map variable?

@kovidgoyal
Copy link
Owner

g is the module globals, it contains all the key names defined at global
scope. And it is there mainly as a performance optimization so you can
check for a key with the is operator instead of the ==

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 9, 2019

Do you know why the exception happens? What can I do to fix it? Do I somehow need to add the keys to g as well?

@kovidgoyal
Copy link
Owner

If you have added them to ENCODING they will be automatically added to g

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 12, 2019

Well, I'm trying to add the new keys to ENCODING and KEY_MAP by running python3 . +runpy "from kitty.key_encoding import update_encoding; update_encoding()" but that throws the exception above. Running python3 . +runpy "from kitty.keys import generate_key_table; generate_key_table()" throws the same exception. If I do

diff --git a/kitty/key_encoding.py b/kitty/key_encoding.py
index 2e2ab6e4..63ca87c2 100644
--- a/kitty/key_encoding.py
+++ b/kitty/key_encoding.py
@@ -352,12 +352,10 @@ for key_name, enc in ENCODING.items():
     key_name = key_name.replace(' ', '_')
     g[key_name] = config_key_map[key_name] = key_name
     key_rmap[enc] = key_name
 config_key_map.update({
-    k: g[v] for k, v in key_name_aliases.items()
+    k: v for k, v in key_name_aliases.items()
 })
 
-enter_key = KeyEvent(PRESS, 0, g['ENTER'])
-backspace_key = KeyEvent(PRESS, 0, g['BACKSPACE'])
+enter_key = KeyEvent(PRESS, 0, 'ENTER')
+backspace_key = KeyEvent(PRESS, 0, 'BACKSPACE')
 del key_name, enc, g

everything seems to work fine.

@kovidgoyal kovidgoyal merged commit f3be5b5 into kovidgoyal:master Sep 14, 2019
@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 14, 2019

Running the two functions now obviously works because the keys are already in ENCODING but undoing the changes to the two generated files and trying to recreate them won't work. Adding new keys will lead to the same problem. Is there anything that can be done about that? Does my patch make sense or does it break anything?

@Luflosi Luflosi deleted the keyboard_improvement branch September 14, 2019 10:40
@kovidgoyal
Copy link
Owner

Of course it works. You simply have to run update_encoding() before adding
the new keys to key_name_aliases. And this can easily be fixed, by only
updating config_key_map with those entries in key_name_aliases that
exist in ENCODING.

@Luflosi
Copy link
Contributor Author

Luflosi commented Sep 17, 2019

Thanks for 41b0f88. It works as expected.

Luflosi added a commit to Luflosi/kitty that referenced this pull request Dec 25, 2019
`glfwGetCocoaKeyEquivalent()` in `glfw/cocoa_window.m` expects the returned characters to be of type `unichar`, which won't work for all unicode characters because it is defined as `unsigned short` according to https://developer.apple.com/documentation/foundation/unichar?language=objc, which is only guaranteed to be at least 16 bits in size. The code calling this function also expects the encoding to be UTF-16.
When I added the various keys in kovidgoyal#1928, I missed these facts. This means, that `glfwGetCocoaKeyEquivalent()` will behave unexpectedly when called with any of the new-ish keys. Luckily this function is currently only used for determining the macOS shortcut for `new_os_window` but I plan on using it more in the future.
Some of the constants, e.g. `NSBackspaceCharacter` are UTF-16 constants, so we can't just use UTF-8 everywhere.
I fixed the problem by using either UTF-8 characters packed into a `uint32_t` or UTF-16 characters in a `unichar` and then converting them to a UTF-8 encoded char string.

`NSEventModifierFlagNumericPad` isn't guaranteed to fit in a `unichar`, which made this undefined behaviour. It also didn't work. I tried to make it work using `NSEventModifierFlagNumericPad` as a modifier instead, as can be seen in this commit, but couldn't get it to work either because the constants used are native key codes and not unicode characters. Therefore the numpad keys will be removed in the next commit.
Luflosi added a commit to Luflosi/kitty that referenced this pull request Dec 27, 2019
`glfwGetCocoaKeyEquivalent()` in `glfw/cocoa_window.m` expects the returned characters to be of type `unichar`, which won't work for all unicode characters because it is defined as `unsigned short` according to https://developer.apple.com/documentation/foundation/unichar?language=objc, which is only guaranteed to be at least 16 bits in size. The code calling this function also expects the encoding to be UTF-16.
When I added the various keys in kovidgoyal#1928, I missed these facts. This means, that `glfwGetCocoaKeyEquivalent()` will behave unexpectedly when called with any of the new-ish keys. Luckily this function is currently only used for determining the macOS shortcut for `new_os_window` but I plan on using it more in the future.
Some of the constants, e.g. `NSBackspaceCharacter` are UTF-16 constants, so we can't just use UTF-8 everywhere.
I fixed the problem by using either UTF-8 characters packed into a `uint32_t` or UTF-16 characters in a `unichar` and then converting them to a UTF-8 encoded char string.

`NSEventModifierFlagNumericPad` isn't guaranteed to fit in a `unichar`, which made this undefined behaviour. It also didn't work. I tried to make it work using `NSEventModifierFlagNumericPad` as a modifier instead, as can be seen in this commit, but couldn't get it to work either because the constants used are native key codes and not unicode characters. Therefore the numpad keys will be removed in the next commit.
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.

Mapping Special Keys (Ctrl+ö)
2 participants