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

Fix macOS keyboard shortcut encoding #2224

Merged

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Dec 22, 2019

Please don't merge this yet. I need to do some cleanup and want to get some feedback first.

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

Is it ok to pass the NSString using a void pointer? Also please check if the memory for NSString is properly handled and not freed prematurely. I may need a retain or something but I still don't understand Objective-C memory management enough.

@kovidgoyal
Copy link
Owner

I'm not sure I follow. If some of the constants are UTF-16 strings, why
not simply use

unsigned short[16] cocoa_key

for the key? Or if you want to return them as UTF-8 which is a better
idea:

char[64] cocoa_key

The calling function can provide the array for it. So you dont have to
worry about memory management. I dont knwo what the limits on sizes of
key names are, so adjust the numbers above accordingly.

@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 23, 2019

Shouldn't two unsigned shorts or four chars be enough, one more of each when treating it as a string with a null character at the end?
I thought about returning a UTF-8 encoded char string but then in the case where I need to return one of the UTF-16 constants, I would convert it to an NSString and call UTF8String to get the UTF-8 encoded string. But later I need the string as an NSString again, which would result in a conversion back and forth, so I chose to just directly return an NSString. I can of course still return a UTF-8 encoded char array but I think I would prefer it if I could just use an NSString.

@kovidgoyal
Copy link
Owner

Depends on what size the keynames can reach, with these sorts of things
always best to be conservative. And I dont think a glfw api should be
returning NSString, too much potential for memory issues. Either return
UTF-16 or UTF-8.

@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 23, 2019

Ok, I will use UTF-8 then. But the length of the key names shouldn't matter, right? We are only returning a single Unicode character.

@kovidgoyal
Copy link
Owner

At the moment its only a single unicode character, who knows if that
will always be the case? I doubt all keys in the world use single character names.

@Luflosi Luflosi force-pushed the macos_fix_keyboard_shortcut_encoding branch from 6bdc2f9 to 8f5c30d Compare December 25, 2019 19:17
`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 Luflosi force-pushed the macos_fix_keyboard_shortcut_encoding branch from 8f5c30d to 1a368bc Compare December 27, 2019 14:09
See previous commit message for the reason.
@Luflosi Luflosi force-pushed the macos_fix_keyboard_shortcut_encoding branch from 1a368bc to 3842350 Compare December 27, 2019 14:16
@Luflosi Luflosi marked this pull request as ready for review December 27, 2019 14:16
@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 27, 2019

Please review.
I made the buffer 17 bytes large to allow for at least four unicode characters, even though I can't find any keys using more than four bytes in https://github.com/xkbcommon/libxkbcommon/blob/master/xkbcommon/xkbcommon-keysyms.h.
The _glfw_strdup() will only copy the part of the string that is actually used, so we only waste memory while the function executes.

@kovidgoyal kovidgoyal merged commit 3842350 into kovidgoyal:master Dec 28, 2019
@Luflosi
Copy link
Contributor Author

Luflosi commented Dec 28, 2019

Why is making new_window_key a char array instead of dynamically allocated desirable? It wastes a tiny bit of memory over the entire runtime of kitty, the maximum size needs to be passed to the function as an extra argument, the size of the array is hardcoded in a different place from where it is written to, making it harder to find and things like if (new_window_key[0]) are a little bit harder to read than if (new_window_key) IMO. The only positive I can see is that the memory doesn't need to be free()d at the end.

@kovidgoyal
Copy link
Owner

In general it is always better to avoid library functions allocating their own
memory. Memory should be allocated and freed as close to where it is
used as possible.

@Luflosi Luflosi deleted the macos_fix_keyboard_shortcut_encoding branch December 29, 2019 14:07
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