handle physical keyboard key states#52
Conversation
arm64-v8a comparisonNo difference. x86_64 comparisonNo difference. |
compatibleSdkVersion: 6.0.0(20) targetSdkVersion: 6.1.1(24)
📝 WalkthroughWalkthroughThis PR adds key state bitmask support to the input event pipeline. The changes thread a ChangesKey State Bitmask Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)entry/src/main/cpp/napi_init.cppentry/src/main/cpp/napi_init.cpp:3:10: fatal error: 'fcitx.h' file not found ... [truncated 2200 characters] ... Called from ClangFrontend__CTrans.CTrans_funct.instruction in file "src/clang/cTrans.ml" (inlined), line 4765, characters 38-71 entry/src/main/cpp/src/keycode.cppIn file included from entry/src/main/cpp/src/keycode.cpp:3: ... [truncated 2200 characters] ... ontend_decl.CFrontend_decl_funct.add_method in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 entry/src/main/cpp/src/fcitx.cppIn file included from entry/src/main/cpp/src/fcitx.cpp:1: ... [truncated 2200 characters] ... ontend_decl_funct.add_method in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52 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: 1
🧹 Nitpick comments (1)
entry/src/main/ets/InputMethodExtensionAbility/model/KeyboardController.ets (1)
403-403: ⚡ Quick winUse strict equality operator.
Replace
!=with!==for type-safe comparison.🔧 Proposed fix
- if (e.key.code != KeyCode.KEYCODE_UNKNOWN) { + if (e.key.code !== KeyCode.KEYCODE_UNKNOWN) {🤖 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 `@entry/src/main/ets/InputMethodExtensionAbility/model/KeyboardController.ets` at line 403, In KeyboardController (the conditional checking key codes), replace the non-strict comparison in the if statement that tests e.key.code against KeyCode.KEYCODE_UNKNOWN by using the strict equality operator (===) or strict inequality (!==) as appropriate; locate the expression "e.key.code != KeyCode.KEYCODE_UNKNOWN" and change it to "e.key.code !== KeyCode.KEYCODE_UNKNOWN" to ensure type-safe comparison in the code that handles key events.
🤖 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 `@build-profile.json5`:
- Around line 8-9: The build-profile.json5 change raises compatibleSdkVersion to
"6.0.0(20)" but there’s no rationale tying that SDK bump to the keyboard code
which listens to keyboardDelegate.on('keyEvent', ...) and computes modifier
state via statesFromKeyEvent; either add a concise justification in the PR or in
build-profile.json5 explaining exactly which HarmonyOS API(s) (e.g.,
KeyEvent.getModifierKeyState or specific properties) require 6.0.0(20) and cite
the minimal API level, or reduce compatibleSdkVersion to the lowest SDK that
actually supports the used modifier APIs after verifying support for
KeyEvent.getModifierKeyState (API 12+) and any isShiftDown/isCtrlDown
equivalents in your statesFromKeyEvent implementation.
---
Nitpick comments:
In `@entry/src/main/ets/InputMethodExtensionAbility/model/KeyboardController.ets`:
- Line 403: In KeyboardController (the conditional checking key codes), replace
the non-strict comparison in the if statement that tests e.key.code against
KeyCode.KEYCODE_UNKNOWN by using the strict equality operator (===) or strict
inequality (!==) as appropriate; locate the expression "e.key.code !=
KeyCode.KEYCODE_UNKNOWN" and change it to "e.key.code !==
KeyCode.KEYCODE_UNKNOWN" to ensure type-safe comparison in the code that handles
key events.
🪄 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: f2ea28ea-aa07-478e-953c-0390a07bc9da
📒 Files selected for processing (9)
build-profile.json5entry/src/main/cpp/napi_init.cppentry/src/main/cpp/src/fcitx.cppentry/src/main/cpp/src/fcitx.hentry/src/main/cpp/src/keycode.cppentry/src/main/cpp/src/keycode.hentry/src/main/cpp/types/libentry/Index.d.tsentry/src/main/ets/InputMethodExtensionAbility/model/KeyState.etsentry/src/main/ets/InputMethodExtensionAbility/model/KeyboardController.ets
| "compatibleSdkVersion": "6.0.0(20)", | ||
| "targetSdkVersion": "6.1.1(24)", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for SDK version requirement documentation in the repository
# Check for any documentation about SDK version requirements
rg -i "sdk.*(version|6\.0|6\.1|requirement)" --type md --type txt -C 2
# Check for KeyEvent API usage that might require specific SDK versions
rg -n "KeyEvent|keyEvent|isShiftDown|isCtrlDown" entry/src/main/ets/ -C 2Repository: fcitx-contrib/fcitx5-harmony
Length of output: 4258
🌐 Web query:
Do HarmonyOS SDK 6.0.0(20) and 6.1.1(24) introduce new keyboard event APIs such as KeyEvent modifier properties (isShiftDown, isCtrlDown, isAltDown, isMetaDown) or the keyEvent listener that are used in fcitx5-harmony?
💡 Result:
HarmonyOS SDK 6.0.0(20) and 6.1.1(24) do not introduce specific property-based modifier APIs like isShiftDown, isCtrlDown, isAltDown, or isMetaDown directly on the KeyEvent object [1][2][3]. Instead, these SDK versions utilize the getModifierKeyState(keys: Array<string>): boolean method, which was introduced in API version 12 and is available in HarmonyOS 6.x [1][2][4]. Key takeaways regarding your query: 1. Modifier Key Handling: Rather than individual boolean properties, developers should use event.getModifierKeyState(['Ctrl', 'Shift', 'Alt', 'Fn']) to check the pressed status of modifier keys [1][2][4]. This method provides a centralized and cleaner way to handle combinations compared to checking individual flags [4]. 2. Input Listener: The onKeyEvent listener is the standard mechanism for handling physical keyboard events in ArkTS [5][6]. Additionally, in HarmonyOS 6, the onKeyPreIme callback is available to intercept key events before they reach the input method framework, providing more control for custom keyboard or text-processing applications [4][7]. 3. API Compatibility: The getModifierKeyState API is supported from API version 12 onwards, making it fully available in the HarmonyOS 6.0.0(20) and 6.1.1(24) SDKs [1][2][4]. For fcitx5-harmony or similar input method implementations, the InputMethodEngine also supports subscribing to keyEvent via getKeyboardDelegate().on('keyEvent', callback), which provides the KeyEvent object containing these interaction capabilities [8][9].
Citations:
- 1: https://gitcode.com/openharmony/docs/blob/OpenHarmony-5.1.0-Release/en/application-dev/reference/apis-arkui/arkui-ts/ts-universal-events-key.md
- 2: https://www.seaxiang.com/blog/i3kYZl
- 3: https://gitcode.com/openharmony/docs/blob/OpenHarmony-5.0.2-Release/en/application-dev/reference/apis-arkui/arkui-ts/ts-universal-events-key.md
- 4: https://segmentfault.com/a/1190000047702975
- 5: https://m.w3cschool.cn/tchmsc/tchmsc-51ya3xaq.html
- 6: https://dev.to/moyantianwang/harmonyos-arkts-keyboard-mouse-event-development-guide-4e1m
- 7: https://github.com/openharmony/docs/blob/master/en/application-dev/ui/arkts-interaction-development-guide-keyboard.md
- 8: https://gitcode.com/openharmony/docs/blob/OpenHarmony-6.0-Release/en/application-dev/reference/apis-ime-kit/js-apis-inputmethodengine.md
- 9: https://developer.huawei.com/consumer/cn/doc/harmonyos-references/js-apis-inputmethodengine
Document/justify compatibleSdkVersion: "6.0.0(20)" for HarmonyOS keyboard keyEvent handling
build-profile.json5 bumps the minimum to compatibleSdkVersion: "6.0.0(20)" (while targetSdkVersion is "6.1.1(24)"). The keyboard code listens to keyboardDelegate.on('keyEvent', ...) and computes modifier state from KeyEvent (e.g., e.altKey via statesFromKeyEvent). There’s no accompanying documentation/rationale in the repo tying 6.0.0(20) (or 6.x generally) specifically to the APIs used, and HarmonyOS docs describe modifier handling via KeyEvent.getModifierKeyState(...) (API 12+) rather than isShiftDown/isCtrlDown-style properties—so this SDK bump isn’t clearly justified by the modifier approach.
Add a short rationale in the PR/file (or lower compatibleSdkVersion if the used keyEvent + modifier APIs are available on earlier SDK levels).
🤖 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 `@build-profile.json5` around lines 8 - 9, The build-profile.json5 change
raises compatibleSdkVersion to "6.0.0(20)" but there’s no rationale tying that
SDK bump to the keyboard code which listens to keyboardDelegate.on('keyEvent',
...) and computes modifier state via statesFromKeyEvent; either add a concise
justification in the PR or in build-profile.json5 explaining exactly which
HarmonyOS API(s) (e.g., KeyEvent.getModifierKeyState or specific properties)
require 6.0.0(20) and cite the minimal API level, or reduce compatibleSdkVersion
to the lowest SDK that actually supports the used modifier APIs after verifying
support for KeyEvent.getModifierKeyState (API 12+) and any
isShiftDown/isCtrlDown equivalents in your statesFromKeyEvent implementation.
There was a problem hiding this comment.
Our CI SDK is outdated so this fails. Is it needed for your changes?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the new KeyState.ets file for specific KeyEvent properties used
cat entry/src/main/ets/InputMethodExtensionAbility/model/KeyState.ets
echo "---"
# Check KeyboardController.ets for the physical key event handler
cat entry/src/main/ets/InputMethodExtensionAbility/model/KeyboardController.etsLength of output: 18370
🌐 Web query:
HarmonyOS InputKit KeyEvent unicodeChar numLock logoKey API level introduced version
Length of output: 11227
🌐 Web query:
HarmonyOS inputMethodEngine KeyboardDelegate on keyEvent API level version introduced js-apis-inputmethodengine
Length of output: 19060
@eagleoflqj, based on the docs research, the SDK bump does not appear to be required for the key state handling changes in this PR:
keyboardDelegate.on('keyEvent', ...)is marked API 10+- All
InputKitKeyEventproperties used instatesFromKeyEvent(altKey,ctrlKey,shiftKey,capsLock,numLock,logoKey) and inphysicalKeyEventHandler(unicodeChar,action,key.code) are available since API 9
The old compatibleSdkVersion: "5.0.1(13)" (API level 13) already covers all of these. The build-profile.json5 change can safely be reverted independently of the keyboard key state changes, which should unblock your CI.
| "compatibleSdkVersion": "6.0.0(20)", | ||
| "targetSdkVersion": "6.1.1(24)", |
There was a problem hiding this comment.
Our CI SDK is outdated so this fails. Is it needed for your changes?
2 participants
Summary by CodeRabbit