fix: render IME preedit text inline instead of only in the candidate popup - #1919
zhulenghan wants to merge 3 commits into
Conversation
Hosts that paint the cursor themselves had the position but not the shape: DECSCUSR (block / underline / beam) and DECTCEM hidden lived only in the Rust RenderState. rio_render_state_cursor_shape returns them as RIO_CURSOR_*, folding "scrolled into history" into HIDDEN the way cursor_visible() already does. rio_surface_child_pid hands out the spawned program's pid. It is a session leader, so a host can killpg() the whole tree on teardown; dropping the surface only hangs up the pty and SIGHUPs that one pid, which left editors the shell had started running as orphans. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…popup The composing string (e.g. pinyin typed before a candidate is chosen) never reached the terminal grid, so typing was effectively blind aside from the OS candidate popup - visible cross-platform (issue raphamorim#1883 on Wayland, also reported on macOS in that thread), not Windows-specific. Three things were needed together: - `Renderer::run` only stashed the first preedit char into an unused cursor field; nothing ever painted the actual composing text. Added `overlay_preedit_text` (mirrors the existing `overlay_hint_labels` mechanism) to splice the preedit string into the cursor's row, and a `HintTag::Preedit` that reuses the `HyperlinkHover` forced-underline path so it's visually marked without recoloring the cell. - `Ime::Preedit` updates never marked the panel dirty, so the render pipeline's per-context damage gate skipped the panel outright on an IME-only frame - same category as command-palette/search-bar input, which already has a documented `set_dirty()` escape hatch for exactly this. - IME composition doesn't generate terminal damage, so even with the panel marked dirty, the row-rebuild decision (driven by PTY/UI damage) never selected the cursor's row. Track the last row that carried a preedit overlay per route and force it into the rebuild set, both while composing and once more after composing ends without a commit (e.g. Escape), so the overlay is cleaned up correctly. Verified end-to-end on Windows 11 with Microsoft Pinyin: composing text now renders live, selecting a candidate commits normally, and Escape cancels cleanly with no leftover overlay. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmiYRs6weCxNBesjhEM3QQ
There was a problem hiding this comment.
🟡 Changes recommended
The new preedit overlay logic has correctness issues that can cause unintended recoloring and potential stale overlay artifacts when the cursor row changes during composition.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes IME composition (“preedit”) rendering so that in-progress text is drawn inline in the terminal grid (at the cursor), rather than being visible only via the OS IME candidate popup. It does this by adding a preedit overlay pass in the grid rendering path and ensuring IME-only updates still trigger the render pipeline to rebuild the affected row.
Changes:
- Add
HintTag::Preeditplusoverlay_preedit_textto splice IME preedit text into the cursor row and mark it for forced underline rendering. - Ensure IME preedit updates mark the current context as dirty so IME-only frames don’t get skipped by the panel dirty gate.
- Track/force rebuild of the cursor row for preedit frames (and after composition ends) so the overlay appears and is properly cleared.
File summaries
| File | Description |
|---|---|
| rio-grid/src/lib.rs | Adds preedit hint tagging and a new overlay function to render IME preedit text inline with forced underline treatment. |
| frontends/rioterm/src/screen/mod.rs | Threads preedit text into the per-panel render snapshot and forces the cursor row into the rebuild set for IME-only frames. |
| frontends/rioterm/src/application.rs | Marks the context dirty on IME preedit updates so the renderer doesn’t skip drawing when only IME state changes. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let prev_preedit_row = preedit_rows.get(route_id).copied(); | ||
| let force_preedit_row = if p.preedit_text.is_some() { | ||
| preedit_rows.insert(*route_id, p.cursor_row); | ||
| Some(p.cursor_row) | ||
| } else { | ||
| preedit_rows.remove(route_id); | ||
| prev_preedit_row | ||
| }; | ||
| let rows_to_rebuild = match (rows_to_rebuild, force_preedit_row) { | ||
| (RowsToRebuild::None, Some(row)) => { | ||
| if let Some(r) = p.visible_rows.get_mut(row as usize) { | ||
| r.dirty = true; | ||
| } | ||
| RowsToRebuild::Dirty | ||
| } | ||
| (mode, Some(row)) => { | ||
| if let Some(r) = p.visible_rows.get_mut(row as usize) { | ||
| r.dirty = true; | ||
| } | ||
| mode | ||
| } | ||
| (mode, None) => mode, | ||
| }; |
There was a problem hiding this comment.
Fixed — now tracking both the current cursor row and the previous overlay row, and forcing whichever ones changed into the rebuild set so a stale overlay from a row the cursor left never lingers. See 619e59a.
| let mut out: Option<(Row<Square>, Vec<Style>)> = None; | ||
| let start_col = col; | ||
| for ch in text.chars() { | ||
| if col >= width { | ||
| break; | ||
| } | ||
| let (target, styles) = out.get_or_insert_with(|| { | ||
| let mut styles = row_styles.to_vec(); | ||
| styles.resize(width, Style::default()); | ||
| (row.clone(), styles) | ||
| }); | ||
| let mut sq = Square::from_char(ch); | ||
| sq.set_style_id(PREEDIT_STYLE_ID); | ||
| target[Column(col)] = sq; | ||
| styles[col] = Style::default(); | ||
| col += 1; | ||
| } |
There was a problem hiding this comment.
Fixed — the per-cell style is no longer reset; it keeps whatever row_styles already had there, so composing over colored text (e.g. a colored prompt) no longer flashes it to default colors. See 619e59a.
…t row - overlay_preedit_text was resetting each overlaid cell's style to Style::default(), which would recolor the cell (e.g. composing over colored prompt text) even though HintTag::Preedit is meant to be underline-only, same as HyperlinkHover. Leave the cloned row_styles entry untouched instead. - The forced-row-rebuild logic only ever dirtied one row (the current cursor row, or the previous one if composing just ended) - if the cursor moved to a different row while still composing, the now-stale row's overlay was never cleared. Track and force both the current and (if different) the just-vacated row. Verified with a rebuilt binary: composing over colored prompt text no longer flashes it to default colors, and commit/cancel still work correctly end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KmiYRs6weCxNBesjhEM3QQ
|
@raphamorim would appreciate a review when you have a moment. Also FYI: the ubuntu |
Summary
The composing string (e.g. the pinyin typed before a candidate is chosen) never reached the terminal grid — typing was effectively blind aside from the OS candidate popup. This is cross-platform, not Windows-specific: see #1883 (Wayland), which also has a comment confirming the same symptom on macOS.
Three independent things were blocking this, all needed together (verified each is load-bearing by testing with one removed at a time):
Renderer::runonly stashed the first preedit char into an unused cursor field; nothing ever painted the actual composing text. Addedoverlay_preedit_text(mirrors the existingoverlay_hint_labelsmechanism) to splice the preedit string into the cursor's row, and aHintTag::Preeditthat reuses theHyperlinkHoverforced-underline treatment so it's visually marked without recoloring the cell.Ime::Preeditupdates never marked the panel dirty, so the render pipeline's per-context damage gate skipped the panel outright on an IME-only frame — same category as command-palette/search-bar input, which already has a documentedset_dirty()escape hatch for exactly this. Without this, the overlay only updates on the very first keystroke of a composition and then visibly freezes (confirmed by testing with it removed).Test plan
Verified end-to-end on Windows 11 with Microsoft Pinyin (built release binary, drove it with real IME input, screenshotted each state):
cargo check --workspace,cargo clippy -p rio-grid -p rioterm --release,cargo fmt --checkall clean (no new warnings)🤖 Generated with Claude Code
https://claude.ai/code/session_01KmiYRs6weCxNBesjhEM3QQ