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

Char Mods callback doesn't trigger when CTRL is pressed #672

Closed
ocornut opened this issue Dec 25, 2015 · 22 comments
Closed

Char Mods callback doesn't trigger when CTRL is pressed #672

ocornut opened this issue Dec 25, 2015 · 22 comments
Assignees
Labels
question Please use the support label instead Windows Win32 specific (not Cygwin or WSL)
Milestone

Comments

@ocornut
Copy link
Contributor

ocornut commented Dec 25, 2015

@dougbinks may be interested

Tested on current master (as of today) with 'events.c' demo on Windows 8.
The callback set from glfwSetCharModsCallback is called when ALT modifier is modified (as messaged via WM_SYSCHAR) but not when CTRL modified is pressed. Making it hard for an application to process shortcuts (e.g. CTRL+S) while relying on translated characters.

I have looked into it, surprising enough it appears that it is a bit of a pain to retrieve this information under Windows.

Adding this at the end of message handler for keys messages appears to fix it, albeit in a rather ugly way:

        case WM_KEYDOWN:
        case WM_SYSKEYDOWN:
        case WM_KEYUP:
        case WM_SYSKEYUP:
[...]
            if ((uMsg == WM_KEYDOWN || uMsg == WM_SYSKEYDOWN) && (mods & GLFW_MOD_CONTROL))
            {
                static BYTE keys[256] = { 0 };
                WCHAR buf[4];
                int buf_sz;
                int n;
                keys[VK_SHIFT] = (mods & GLFW_MOD_SHIFT) ? 0x80 : 0;
                buf_sz = ToUnicodeEx(wParam, scancode, keys, buf, 4, 0, NULL);
                for (n = 0; n < buf_sz; n++)
                    _glfwInputChar(window, buf[n], getKeyMods(), FALSE);
            }

            break;

As I understand it, DefWindowProc generates WM_CHAR / WM_SYSCHAR messages from WM_KEYDOWN / WM_SYSKEYDOWN messages, and code above replicated the effect for me. It looks a little dodgy but I haven't spotted a flaw with it yet.

It might fail on certain edge cases (I don't know them) but I am tempted to say that it can't be a regression since currently we get nothing under Windows when CTRL is held.

(With Microsoft IME active when holding is doesn't popup anyway so I typically get the Latin character in this case, which is desirable)

@ocornut
Copy link
Contributor Author

ocornut commented Dec 25, 2015

As a side note I've had user report that a similar problem appears on Mac, may be worth a separate fix. Unfortunately I don't have a Mac here to test/fix. Haven't tested on Linux either.

About testing on Mac, emoon said:

So it seems Ctrl+A-Z doesn't work but 0-9 and other non A-Z keys (like /= etc) works fine.
Alt seems to work on all keys. Ctrl+Alt-.... behaves the same as Ctrl- only

Throwing the guess that just like Windows, some path are translating CTRL+AlphaLetter to a < 32 character value by default?

@dmitshur
Copy link
Collaborator

I don't think you can use characters for determining shortcut presses like Ctrl+S or Alt+S. The modifier keys Ctrl or Alt may modify how characters get processed. For example, on some keyboard layouts, pressing Alt+S generates a 'ß' character. So if, for example, you tried to detect a Ctrl+Alt+S shortcut press, it would be impossible to trigger it. (It'd be seen as Ctrl+Alt+ß or something.)

@ocornut
Copy link
Contributor Author

ocornut commented Dec 26, 2015

That's an interesting point. Note that the above code would see Ctrl+Alt+S in this situation (but probably not Alt-ß since alt only isn't altered by this code).

@ocornut ocornut closed this as completed Dec 26, 2015
@ocornut ocornut reopened this Dec 26, 2015
@ocornut
Copy link
Contributor Author

ocornut commented Dec 26, 2015

Sorry I didn't mean to close the issue, it was a misclick.

Which keyboard layout does that, out of curiosity? So I can perform more testing. German/Germany doesn't seem to have that.

I'm not sure it would be a problem for GLFW to aim to handle character inputs this way while CTRL-key is held, since this is the intent of glfwSetCharModsCallback .If there are holes/edge cases they can be fixed later.

How do you think shortcuts should be handled?

@dougbinks
Copy link
Contributor

A quick thought, though I'm both down with flu and busy dealing with combined floods & family celebrations (yeah global warming), so I've not researched this as much as I should.

@elmindreda should be able to clarify at some point, but from looking at the docs I'd assume glfwSetCharModsCallback may or may not be designed to receive none printable characters (CTRL+S) plus the modifier (CTRL).

CTRL+S and other none printables will be received by the key callback, for example:

void key_callback(GLFWwindow* window, int key, int scancode, int action, int mods)
{
    if( ( GLFW_MOD_CONTROL == mods ) && ( 'Z' == key ) )
        undo();
}

I use this in my own apps, so know it works.

@dougbinks
Copy link
Contributor

Adding a note to above: I meant that it's unclear if glfwSetCharModsCallback is designed to receive ALL non-printables, since anything lacking a unicode code point can't be handled.

@dmitshur
Copy link
Collaborator

Which keyboard layout does that, out of curiosity? So I can perform more testing. German/Germany doesn't seem to have that.

It's the default US English keyboard layout on a MacBook Pro. I'm pretty sure all Macs print different characters when you hold down Alt.

I forgot about glfwSetCharModsCallback. Perhaps what I said in my above comment is not correct then.

@ocornut
Copy link
Contributor Author

ocornut commented Dec 26, 2015

Bit of a side-topic here, but I'd be curious as to how you think an application should handle CTRL/ALT shortcuts if they expect to use translated letters, if you have any opinion/experience on the topic? I came to glfwSetCharModsCallback because I imagined it was the only reliable way to do so and so I was surprised to find how untrivial it was.

@dmitshur
Copy link
Collaborator

I think that's a really good question, I too would like to think more carefully about it and figure out the best way to solve the problem.

I initially thought that normal key handler should be used, but that might not be correct.

Using character handler, even with mods, seems imperfect too. It has weird handling of some of the modifier keys (primarily Alt), and it may produce repeated character events that would be undesired. If I press Ctrl+S once and hold it for 2 seconds, I wouldn't want there to be multiple save events triggered.

I'd really like to know what @elmindreda thinks since this is probably a solved problem, I just can't remember what the solution was.

@ocornut
Copy link
Contributor Author

ocornut commented Dec 27, 2015

On the matter is handling shortcuts, Windows appears to have two related systems:

1) RegisterHotKey / WM_HOTKEY
This takes virtual key code only so we can ignore that (it is basically sugar over a key handler).
RegisterHotKey: https://msdn.microsoft.com/en-us/library/windows/desktop/ms646309%28v=vs.85%29.aspx

2) LoadAccelerators / TranslateAccelerator / WM_COMMAND
Accelerators: https://www-user.tu-chemnitz.de/~ygu/petzold/ch10d.htm
TranslateAccelerator: https://msdn.microsoft.com/en-us/library/windows/desktop/ms646373%28v=vs.85%29.aspx

TranslateAccelerator() Processes accelerator keys for menu commands. The function translates a WM_KEYDOWN or WM_SYSKEYDOWN message to a WM_COMMAND or WM_SYSCOMMAND

Accelerators are defined with ACCEL structure and can be specified either by virtual key code, either by character code. This is a rather OS-specific high-level system (because a table resources has to be built, etc.) but it looks like what we'd like is essentially to replicate whatever TranslateAccelerator() is doing to decide on a character from a WM_KEYDOWN message. Exact logic to extract a character might or not be exactly what my code above is doing already, at least comforting me that my code is headed in the right direction. (except the accelerator API sends WM_COMMAND messages and doesn't emit characters, but it appears that essentially we'd get the same information in an OS agnostic way from a fully functional CharMods callback.

I have done some basic reverse engineering on TranslateAccelerator(), user-land code filters and only cares about WM_KEYDOWN/WM_KEYUP/WM_CHAR/WM_DEADCHAR/WM_SYSKEYDOWN/WM_SYSKEYUP/WM_SYSCHAR, so I selectively filtered those messages.
To read a "F1" it is only requiring the WM_KEYDOWN message. To read a "CTRL+A" it is only relying on WM_CHAR sending non-printable (in this case a 0x01 character).
Then I went and wanted to try non alphabetical CTRL+XX shortcuts and the resource compiler refuse them.
Error 1 error RC2154: control character out of range [^A - ^Z] poppad2.rc 53

So TranslateAccelerator() being an old API relies on non-printable 1-27 for CTRL+letter (or CTRL+ALT+letter) and must apply a translation table to apply keyboard layout over that. Unfortunately I can step through a disassembly once it gets into Nt* stuff (not user land I presume?).

Now if I look at most Windows application it seems that CTRL+XX shortcuts are limited to VK (like the functions keys) or alphabetical characters. More elaborate applications like Visual Studio can bother writing more custom code.

TL;DR;

Based on this minimum requirement to implement an equivalent to TranslateAccelerator() is that GLFW could take WM_CHAR 1-27, convert to VK int vk = wParam-1+'A', convert this alphabetical VK to a translated character using MapVirtualKeyEx() and send this character to the CharMods callback with CTRL modifier held.

(note, I don't think it is the right solution, but providing it to make the explanation more complete)

So this

        case WM_CHAR:
        case WM_SYSCHAR:
        case WM_UNICHAR:
        {
            const GLFWbool plain = (uMsg != WM_SYSCHAR);

            if (uMsg == WM_UNICHAR && wParam == UNICODE_NOCHAR)
            {
                // WM_UNICHAR is not sent by Windows, but is sent by some
                // third-party input method engine
                // Returning TRUE here announces support for this message
                return TRUE;
            }

            _glfwInputChar(window, (unsigned int) wParam, getKeyMods(), plain);
            return 0;
        }

Becomes

        case WM_CHAR:
        case WM_SYSCHAR:
        case WM_UNICHAR:
        {
            const int mods = getKeyMods();
            GLFWbool plain = (uMsg != WM_SYSCHAR);

            if (uMsg == WM_UNICHAR && wParam == UNICODE_NOCHAR)
            {
                // WM_UNICHAR is not sent by Windows, but is sent by some
                // third-party input method engine
                // Returning TRUE here announces support for this message
                return TRUE;
            }

            if (wParam >= 1 && wParam <= 27)
            {
                int vkcode = ((int) wParam) - 1 + 'A';
                UINT ch = MapVirtualKeyW(vkcode, MAPVK_VK_TO_CHAR);
                if (ch != 0)
                {
                    wParam = ch;
                    plain = FALSE;
                }
            }

            _glfwInputChar(window, (unsigned int) wParam, mods, plain);
            return 0;
        }

However, this would be restricted to alphabetical characters and lose casing, so even so the information is sufficient to emulate TranslateAccelerator() it is rather bit dodgy to feed that into the CharMods callback.

As thus, my suggestion and PR above may be a more complete solution. It will certainly fail some edge cases BUT it is still a notable and very useful improvement for the purpose for handling shortcuts. Consider the fact than a end-user application is extremely unlikely to provide a default CTRL+ALT+ß shortcut. It is does it must have a mechanism to redefine shortcuts in which case the translated user inputs will matter anyway (e.g. it may record and display CTRL+ALT+S that would be fine). As far as I understand the keyboard layout of most (all?) regions have known Latin letters, and shortcuts are commonly expressed with alphabetical letters.

So I think adding code like in the PR would fill most of that hole and still be of great use to anyone willing to implement a shortcut scheme with proper translation.

@dougbinks
Copy link
Contributor

@ocornut In response to how you think we should handle shortcuts (I take it you mean shortcuts to actions and not key combos which deliver a unicode printable character), then I think they should be handled via the physical key callback. The displayed shortcut key then becomes an issue (as Q would be displayed instead of A for an FR keyboard etc.), this is being worked on, see below.

Note that using unicode characters means you won't get the F1 and some other physical keys which don't have translations. Using physical keys alongside a unicode text input callback isn't a problem, as shortcuts shouldn't be bound to printable (visible) characters.

Discussions (lots!):
#117
https://sourceforge.net/p/glfw/discussion/247562/thread/c9bd5374/
#114
#464

Branch:

https://github.com/glfw/glfw/commits/horrible-keyname-hacks

@elmindreda elmindreda changed the title Char Mods callback doesn't trigger when CTRL is pressed (Windows) Char Mods callback doesn't trigger when CTRL is pressed Dec 27, 2015
@elmindreda elmindreda added the Windows Win32 specific (not Cygwin or WSL) label Dec 27, 2015
@elmindreda
Copy link
Member

What @dougbinks said.

Key chords should be handled with the key callback, not any of the character callbacks. The previously missing piece was how to refer to printable physical keys, which is why glfwGetKeyName has been added for the next release.

@elmindreda elmindreda self-assigned this Dec 27, 2015
@ocornut
Copy link
Contributor Author

ocornut commented Dec 27, 2015

Thanks!

Any intuition when that branch will be merged into working master? (or 3.2 that for that matter?). May I provide any help on remaining work to have glfwGetKeyName() merged in?

I was about to say there is still value in adding support for CTRL in the CharMods callback seeing it is simple code that doesn't rely on data. But..

For my library (adding shortcuts to ImGui ocornut/imgui#456 ) I am mostly interested in finding the most natural common ground between different input layers, could be GLFW, SDL2, raw Win32 or various high-level engines. The library is platform agnostic and not reliant on GLFW, so above all I trying to devise a system that the end-user can easily feed with the right data.

By looking at SDL2 it doesn't look like it supports a scheme like CharMods does anyway (tried now) so I suppose it is saner that I drop this idea and just go and design an API that's easy to use with the keys events commonly offered by different API. I have a major redesign of my own inputs system to do anyway so it's a good timing that I encounter this problem and I can solve it neatly.

I believe you can close this and the PR. Thanks a lot for your time :)

@elmindreda elmindreda added the question Please use the support label instead label Dec 27, 2015
@elmindreda
Copy link
Member

glfwGetKeyName was merged a couple of months ago with 9c31541. I'm going to do some more work to make the results line up better between platforms (although I don't know yet if I can promise it always will).

@elmindreda elmindreda added this to the milestone Dec 27, 2015
@ocornut
Copy link
Contributor Author

ocornut commented Dec 27, 2015

Thanks, somehow I assumed it wasn't merged. I personally don't mind the strings varying per platform, my user code will need to ignore those strings either way to be portable across engines. So please do not consider this thread as putting any extra weight on that.

@elmindreda
Copy link
Member

@ocornut Thank you, always good to know!

@dmitshur
Copy link
Collaborator

Thanks, I'm glad the solution was this simple.

I do wonder, does anyone recall what glfwSetCharModsCallback was added for? Why would you want to know the mods for character callback?

@elmindreda
Copy link
Member

@shurcooL Home-made IMEs (#203) and event data preservation (#40, #305).

@ocornut
Copy link
Contributor Author

ocornut commented Mar 14, 2019

Sorry for replying to an old thread (this might lead to a new thread if we are in agreement!).

The discussion here (which started from a discarded PR) led to the conclusion that Shortcuts (e.g. CTRL+A) should be relying on Key callbacks (not Char callbacks), and the app should "translate" the keycode into a localized string, using glfwGetKeyName() which GLFW_KEY_A which would return e.g "A" or "Q" depending on the keyboard mapping.

Those are the key comments discussing this:
#672 (comment)
#672 (comment)
e.g.

Key chords should be handled with the key callback, not any of the character callbacks. The previously missing piece was how to refer to printable physical keys, which is why glfwGetKeyName has been added for the next release.

This suggest that my originally CTRL+A shortcut should be displayed as "CTRL+Q" in the user interface of a French user.

Long after that discussion I realized that noone reacted to the fact that this doesn't seem in line with how the majority of desktop applications are functioning.

If I switch between US and FR keyboards, all the Windows applications at my disposal handle CTRL+A when I press the key that would produce the 'a' character.

  • Win32 native events WM_KEYDOWN provide localized VK_ keys in wParam, so when I change keyboard mapping and press the same physical key I can receive the VK_ value for 'A' or 'Q' (which happens to be 'A' or 'Q' due to how VK_ values are mapped).

  • With SDL when pressing same physical key with different mapping I get the same scancode but different keycodes.

  • Yet GLFW gives me the same key and the same scancode in my key callback regardless of my active keyboard mapping, which AFAIK makes it impossible to handle shortcuts in a standard way?
    Basically GLFW is discarding the wParam which provides this information:

win32_window.c (current GLFW head)

const int key = translateKey(wParam, lParam);
const int scancode = (lParam >> 16) & 0x1ff;

For most keys translateKey() basically does:

return _glfw.win32.keycodes[HIWORD(lParam) & 0x1FF];

Which is then passed to the user key callback as key.

I may be wrong but feel this is undoing the point of having separated key and scancode.
Right now with GLFW there's more or less a 1<>1 mapping between key and scancode regardless of the keyboard layout, whereas the concept of key should in most cases reflect the character typically produced with this key. In GLFW land I currently do not know how to handle a CTRL+A shortcut that follow what users would see on their keyboard.

@ocornut
Copy link
Contributor Author

ocornut commented Mar 14, 2019

I traced back to #114 and dc987ed (2013, @dougbinks)

While I understand what it was trying to solve:

This creates a potential problem in setting up the default keys for an application, such as WASD for an FPS. Whilst on OS X these can be tested as the known physical key W on Windows they cannot.

I think the solution went the wrong way, probably because both the OSX code at the time and the documentation suggested a mapping to physical keys.

Default mapping for games typically want to use scancode to ensure a known physical location, whereas default mapping for interface which have a connection to the actual letter (e.g application shortcuts) want to use keycodes. And keycodes follow the keyboard layout (at least in SDL and Win32 api).

If anything, I think the righter solution should be that scancode be mapped into a GLFW enum (e.g. GLFW_SCANCODE_, analoguous to how SDL has SDL_KEY_ and SDL_SCANCODE_ enums). It might even be possible to reuse the same enum but that makes things weird and more confusing.

While this sounds like a big change on the user, it actually isn't, and we can be considerate that the change in #114 (which made platforms more consistent but imho pushed in a wrong direction) happened in 3.1 which is not so long ago in GLFW versioning terms, so I would suggest considering reverting on that change.

If you think those two posts make sense I can open a clean issue with those two messages and references/links.

@dougbinks
Copy link
Contributor

What @juliettef and I do for shortcuts like CTRL+S etc. is look for a matching translation using glfwGetKeyName() and use that key, or a default key if there is no matching translation. Deciding when/how to do it is somewhat awkward, as users can change language on the fly.

Note that the change in 3.1 aligned all platforms, prior to that there was a difference. Rather than undoing this I would propose if desired an additional callback for translated keys, or a config parameter.

@ocornut
Copy link
Contributor Author

ocornut commented Mar 14, 2019

Note that the change in 3.1 aligned all platforms, prior to that there was a difference. Rather than undoing this I would propose if desired an additional callback for translated keys, or a config parameter.

Thanks! I'll open a new issue to discuss this and potential solutions.
I understand it aligned all platforms but it still feels the other platforms were the wrong ones, as currently key carries essentially the same contents as scancode (except the earlier has an enum and the later is opaque).

I'll try to investigate the other platforms a little bit then will gather this in a new post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Please use the support label instead Windows Win32 specific (not Cygwin or WSL)
Projects
None yet
Development

No branches or pull requests

4 participants