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

Add a shortkey for the keyboard layout manual switching #3859

Merged
merged 10 commits into from
May 20, 2023

Conversation

graph-inc
Copy link

The usual way for switching the keyboard layouts is to ask the platform (i.e., Windows, POSIX) to perform the change and update the xpra keymap when the platform layout switching is detected. This merge request enables the opposite workflow by introducing the next_keyboard_layout(self, update_platform_layout: bool) method, so users can press a platform-independent shortkey and ask the xpra to update its keymap, and optionally inform the platform to switch its keyboard layout.

This patch set requires three CLI options to work independent of the platform settings (of course, except the local support for the chosen keyboard layout). For example, to have "us" and "ir" keyboard layouts, starting by the "us" layout and switching to "ir" and back to the "us" again whenever the "Control+space" shortkey is pressed, following options may be passed:

xpra attach :1234 --keyboard-layout="us" --keyboard-layouts="us,ir" --key-shortcut=Control+space:'next-keyboard-layout(true)'

Usage of the false boolean argument is to avoid race conditions when a shortkey is chosen which is monitored by the platform too. For example, if Alt+Shift+Shift_L is chosen in Windows platform, --key-shortcut=Alt+Shift+Shift_L:'next-keyboard-layout(false)' is appropriate since Windows will switch the layout automatically.
The platform layout switching is only supported in POSIX and Win32 platforms by this patch set.

A side-benefit of this opposite workflow (changing the xpra layout first and updating the platform layout later) is that it is more resilient against the platform layout switching detection bugs and complements the keyboard layout locking feature.

@@ -114,6 +114,10 @@ def parse_shortcuts(strs=(), shortcut_modifiers=(), modifier_names=()):
args.append(None)
elif x.find(".") != -1:
args.append(float(x))
elif x in ("yes", "true", "on"):
args.append(True)
elif x in ("no", "false", "off"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best to use x.lower() in TRUE_OPTIONS / x.lower() in FALSE_OPTIONS

Copy link
Collaborator

@totaam totaam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good PR.

If you can address the minor points in this review, LGTM!

xpra/keyboard/layouts.py Show resolved Hide resolved
xpra/keyboard/layouts.py Show resolved Hide resolved
xpra/platform/posix/keyboard.py Show resolved Hide resolved
# https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-activatekeyboardlayout
# KLF_SETFORPROCESS|KLF_REORDER = 0x108
old_hkl_or_zero_on_failure = ActivateKeyboardLayout(hkl, 0x108)
return old_hkl_or_zero_on_failure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the other set_platform_layout functions return a value.
If anything, they should all return a boolean. (no point in exposing the "old hkl" to callers - they won't know what to do with it)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the return value and just logged the failures because in the "gnome dbus request" scenario, successful/failed response will be determined by a callback and it does not seem good to block the shortkey processing until the callback is invoked.

return layout_to_hkl



Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick: one CR too many!

@graph-inc graph-inc requested a review from totaam May 20, 2023 11:01
@totaam totaam merged commit 5e9b18b into Xpra-org:master May 20, 2023
@totaam
Copy link
Collaborator

totaam commented May 20, 2023

Thanks!

totaam added a commit that referenced this pull request May 29, 2023
@totaam
Copy link
Collaborator

totaam commented May 29, 2023

@graph-inc FYI: a minor fix was needed.

totaam added a commit that referenced this pull request Jul 24, 2024
only the gnome input source bits do
@totaam
Copy link
Collaborator

totaam commented Jul 24, 2024

Also got complaints that this broke the keyboard for users who don't have / want dbus installed, the commit above fixes that.

totaam added a commit that referenced this pull request Jul 24, 2024
only the gnome input source bits do
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.

3 participants