Repository navigation
fix(input): preserve fullwidth IME text identity (#458) - #467
Merged
Merged
Conversation
Fullwidth punctuation committed by fcitx5 was transported as an untyped integer and then interpreted as an X11 keysym. U+FF0C consequently became the undefined key-65292 event. Other Unicode scalars overlap named keysyms, including Backspace, Tab, Return, Escape, F13, and Redo, so recognizing unnamed registry holes alone would leave those collisions unresolved. Introduce FrontendKey::Character(char) and FrontendKey::Keysym(u32) and preserve that distinction from frontend input through the core keyboard boundary. IME commits, keyboard committed text, logical character keys, and crossterm characters use the character variant. Genuine key identities retain existing keysym naming and function-key modifier handling. Keep raw control aliases and Unix TTY byte decoding intact, restrict scroll observation to keysyms, and convert numeric WebView input only at its API boundary. Rename the raw toolkit helper to translate_keysym. Follow GNU Emacs's separation of decoded IME text from function-key events. Document the GNU source investigation, primary-source research, typed transport design, and reproduction commands. Fix private Xvfb isolation in neomacs-infra by overriding inherited Wayland connection settings and pinning GTK to X11. The native fcitx regression owns its configuration, runtime directory, and D-Bus session and verifies the editor's PID and focus on the private display before sending input. Add regression coverage for the original comma failure, character/keysym collisions, logical and crossterm input, GNU Lisp event parity, paired GNU and Neomacs TUI insertion, and real fcitx commits alongside a physical F13 binding. Preserve final redisplay snapshots and capture later GUI frames. Fixes #458
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (24)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It reworks the keyboard transport boundary across many crates and relies on platform-specific X11/Wayland/fcitx IME behavior that cannot be fully validated in this environment, so final human review is warranted.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR fixes issue #458, where fullwidth/halfwidth punctuation committed by an input method (e.g. fcitx5 on X11) was lost because the character's Unicode scalar was transported as a bare u32 and then re-interpreted by the X11 keysym classifier (so ， U+FF0C became <key-65292>, and U+FF08/FF09/FF0D/FF1B/FFCA collided with Backspace/Tab/Return/Escape/F13). The fix introduces a closed FrontendKey { Character(char), Keysym(u32) } sum type that preserves the toolkit's text-vs-key provenance end-to-end through the GUI IME/text path, logical-key fallback, crossterm, and the input bridge, so committed text self-inserts while physical function keys keep their bindings.
Changes:
- Replace the
keysym: u32field ofInputEvent::Keywithkey: FrontendKey, and cookCharacterwith the ordinary modifier policy (even when the code point overlaps a function-key band) whileKeysymretains existing GNU key naming. - Split
translate_keyintotranslate_key_input(text-aware, returnsOption<FrontendKey>) andtranslate_keysym; thread the typed key through all producers/consumers; convert to the WebView's numeric contract only at that boundary. - Harden Xvfb isolation (empty
WAYLAND_DISPLAY/WAYLAND_SOCKET,GDK_BACKEND=x11) and add core, oracle, TUI, and real-fcitx GUI regressions plus design/research docs.
| File | Description |
|---|---|
| crates/neovm-core/src/keyboard.rs | Adds FrontendKey enum; render_key_transport_to_input_event now dispatches Character→FrontendCharacterInput vs Keysym→classifier |
| crates/neovm-core/src/keyboard/keysym.rs | Doc clarifies range classification applies to keysym input, not committed text |
| crates/neomacs-display-runtime/src/render_thread/input.rs | Splits translate_key into translate_key_input/translate_keysym; translate_committed_text returns Vec<FrontendKey> |
| crates/neomacs-display-runtime/src/render_thread/window_events.rs | Routes IME/text/logical keys as typed FrontendKey; character modifiers use Ordinary policy; WebView numeric conversion at boundary |
| crates/neomacs-display-runtime/src/tty_input.rs | Crossterm mapping emits FrontendKey::Character for text, Keysym for named keys |
| crates/neomacs-display-runtime/src/thread_comm.rs | InputEvent::Key.key: FrontendKey; updates latency-match sites |
| crates/neomacs/src/input_bridge.rs | Forwards key and updates debug logging |
| crates/neomacs/src/bin/mock-display.rs | Quit/Escape detection matches on FrontendKey variants |
| crates/neomacs-infra/src/display.rs | Xvfb session clears inherited Wayland env and pins GDK_BACKEND=x11 |
| crates/neomacs-gui-tests/tests/native_frame_focus/ime.rs | New opt-in real-fcitx5 regression (text vs F13 key) |
| crates/neomacs-gui-tests/tests/native_frame_focus.rs | Registers the ime module |
| crates/neomacs-gui-tests/tests/harness_contract.rs | Asserts the new Xvfb env isolation facts |
| crates/neomacs-gui-tests/fixtures/issue-458-ime.el, issue-458-fcitx-profile | Fixtures for the fcitx scenario |
| crates/neomacs-tui-tests/tests/editing.rs | TUI regression: fullwidth text self-inserts like GNU |
| crates/neovm-oracle-tests/src/kbd_event_advanced.rs | GNU oracle parity for fullwidth character identity |
| crates/neovm-core/src/keyboard/tests/mod.rs, .../render_thread/**, .../thread_comm/tests, .../tty_input/tests, input_bridge/tests | Tests updated to the typed transport, plus new collision coverage |
| docs/design/input-keysyms.md, docs/research/issue-458-text-and-keysym-identity.md | Design note and primary-source research |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+133
to
+134
| let window = | ||
| wait_for_window(&env, &ready["pid"].to_string(), None, &|| run.is_finished())?; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fullwidth punctuation committed by fcitx5 was interpreted as X11 keysyms:
，became the undefinedkey-65292event, while other Unicode scalars collided with Backspace, Tab, Return, Escape, F13, and Redo. Preserve text identity from the toolkit through the core keyboard boundary so these characters self-insert and physical function keys keep their bindings.Fixes #458.
Changes
FrontendKey::Character(char)orFrontendKey::Keysym(u32)through GUI IME/text input, logical-key fallback, crossterm, and the input bridge. Rust's exhaustive matches keep text and function-key policies distinct.The original unit regression and native fcitx reproduction failed before the fix. The design follows GNU Emacs's decoded IME character events and is documented with primary-source research in
docs/research/issue-458-text-and-keysym-identity.md. Only the reporter's issue body was used.Review
Standards and spec reviews found no blockers, including a second integration review after rebasing onto
origin/main. The raw helper was renamed totranslate_keysymto make its domain explicit.Validation
Rebuilt and rerun after rebasing onto
bff37071fd:cargo build -p neomacs --binsThe same native fcitx scenario also passed against GNU Emacs 31.1 before the rebase. Native GUI verification covers Linux/X11; macOS and Windows were not run.