Ignore Super and other modifier keys except for Alt #1059

kzys · 2024-09-13T07:46:51Z

Description

Mozc doesn't know Super and other extra modifier keys except for Alt. For example, "Super + Space" (IBus' default for switching input methods) is recognized as "Space".

This change ignores key events that have these extra modifiers to let others handle the events.

Issue IDs

#853

Steps to test new behaviors (if any)

A clear and concise description about how to verify new behaviors (if any).

OS: Ubuntu 24.04 + Wayland
Steps:
1. Switch to Japanese (Mozc) by pressing Super-Space
2. Switch to English by pressing Super-Space again
3. Confirm the switch (before this PR, a full-width space was inserted instead)

Additional context

Add any other context about the pull request here.

phoepsilonix · 2024-09-13T09:42:39Z

When using ibus-mozc with Japanese input mode enabled and set to full-width hiragana input mode, there is an issue where pressing Super+space inserts a full-width space instead of switching the input source. This PR resolves this issue. I tested it on Manjaro Linux(GNOME/Wayland) and confirmed that it works well.

#854

hiroyuki-komatsu · 2024-09-13T09:57:19Z

Hi kzys,

Thank you for your PR with tests! Overall, it looks good to me.
I left small comments regarding our coding style.

Thank you,

hiroyuki-komatsu · 2024-09-13T09:42:11Z

src/unix/ibus/key_event_handler.cc

@@ -38,6 +38,10 @@ namespace mozc {
 namespace ibus {

 namespace {
+
+// Mod1 is Alt, which is used by Mozc.
+const uint kExtraModMask = IBUS_MOD2_MASK | IBUS_MOD3_MASK | IBUS_MOD4_MASK | IBUS_MOD5_MASK;


Let's use constexper instead of const, and move this into the function.
Also please wrap the line within 80 chars.

// Mod1 is Alt, which is used by Mozc. constexpr uint kExtraModMask = IBUS_MOD2_MASK | IBUS_MOD3_MASK | IBUS_MOD4_MASK | IBUS_MOD5_MASK; if (modifiers & kExtraModMask) { return false; }

hiroyuki-komatsu · 2024-09-13T09:43:01Z

src/unix/ibus/key_event_handler.cc

@@ -62,6 +66,14 @@ bool KeyEventHandler::GetKeyEvent(uint keyval, uint keycode, uint modifiers,
  DCHECK(key);
  key->Clear();

+  // If the key is pressed with these extra modifiers that Mozc doesn't know,
+  // don't pass to the translator.
+  // This is needed to handle Super-Space, IBus's default for switching input methods.


Please wrap the line within 80 chars.

// If the key is pressed with these extra modifiers that Mozc doesn't know, // don't pass to the translator. This is needed to handle Super-Space, IBus's // default for switching input methods. // https://github.com/google/mozc/issues/853

kzys · 2024-09-14T01:06:05Z

Thanks! Addressed in the last revision.

hiroyuki-komatsu · 2024-09-14T15:00:51Z

src/unix/ibus/key_event_handler.cc

@@ -38,6 +38,7 @@ namespace mozc {
 namespace ibus {

 namespace {
+


Please remove this line.

phoepsilonix · 2024-09-14T17:04:15Z

I continued troubleshooting.
It might depend on keyboard layouts, but in my environment, adding IBUS_MOD2_MASK to kExtraModMask prevented the IME from being activated by the half-width/full-width key or the hiragana key.
Removing IBUS_MOD2_MASK from kExtraModMask and using only IBUS_MOD4_MASK, or adding either or both of IBUS_MOD3_MASK and IBUS_MOD5_MASK, allowed the IME to be activated.
Since this might be environment-dependent, I think it might be better to limit it to just IBUS_MOD4_MASK for the Super key to minimize potential impacts.

For your reference.

 * @IBUS_MOD1_MASK: Modifier 1 (Usually Alt_L (0x40),  Alt_R (0x6c),  Meta_L (0xcd)) activated.
 * @IBUS_MOD2_MASK: Modifier 2 (Usually Num_Lock (0x4d)) activated.
 * @IBUS_MOD3_MASK: Modifier 3 activated.
 * @IBUS_MOD4_MASK: Modifier 4 (Usually Super_L (0xce),  Hyper_L (0xcf)) activated.
 * @IBUS_MOD5_MASK: Modifier 5 (ISO_Level3_Shift (0x5c),  Mode_switch (0xcb)) activated.

kzys · 2024-09-15T02:59:52Z

It might depend on keyboard layouts, but in my environment, adding IBUS_MOD2_MASK to kExtraModMask prevented the IME from being activated by the half-width/full-width key or the hiragana key.

Thanks for testing. Let me check. I don't have a Japanese keyboard nearby though.

I don't know how these Japanese-specific keys get Mod2. Is it coming from XKB somehow?

https://github.com/ibus/ibus/blob/fa10c116c2f0dad5026fa6665e9513cd5dcded75/client/wayland/ibuswaylandim.c#L692-L693

kzys · 2024-09-15T03:02:13Z

@phoepsilonix Can you check xmodmap on your machine? I got the below.

% xmodmap 
xmodmap:  up to 4 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x32),  Shift_R (0x3e)
lock        Caps_Lock (0x42)
control     Control_L (0x25),  Control_R (0x69)
mod1        Alt_L (0x40),  Alt_R (0x6c),  Alt_L (0xcc),  Meta_L (0xcd)
mod2        Num_Lock (0x4d)
mod3        ISO_Level5_Shift (0xcb)
mod4        Super_L (0x85),  Super_R (0x86),  Super_L (0xce),  Hyper_L (0xcf)
mod5        ISO_Level3_Shift (0x5c)

phoepsilonix · 2024-09-15T09:00:56Z

@kzys

Hi.
I'm also a bit confused, not knowing what impact numlock actually has. My laptop might be starting to malfunction.😂
The differences are about as follows. Eisu_toglle probably refers to the alphanumeric toggle.(Eisu＝英数(えいすう)＝英語の文字、数字＝alphabet and numeric.)

lock Eisu_toggle (0x42)
lock Caps_Lock (0x42)

xmodmap:  up to 4 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x32),  Shift_R (0x3e)
lock        Eisu_toggle (0x42)
control     Control_L (0x25),  Control_R (0x69)
mod1        Alt_L (0x40),  Alt_R (0x6c),  Alt_L (0xcc),  Meta_L (0xcd)
mod2        Num_Lock (0x4d)
mod3        ISO_Level5_Shift (0xcb)
mod4        Super_L (0x85),  Super_R (0x86),  Super_L (0xce),  Hyper_L (0xcf)
mod5        ISO_Level3_Shift (0x5c)

phoepsilonix · 2024-09-15T09:18:12Z

It seems that the on/off state of Num Lock is the key point of the issue.

I use Num_Lock enabled at startup. I always keep Num_Lock on because I have a numeric keypad. Some keyboards don't have a numeric keypad, and Num_Lock might affect the main keyboard layout. I was wondering if this could be the cause of the issue. So I tested what happens when Num_Lock is turned off.
This is a behavior test of ibus-mozc with IBUS_MOD2_MASK added to kExtraModMask.

When Num_Lock is off, the IME becomes active and Japanese input becomes possible.
It is possible to toggle the IME on and off.
When Num_Lock is on, it seems impossible to toggle the IME on and off.

In other words, the on/off state of Num_Lock is affecting the behavior of the full-width/half-width key and the hiragana key. This issue does not occur with ibus-mozc without adding IBUS_MOD2_MASK to kExtraModMask.

kzys · 2024-09-16T03:30:47Z

@phoepsilonix Thanks! That makes more sense. Updated the mask.

yukawa · 2024-09-17T01:25:08Z

src/unix/ibus/key_event_handler.cc

+  constexpr uint kExtraModMask =
+    IBUS_MOD3_MASK | IBUS_MOD4_MASK | IBUS_MOD5_MASK;


The line 72 should have 4 spaces for the indentation.

constexpr uint kExtraModMask = IBUS_MOD3_MASK | IBUS_MOD4_MASK | IBUS_MOD5_MASK;

I missed that :(

Fixed in the last revision.

Mozc doesn't know Super and other extra modifier keys except for Alt. For example, "Super + Space" (IBus' default for switching input methods) is recognized as "Space". This change ignores key events that have these extra modifiers to let others handle the events. Fixes google#853.

hiroyuki-komatsu · 2024-09-17T16:36:16Z

We have merged your PR.
Thank you for your contribution!

2.30.5595 → 2.30.5618 Conversion * Updated zip code entries (#1063). * Updated word entries (#1068, #1069) Bug fix * Linux: Enabled switching to the DIRECT mode from the menu (#1061) * Linux: Enabled switching modes with Super+Space in Wayland (#853, #1059) * macOS: Fixed the limitation of mouse clicking on the renderer on macOS 15 (120bd93) * Windows: Fixed the setting of Omaha updater for the GoogleJapaneseInput build (#1072) Build * Android: Switched the Bazel rules from WORKSPACE to Bzlmod (#1002) * Android: Updated the Docker build environment * Linux: Update the build environment: Ubuntu 22.04 → 24.04 (#924) * macOS / Windows: Updated the Qt version: 6.7.2 → 6.7.3 (#1065) * Windows: Supported Bazel build (#948) * Windows: Updated the Wix version: 4.0.5 → 5.0.2 (#1070) Code * Performed code refactoring PiperOrigin-RevId: 684295272

kzys force-pushed the ignore-super branch from 787f9be to 61a47cd Compare September 13, 2024 07:49

kzys changed the title ~~Ignore Super and other modifier keys~~ Ignore Super and other modifier keys except for Alt Sep 13, 2024

hiroyuki-komatsu reviewed Sep 13, 2024

View reviewed changes

kzys force-pushed the ignore-super branch from 61a47cd to 4913a3c Compare September 14, 2024 01:05

hiroyuki-komatsu reviewed Sep 14, 2024

View reviewed changes

kzys force-pushed the ignore-super branch from 4913a3c to e55590f Compare September 16, 2024 03:29

yukawa reviewed Sep 17, 2024

View reviewed changes

kzys force-pushed the ignore-super branch from e55590f to f87c73d Compare September 17, 2024 03:54

hiroyuki-komatsu merged commit b52f304 into google:master Sep 17, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore Super and other modifier keys except for Alt #1059

Ignore Super and other modifier keys except for Alt #1059

kzys commented Sep 13, 2024 •

edited

Loading

phoepsilonix commented Sep 13, 2024 •

edited

Loading

hiroyuki-komatsu commented Sep 13, 2024

hiroyuki-komatsu Sep 13, 2024

hiroyuki-komatsu Sep 13, 2024

kzys commented Sep 14, 2024

hiroyuki-komatsu Sep 14, 2024

kzys Sep 15, 2024

phoepsilonix commented Sep 14, 2024

kzys commented Sep 15, 2024 •

edited

Loading

kzys commented Sep 15, 2024

phoepsilonix commented Sep 15, 2024 •

edited

Loading

phoepsilonix commented Sep 15, 2024 •

edited

Loading

kzys commented Sep 16, 2024

yukawa Sep 17, 2024

kzys Sep 17, 2024

hiroyuki-komatsu commented Sep 17, 2024

		@@ -38,6 +38,7 @@ namespace mozc {
		namespace ibus {

		namespace {

		constexpr uint kExtraModMask =
		IBUS_MOD3_MASK \| IBUS_MOD4_MASK \| IBUS_MOD5_MASK;

Ignore Super and other modifier keys except for Alt #1059

Ignore Super and other modifier keys except for Alt #1059

Conversation

kzys commented Sep 13, 2024 • edited Loading

Description

Issue IDs

Steps to test new behaviors (if any)

Additional context

phoepsilonix commented Sep 13, 2024 • edited Loading

hiroyuki-komatsu commented Sep 13, 2024

hiroyuki-komatsu Sep 13, 2024

Choose a reason for hiding this comment

hiroyuki-komatsu Sep 13, 2024

Choose a reason for hiding this comment

kzys commented Sep 14, 2024

hiroyuki-komatsu Sep 14, 2024

Choose a reason for hiding this comment

kzys Sep 15, 2024

Choose a reason for hiding this comment

phoepsilonix commented Sep 14, 2024

kzys commented Sep 15, 2024 • edited Loading

kzys commented Sep 15, 2024

phoepsilonix commented Sep 15, 2024 • edited Loading

phoepsilonix commented Sep 15, 2024 • edited Loading

kzys commented Sep 16, 2024

yukawa Sep 17, 2024

Choose a reason for hiding this comment

kzys Sep 17, 2024

Choose a reason for hiding this comment

hiroyuki-komatsu commented Sep 17, 2024

kzys commented Sep 13, 2024 •

edited

Loading

phoepsilonix commented Sep 13, 2024 •

edited

Loading

kzys commented Sep 15, 2024 •

edited

Loading

phoepsilonix commented Sep 15, 2024 •

edited

Loading

phoepsilonix commented Sep 15, 2024 •

edited

Loading