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

Use generic hot key combo for client control #1375

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

CendioOssman
Copy link
Member

We want to be able to define more keyboard combinations that allows the user to control the client instead of sending the keys to the server. Instead of adding a never ending stream of random keys that just confuses things, we'll define a set of modifiers that are the base for all client control commands.

As a start, this adds hot keys for the menu (replacing menu key), grabbing and ungrabbing the keyboard, and toggling fullscreen.

Fixes #2 and partially #102. We cannot grab the keyboard on macOS in windowed mode with the current mechanism unfortunately.

@CendioOssman
Copy link
Member Author

Annoying. GNOME has a bug that prevents moving the window whilst the keyboard is grabbed:

https://gitlab.gnome.org/GNOME/mutter/-/issues/2021

Unfortunately it's old enough that MATE also has it:

mate-desktop/marco#704

I don't see a way around it, so those users will have to live with having to ungrab to move the window. :/

@CendioOssman
Copy link
Member Author

CendioOssman commented Dec 7, 2021

My first plan was to keep the menu and have it pop up using <Modifiers>+M. However I see that at least PCoIP uses that to minimize the window:

https://www.teradici.com/web-help/TER1706002/3.3/Content/Topics/Using/Changing_window_mode.htm

We'd like people to keep their experiences from other software, and a menu is more uncommon, so perhaps it should have some other hot key.

@CendioOssman
Copy link
Member Author

A major issue here is how to deal with non-Latin keyboards. E.g. Cyrillic or Greek keyboards. I've had a look what other system do:

GTK on Linux

Checks all other XKB groups for matches. So it basically expects a Latin alternative to be configured at all times. Also not that this is on the XKB level, where only four alternatives can be present at all time, not the much larger list that might be configured in GNOME.

If that isn't the case then no keyboard shortcuts work in at least GNOME's applications. It doesn't matter what the display language is, shortcuts are always in Latin.

macOS

Refuses to let you remove the last Latin keyboard from the configuration, meaning one Latin keyboard layout is always present. Keyboard shortcuts are always in Latin and use that layout during lookup (somehow).

Windows

Gladly lets you remove all Latin languages and keyboard layouts. Shortcuts are still in Latin though and still work. Unknown exactly what algorithm it uses at this point.

@michaeltraxler
Copy link

I'm so happy that you put in the work to add this feature!
Thanks a lot!
Would it be possible to also add the "right-ctrl" key to the list of hot-keys?
So, to make them to two keys, what they are, the left and right ctrl-key.

@CendioOssman
Copy link
Member Author

This is already fairly complex, so that is not in the plans, no. :/

Is there some scenario where you think you'll still have conflicts with other hotkeys?

@CendioOssman
Copy link
Member Author

macOS

Refuses to let you remove the last Latin keyboard from the configuration, meaning one Latin keyboard layout is always present. Keyboard shortcuts are always in Latin and use that layout during lookup (somehow).

The magic seems to be that Command+<key> always produces a latin character. This makes sense given that Command is pretty much always used as a modifier for shortcuts on macOS.

This is a bit obscured during normal use of VNC as we only respect Shift, CapsLock and Alt (really AltGr) when generating keys for the server.

Also note #1396 which currently prevents non-Latin keyboard layouts anyway.

@michaeltraxler
Copy link

Is there some scenario where you think you'll still have conflicts with other hotkeys?

Yes, so many key-combinations are already used. ctrl-alt is "quite" common, so used by by some applications. With Alt+Win I can live as this is not used up to now by any known application. Not so easy to press, but humans are adaptable. Even better would be also to support "AltGr", instead of only "Alt". The same argument as for left and right "ctrl". The more options, the better, as everyone has a different keyboard and wants to adapt the hotkey to the specific keyboard he/she is using. But still I would prefer to use the ctrl_right key, as remmina is using for the grab key. But sure, it might be that others really use this ctrl_right key for their normal work :-) But it is really great that you give so many options to the user already with the current version! A great way to go!

@CendioOssman
Copy link
Member Author

Windows

Gladly lets you remove all Latin languages and keyboard layouts. Shortcuts are still in Latin though and still work. Unknown exactly what algorithm it uses at this point.

My guess is that applications focus on virtual-key codes. If I use MapVirtualKey() to map a scan code to a virtual key, then that virtual key to a character, then I always get a Latin character, even if the keyboard layout is Greek or a Cyrillic one.

The behavior seem to match what I'm seeing from other applications, so I'll go with that.

@CendioOssman
Copy link
Member Author

My first plan was to keep the menu and have it pop up using +M. However I see that at least PCoIP uses that to minimize the window:

https://www.teradici.com/web-help/TER1706002/3.3/Content/Topics/Using/Changing_window_mode.htm

We'd like people to keep their experiences from other software, and a menu is more uncommon, so perhaps it should have some other hot key.

Counter point: Most, if not all, systems have a shortcut for this already. E.g. Win+H in GNOME, Win+Down in Windows, and Cmd+M in macOS. As such I don't think we need to bother with duplicating functionality.

We added minimize to the context menu to solve #1070. However in that case the scenario was full screen, and likely keyboard grab was also active. That meant the system shortcuts didn't work. With this PR there will be a keyboard shortcut to release the keyboard, and then you can use the system shortcut. E.g. Ctrl+Alt followed by Win+Down on Windows.

We could still move the menu key to something else to avoid confusion, but I don't think we should add a shortcut for minimize.

To be able to debug exactly what is wrong with the layout. Unfortunately
we don't know what log level is used for actual "invalid layout"
message, or if it is even logged as all, since that happens elsewhere.
So let's be cautious and use a debug log level here.
This is a general thing so move it in to the core library instead,
letting vncviewer focus on just translation of system events to VNC
ones.
Encapsulate all the platform specific magic around keyboard in to
specific classes, in order to keep the core code more readable.
This make sure we have any new defines, in case we want to use them.
This is mainly a copy of XKeysymToString() from libX11. We've also added
a wrapper that still gives a string, even if there is no name for the
requested keysym.

This grows the binaries a bit, but not with any extreme amount so is
hopefully worth it to get better debug logging.
The fake ones have a special mode, so we can simply filter them before
they are passed on as FLTK events.
We want to be able to define more keyboard combinations that allows the
user to control the client instead of sending the keys to the server.
Instead of adding a never ending stream of random keys that just
confuses things, we'll define a set of modifiers that are the base for
all client control commands.
Allows you to quickly enter and leave full-screen mode.
No matter how carefully you choose your hot key modifiers, there might
still be situations where you need to send those hot key combinations to
the server instead.

This commit adds a method for this by letting the <hotkeys>+Space
combination temporarily bypass the hot key logic and send everything to
the server (until all keys are released again).
Allows you to grab the keyboard input from the desktop environment even
in windowed mode.
We want to show tips for more things, so rework the overlay
infrastructure so it can handle an arbitrary amount of messages.
These modes can be non-obvious for users how to get out of, so show an
overlay with how to get back to normal when these modes are enabled.
It can get a bit annoying if you keep getting these constantly, so only
show them again if enough time has passed since we last showed them.
Let's add an ellipsis if we have to crop the title.

Also reduce the maximum length to something reasonable. We want to do
the cropping, not let the window manager do it, as otherwise "TigerVNC"
might get cropped.
Provide some UI feedback that the keyboard is currently grabbed and
local input is prevented.
Make this less complex by removing the timer and retrying things
synchronously. This can freeze the UI, but we give up after half a
second so it should hopefully not be noticable.

The advantage is that we can directly determine if we succeeded or
failed grabbing the keyboard. The previous code could in theory continue
retrying forever.
It is now very likely that this happens as we will not have permission
to do this by default on macOS. So present a message to the user so they
understand why the keyboard isn't working as expected.
The system steals keyboard events for certain system keyboard shortcuts,
e.g. Cmd+Tab. Unfortunately this isn't considered a focus loss, so we
don't realise we've lost a few keyboard events and can end up in a
confused state.

Fortunately it is possible to detect when this happens and reset the
keyboard state, just like we do when focus is lost.
These are the slightly more proper way to grab the keyboard and macOS
and is what similar applications do. It avoids a lot of issues we have,
e.g., problems with multiple monitors.

Unfortunately we need to have the user explicitly approve this (which
really is a good thing, security wise), and Apple have chosen to mark
this feature as only for accessibility.
@rudironsonijr
Copy link
Contributor

@CendioOssman, this refactoring makes a lot of sense. Do you intend to move it forward?
I can help!

@CendioOssman
Copy link
Member Author

At some point. But at the moment there are other projects ahead of this on my todo.

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.

fast access (hotkey) to grab_keyboard()
3 participants