Add option to consume redundant Activate/Deactivate key press - #1616
Add option to consume redundant Activate/Deactivate key press#1616akovalenko wants to merge 3 commits into
Conversation
Dedicated ActivateKeys/DeactivateKeys act as modeless switches: pressing the activate key when the input method is already active is expected to be a no-op. Currently such a press falls through the state-gated hotkey handlers (their check() is canActivate/canDeactivate) and leaks to the client, typing the letter into the application. Add a Hotkey/ConsumeRedundantActivateKeys boolean option, default false to preserve current behavior. When enabled, a non-modifier press of an activate/deactivate key that no handler claimed is consumed after the handler loop instead of being forwarded. Modifier-only keys are left alone, so a bare modifier press still reaches the client; a key shared between both lists (toggle setup) always matches a handler earlier and is unaffected. Related: fcitx#845. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a configurable ChangesRedundant activation key consumption
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant HotkeyConfig
participant GlobalConfig
participant InstancePrivate
HotkeyConfig->>GlobalConfig: configure consumeRedundantActivateKeys
InstancePrivate->>GlobalConfig: read configured value
GlobalConfig-->>InstancePrivate: return configured value
InstancePrivate->>InstancePrivate: allow redundant activation or deactivation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/fcitx/instance.cpp (1)
876-888: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for the option branches.
Cover the default-off behavior, redundant activate/deactivate consumption, non-redundant state changes, modifier-only forwarding, and shared-list ordering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/fcitx/instance.cpp` around lines 876 - 888, Add regression tests for the activate/deactivate handling around the consumeRedundantActivateKeys branch in instance.cpp. Cover the option disabled path, consumption of redundant activate and deactivate keys, forwarding of non-redundant state changes and modifier-only keys, and ordering when activate/deactivate keys share a list; preserve the expected client forwarding and state transitions for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/fcitx/instance.cpp`:
- Around line 876-888: Update the redundant activate/deactivate handling around
globalConfig_.consumeRedundantActivateKeys() to record consumed presses and
consume their matching release events as well. Ensure the release path runs even
when no input-method engine or entry is available, preventing the key-up from
reaching the client; preserve the existing keyEvent.filterAndAccept() behavior
for the press.
- Around line 876-888: Move the redundant activate/deactivate key consumption
currently in the Instance event dispatcher into an addon or module, leaving
Instance free of this feature-specific special case. Preserve the existing
consumeRedundantActivateKeys, modifier, canTrigger, activate/deactivate key
matching, and filterAndAccept behavior through the new runtime extension; only
retain it in Instance if an approved exception is documented.
---
Nitpick comments:
In `@src/lib/fcitx/instance.cpp`:
- Around line 876-888: Add regression tests for the activate/deactivate handling
around the consumeRedundantActivateKeys branch in instance.cpp. Cover the option
disabled path, consumption of redundant activate and deactivate keys, forwarding
of non-redundant state changes and modifier-only keys, and ordering when
activate/deactivate keys share a list; preserve the expected client forwarding
and state transitions for each case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 489e5ad9-acfa-4c83-8b0a-e68992f6beaf
📒 Files selected for processing (3)
src/lib/fcitx/globalconfig.cppsrc/lib/fcitx/globalconfig.hsrc/lib/fcitx/instance.cpp
| if (d->globalConfig_.consumeRedundantActivateKeys() && | ||
| !isModifier && canTrigger() && | ||
| (origKey.keyListIndex(d->globalConfig_.activateKeys()) >= | ||
| 0 || | ||
| origKey.keyListIndex(d->globalConfig_.deactivateKeys()) >= | ||
| 0)) { | ||
| // Activate/Deactivate keys act as modeless switches: when | ||
| // the state already matches, the state-gated handlers | ||
| // above do not claim the key. Consume it instead of | ||
| // leaking it to the client. | ||
| keyEvent.filterAndAccept(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Consume the matching release as well.
The normal hotkey path records keyReleased_ before consuming a press; this fallback does not. The redundant key’s release can therefore bypass the early release filter and reach the client when no input-method engine or entry is available, leaving an unmatched key-up. Track the consumed press or add an equivalent release path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/fcitx/instance.cpp` around lines 876 - 888, Update the redundant
activate/deactivate handling around globalConfig_.consumeRedundantActivateKeys()
to record consumed presses and consume their matching release events as well.
Ensure the release path runs even when no input-method engine or entry is
available, preventing the key-up from reaching the client; preserve the existing
keyEvent.filterAndAccept() behavior for the press.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move this runtime behavior behind an addon or module.
This adds feature-specific behavior directly to the server-side Instance event dispatcher. Please avoid another special case in Instance and implement the consumption through an addon/module, or document an approved exception.
As per coding guidelines, src/**/*.cpp must “Implement new runtime behavior as an addon or module rather than adding special cases to the server binary.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/fcitx/instance.cpp` around lines 876 - 888, Move the redundant
activate/deactivate key consumption currently in the Instance event dispatcher
into an addon or module, leaving Instance free of this feature-specific special
case. Preserve the existing consumeRedundantActivateKeys, modifier, canTrigger,
activate/deactivate key matching, and filterAndAccept behavior through the new
runtime extension; only retain it in Instance if an approved exception is
documented.
Source: Coding guidelines
|
I think there the code can be much simpler: change canActivate / canDeactivate's return value to be inputState->isActive() || your new option. |
Do not special-case into filterAndAccept when the option is on and the key is not a modifier etc. Pretend instead, when the option is on, that we always canActivate and canDeactivate.
Thank you, tried your approach, much simpler indeed. Works for me for several days. PR updated. |
2 participants
The current behavior — an Activate/Deactivate key press that doesn't
change state falls through to the application — is clearly intentional
and fine for most setups (as discussed in #845), so this PR changes no
defaults.
My use case is a modeless switch: dedicated
ActivateKeys=Super+N/DeactivateKeys=Super+S, picked to be idempotent — a redundant pressshould be a no-op. Instead it leaks into the client and types a letter:
with the IM already active, Super+N inserts "n" (easy to reproduce with
two input methods in a group).
This adds an opt-in Hotkey option "Consume redundant Activate/Deactivate
key" (
ConsumeRedundantActivateKeys, default false). When enabled, anon-modifier press of an activate/deactivate key that no state-gated
handler claimed is consumed after the handler loop instead of being
forwarded. Modifier-only keys are never consumed (a bare modifier press
still reaches the client), and a key present in both lists (toggle
setup) matches a handler earlier and is unaffected.
Summary by CodeRabbit
New Features
Bug Fixes