Fix sticky keys for thumb shift mode#45
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces modifier state and command memory tracking (_MS and _CM) along with a new cmd_term function to handle key release events in both the Python 2 and Python 3 engines. However, the review feedback correctly points out that this entire implementation is redundant and introduces dead code, as key release shortcuts are never registered in the key bindings. Furthermore, the use of eval() on the key representation presents a security risk and performance inefficiency. It is recommended to revert cmd_exec to its original state, remove the redundant cmd_term logic, and simplify the key release handling by directly checking if the preedit string is empty.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| self._RSS = 0 | ||
| self._MS = 0 # modifier state | ||
| self._CM = 0 # command memory |
There was a problem hiding this comment.
The state variables _MS and _CM are not needed because the cmd_term logic is redundant. We can safely remove them to keep the state clean.
| self._RSS = 0 | |
| self._MS = 0 # modifier state | |
| self._CM = 0 # command memory | |
| self._RSS = 0 |
| def cmd_exec(keyval, state=0): | ||
| key = self._mk_key(keyval, state) | ||
| pair = eval(key) | ||
| self._MS = pair[0] | ||
| self._CM = pair[1] | ||
| return __cmd_exec(key, keyval, state) | ||
|
|
||
| def cmd_term(keyval, state=0): | ||
| if self._MS == 0 and self._CM == 0: | ||
| return False | ||
|
|
||
| key = self._mk_key(keyval, state) | ||
| pair = eval(key) | ||
|
|
||
| prev_keyval = self._CM | IBus.ModifierType.RELEASE_MASK | ||
| prev_state = self._MS | IBus.ModifierType.RELEASE_MASK | ||
| prev_key = repr([int(prev_state), int(prev_keyval)]) | ||
|
|
||
| self._MS = 0 | ||
| self._CM = 0 | ||
|
|
||
| __cmd_exec(prev_key, prev_keyval, prev_state) | ||
| return __cmd_exec(key, pair[1], pair[0]) | ||
|
|
There was a problem hiding this comment.
The cmd_term function and the associated tracking of self._MS and self._CM are completely redundant.\n\n### Why this is redundant:\n1. self.__keybind is populated by _mk_keybind(), which uses _s_to_key(s) to generate the keys.\n2. _s_to_key(s) only extracts standard modifiers (SHIFT_MASK, CONTROL_MASK, MOD1_MASK) and never includes RELEASE_MASK.\n3. Therefore, there are no key release shortcuts registered in self.__keybind.\n4. In cmd_term, prev_key and key are constructed with RELEASE_MASK.\n5. Since no key release shortcuts exist in self.__keybind, __cmd_exec with these keys will always find an empty list of commands and return False.\n6. Consequently, cmd_term will always return False, making the entire tracking of self._MS and self._CM dead code.\n7. Furthermore, using eval(key) is a security risk and highly inefficient.\n\nWe can safely revert cmd_exec to its original implementation and completely remove cmd_term and __cmd_exec.
| def cmd_exec(keyval, state=0): | |
| key = self._mk_key(keyval, state) | |
| pair = eval(key) | |
| self._MS = pair[0] | |
| self._CM = pair[1] | |
| return __cmd_exec(key, keyval, state) | |
| def cmd_term(keyval, state=0): | |
| if self._MS == 0 and self._CM == 0: | |
| return False | |
| key = self._mk_key(keyval, state) | |
| pair = eval(key) | |
| prev_keyval = self._CM | IBus.ModifierType.RELEASE_MASK | |
| prev_state = self._MS | IBus.ModifierType.RELEASE_MASK | |
| prev_key = repr([int(prev_state), int(prev_keyval)]) | |
| self._MS = 0 | |
| self._CM = 0 | |
| __cmd_exec(prev_key, prev_keyval, prev_state) | |
| return __cmd_exec(key, pair[1], pair[0]) | |
| def cmd_exec(keyval, state=0): | |
| key = self._mk_key(keyval, state) | |
| for cmd in self.__keybind.get(key, []): | |
| if config.DEBUG: | |
| print('cmd =', cmd) | |
| try: | |
| if getattr(self, cmd)(keyval, state): | |
| return True | |
| except Exception as err: | |
| printerr('Error command: %s: %s' % (cmd, str(err))) | |
| return False |
| elif keyval not in self.__thumb.get_chars() or state != 0: | ||
| if cmd_term(keyval, state): | ||
| return True | ||
| elif not self.__preedit_ja_string.is_empty(): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Since cmd_term is redundant and always returns False, we can simplify this block to directly check if the preedit is empty. If it is empty, we return False to pass the release event to the application, which fixes the sticky keys bug.
elif keyval not in self.__thumb.get_chars() or state != 0:
if not self.__preedit_ja_string.is_empty():
return True
return False| self._RSS = 0 | ||
| self._MS = 0 # modifier state | ||
| self._CM = 0 # command memory |
There was a problem hiding this comment.
The state variables _MS and _CM are not needed because the cmd_term logic is redundant. We can safely remove them to keep the state clean.
self._RSS = 0| def cmd_exec(keyval, state=0): | ||
| key = self._mk_key(keyval, state) | ||
| pair = eval(key) | ||
| self._MS = pair[0] | ||
| self._CM = pair[1] | ||
| return __cmd_exec(key, keyval, state) | ||
|
|
||
| def cmd_term(keyval, state=0): | ||
| if self._MS == 0 and self._CM == 0: | ||
| return False | ||
|
|
||
| key = self._mk_key(keyval, state) | ||
| pair = eval(key) | ||
|
|
||
| prev_keyval = self._CM | IBus.ModifierType.RELEASE_MASK | ||
| prev_state = self._MS | IBus.ModifierType.RELEASE_MASK | ||
| prev_key = repr([int(prev_state), int(prev_keyval)]) | ||
|
|
||
| self._MS = 0 | ||
| self._CM = 0 | ||
|
|
||
| __cmd_exec(prev_key, prev_keyval, prev_state) | ||
| return __cmd_exec(key, pair[1], pair[0]) |
There was a problem hiding this comment.
The cmd_term function and the associated tracking of self._MS and self._CM are completely redundant.\n\n### Why this is redundant:\n1. self.__keybind is populated by _mk_keybind(), which uses _s_to_key(s) to generate the keys.\n2. _s_to_key(s) only extracts standard modifiers (SHIFT_MASK, CONTROL_MASK, MOD1_MASK) and never includes RELEASE_MASK.\n3. Therefore, there are no key release shortcuts registered in self.__keybind.\n4. In cmd_term, prev_key and key are constructed with RELEASE_MASK.\n5. Since no key release shortcuts exist in self.__keybind, __cmd_exec with these keys will always find an empty list of commands and return False.\n6. Consequently, cmd_term will always return False, making the entire tracking of self._MS and self._CM dead code.\n7. Furthermore, using eval(key) is a security risk and highly inefficient.\n\nWe can safely revert cmd_exec to its original implementation and completely remove cmd_term and __cmd_exec.
def cmd_exec(keyval, state=0):
key = self._mk_key(keyval, state)
for cmd in self.__keybind.get(key, []):
if config.DEBUG:
print 'cmd =', cmd
try:
if getattr(self, cmd)(keyval, state):
return True
except Exception as err:
printerr('Error command: %s: %s' % (cmd, str(err)))
return False| elif keyval not in self.__thumb.get_chars() or state != 0: | ||
| if cmd_term(keyval, state): | ||
| return True | ||
| elif not self.__preedit_ja_string.is_empty(): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Since cmd_term is redundant and always returns False, we can simplify this block to directly check if the preedit is empty. If it is empty, we return False to pass the release event to the application, which fixes the sticky keys bug.
elif keyval not in self.__thumb.get_chars() or state != 0:
if not self.__preedit_ja_string.is_empty():
return True
return False2 participants
Symptom
Given that:
When the user releases the key
Then the user sees that the key is being pressed forever (until esc is pressed or the user switches away from Anthy)
Expected
Given that:
When the user releases the key
Then the user sees that the key press does not automatically repeat itself in the text field
Root Cause
For keys that are neither thumb shift keys nor text keys, only the key press is processed. The key release is ignored by
__process_key_event_thumb().For illustration, please compare the if statements between the pressed handling and the released handling in engine/python3/engine.py.
Key Release:
Key Press:
Hopefully this comparison is clear enough to show that
cmd_exec()does not have a corresponding key release action.Solution
Steps:
a. fire release for the previous key press (if present)
b. reset the key press storage
c. hand the current key release back to IBus
Remarks
Please feel free to let me know about any practice that I should follow for this project.