gpui: Anchor IME candidate window to the start of the visual line - #55876
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 255a2a2493
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.state.is_composing.get() { | ||
| return; |
There was a problem hiding this comment.
Allow IME cursor moves to reposition candidates
When an IME changes the composition cursor without ending composition (for example, Japanese conversion/arrowing through clauses), handle_ime_composition_inner already consumes GCS_CURSORPOS and updates the selected range inside the marked text; that selection change is what drives invalidate_character_coordinates(). This guard drops those bounds updates for the entire composition, so ImmSetCandidateWindow remains at the original composition start even though the active clause/cursor has moved, leaving the candidate UI detached from the segment being converted.
Useful? React with 👍 / 👎.
…ng composition" This reverts commit 255a2a2.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02789d1a3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(selection) = input_handler.marked_text_range() { | ||
| bounds = input_handler.bounds_for_range(selection.start..selection.start); | ||
| let marked_range = input_handler.marked_text_range(); | ||
| if let Some(selection) = input_handler.selected_text_range(true) { |
There was a problem hiding this comment.
Handle marked text when selection is unavailable
On Wayland, this now drops the IME area whenever the input handler has marked text but returns no selection. That happens in the terminal while an alternate-screen program is active: TerminalInputHandler::selected_text_range returns None for TermMode::ALT_SCREEN, but marked_text_range still returns the preedit range after SetMarkedText. The old code still positioned the preedit at the marked start; with this guard get_ime_area() returns None, so the PreeditString Done path in client.rs skips set_cursor_rectangle and its conditional text_input.commit(), regressing Wayland IME preedit/candidate positioning in full-screen terminal apps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this is a regression introduced by this change.
On Wayland, the previous get_ime_area() implementation also depended on selected_text_range(true), so in alt-screen terminal mode it already returned None when
selected_text_range() was unavailable. In other words, this edge case was not handled before, and this patch does not change that behavior.
So this may still be a separate limitation worth addressing, but it is not newly introduced by this change.
| pub fn selected_bounds(&mut self, window: &mut Window, cx: &mut App) -> Option<Bounds<Pixels>> { | ||
| let marked_range = self.handler.marked_text_range(window, cx); | ||
| let selection = self.handler.selected_text_range(true, window, cx)?; | ||
| self.handler.bounds_for_range( | ||
| if selection.reversed { | ||
| selection.range.start..selection.range.start | ||
|
|
||
| if let Some(marked_range) = marked_range { | ||
| let (base_start, caret_offset) = | ||
| if selection.range.is_empty() && selection.range.end == marked_range.end { | ||
| (marked_range.start, selection.range.end) | ||
| } else { | ||
| (selection.range.start, selection.range.end) | ||
| }; | ||
|
|
||
| let mut anchor_offset = base_start; | ||
| if let Some(caret_bounds) = | ||
| self.handler | ||
| .bounds_for_range(caret_offset..caret_offset, window, cx) | ||
| { | ||
| for i in (base_start..caret_offset).rev() { | ||
| if let Some(b) = self.handler.bounds_for_range(i..i, window, cx) { | ||
| if (b.origin.y - caret_bounds.origin.y).abs() > px(0.1) { | ||
| anchor_offset = i + 1; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| self.handler | ||
| .bounds_for_range(anchor_offset..anchor_offset, window, cx) | ||
| } else { | ||
| let offset = if selection.reversed { | ||
| selection.range.start | ||
| } else { | ||
| selection.range.end..selection.range.end | ||
| }, | ||
| window, | ||
| cx, | ||
| ) | ||
| selection.range.end | ||
| }; | ||
| self.handler.bounds_for_range(offset..offset, window, cx) | ||
| } |
There was a problem hiding this comment.
Sorry for the late review. Mind adding a comment or two to this logic explaining what is being done here? It's a lot more complicated from what was here before for something that should presumably be very simple in nature
There was a problem hiding this comment.
Simplified the logic as requested. Let me know if any further changes are needed.
186de08 to
849ca9f
Compare
…d-industries#55876) ## Summary This PR fixes horizontal jumping of the IME candidate window during text composition. On the shared `selected_bounds` path used by Windows, the candidate window no longer follows the preedit caret character-by-character. Instead, it anchors to the start of the current visual line, which keeps the candidate window stable while typing. Linux Wayland uses a different IME positioning path, so it was not affected by the original implementation in this PR. This PR now also includes a Wayland-specific adjustment so that Wayland uses the same visual-line anchoring behavior. ## Changes 1. **Shared / Windows path** Updated `selected_bounds` in `crates/gpui/src/platform.rs` to use a visual-line-aware anchor for preedit text. This removes the distracting horizontal movement of the candidate window while typing and keeps the anchor aligned with the active visual line. 2. **Linux Wayland path** Updated the Wayland IME area calculation in `crates/gpui_linux/src/linux/wayland/window.rs` to use the same visual-line-start anchoring strategy for preedit text. This brings Linux Wayland in line with the Windows behavior while preserving Wayland's platform-specific IME handling. ## Notes I had previously tried a separate Wayland-specific fix in 5d0c968, but later reverted it in 8cfe7a2 because the behavior was not good enough and it regressed the original positioning behavior. The current Wayland implementation is a new, simpler approach that keeps the original behavior intact while also removing horizontal candidate-window jumping. ## Visuals ### Windows / shared path * **Before the fix:** (The candidate box moves along with the preedit text, which is distracting) <img width="918" height="675" alt="before" src="https://github.com/user-attachments/assets/29cb05e4-3e99-4b54-9ce3-78710b307ce6" /> <img width="478" height="682" alt="before_1" src="https://github.com/user-attachments/assets/cff38b65-96dd-4ed7-b06b-7fbcd448fab9" /> * **After the fix:** (Candidate box stays fixed at the line start during typing, correctly jumps to new lines or follows active segments) <img width="918" height="675" alt="after" src="https://github.com/user-attachments/assets/4f50a710-dc0d-4959-a313-1184122ab759" /> <img width="478" height="683" alt="after_1" src="https://github.com/user-attachments/assets/8e9df121-ffcc-45ad-ba0b-f1ad3cef87bc" /> ### Linux Wayland * **Before / previous behavior** <img width="552" height="796" alt="before" src="https://github.com/user-attachments/assets/e3e84312-c626-41cb-945f-66c13aa96df6" /> * **After / current implementation** <img width="546" height="772" alt="after" src="https://github.com/user-attachments/assets/a01ab6d7-a3b6-4d56-a1cb-b2b112b6b914" /> --- ## Self-Review Checklist: * [x] I've reviewed my own diff for quality, security, and reliability * [x] Unsafe blocks (if any) have justifying comments * [x] The content is consistent with the UI/UX checklist * [x] Tests cover the new/changed behavior (manual verification only) * [x] Performance impact has been considered and is acceptable Release Notes: - N/A
…d-industries#55876) ## Summary This PR fixes horizontal jumping of the IME candidate window during text composition. On the shared `selected_bounds` path used by Windows, the candidate window no longer follows the preedit caret character-by-character. Instead, it anchors to the start of the current visual line, which keeps the candidate window stable while typing. Linux Wayland uses a different IME positioning path, so it was not affected by the original implementation in this PR. This PR now also includes a Wayland-specific adjustment so that Wayland uses the same visual-line anchoring behavior. ## Changes 1. **Shared / Windows path** Updated `selected_bounds` in `crates/gpui/src/platform.rs` to use a visual-line-aware anchor for preedit text. This removes the distracting horizontal movement of the candidate window while typing and keeps the anchor aligned with the active visual line. 2. **Linux Wayland path** Updated the Wayland IME area calculation in `crates/gpui_linux/src/linux/wayland/window.rs` to use the same visual-line-start anchoring strategy for preedit text. This brings Linux Wayland in line with the Windows behavior while preserving Wayland's platform-specific IME handling. ## Notes I had previously tried a separate Wayland-specific fix in 5d0c968, but later reverted it in 8cfe7a2 because the behavior was not good enough and it regressed the original positioning behavior. The current Wayland implementation is a new, simpler approach that keeps the original behavior intact while also removing horizontal candidate-window jumping. ## Visuals ### Windows / shared path * **Before the fix:** (The candidate box moves along with the preedit text, which is distracting) <img width="918" height="675" alt="before" src="https://github.com/user-attachments/assets/29cb05e4-3e99-4b54-9ce3-78710b307ce6" /> <img width="478" height="682" alt="before_1" src="https://github.com/user-attachments/assets/cff38b65-96dd-4ed7-b06b-7fbcd448fab9" /> * **After the fix:** (Candidate box stays fixed at the line start during typing, correctly jumps to new lines or follows active segments) <img width="918" height="675" alt="after" src="https://github.com/user-attachments/assets/4f50a710-dc0d-4959-a313-1184122ab759" /> <img width="478" height="683" alt="after_1" src="https://github.com/user-attachments/assets/8e9df121-ffcc-45ad-ba0b-f1ad3cef87bc" /> ### Linux Wayland * **Before / previous behavior** <img width="552" height="796" alt="before" src="https://github.com/user-attachments/assets/e3e84312-c626-41cb-945f-66c13aa96df6" /> * **After / current implementation** <img width="546" height="772" alt="after" src="https://github.com/user-attachments/assets/a01ab6d7-a3b6-4d56-a1cb-b2b112b6b914" /> --- ## Self-Review Checklist: * [x] I've reviewed my own diff for quality, security, and reliability * [x] Unsafe blocks (if any) have justifying comments * [x] The content is consistent with the UI/UX checklist * [x] Tests cover the new/changed behavior (manual verification only) * [x] Performance impact has been considered and is acceptable Release Notes: - N/A
…d-industries#55876) ## Summary This PR fixes horizontal jumping of the IME candidate window during text composition. On the shared `selected_bounds` path used by Windows, the candidate window no longer follows the preedit caret character-by-character. Instead, it anchors to the start of the current visual line, which keeps the candidate window stable while typing. Linux Wayland uses a different IME positioning path, so it was not affected by the original implementation in this PR. This PR now also includes a Wayland-specific adjustment so that Wayland uses the same visual-line anchoring behavior. ## Changes 1. **Shared / Windows path** Updated `selected_bounds` in `crates/gpui/src/platform.rs` to use a visual-line-aware anchor for preedit text. This removes the distracting horizontal movement of the candidate window while typing and keeps the anchor aligned with the active visual line. 2. **Linux Wayland path** Updated the Wayland IME area calculation in `crates/gpui_linux/src/linux/wayland/window.rs` to use the same visual-line-start anchoring strategy for preedit text. This brings Linux Wayland in line with the Windows behavior while preserving Wayland's platform-specific IME handling. ## Notes I had previously tried a separate Wayland-specific fix in 5d0c968, but later reverted it in 8cfe7a2 because the behavior was not good enough and it regressed the original positioning behavior. The current Wayland implementation is a new, simpler approach that keeps the original behavior intact while also removing horizontal candidate-window jumping. ## Visuals ### Windows / shared path * **Before the fix:** (The candidate box moves along with the preedit text, which is distracting) <img width="918" height="675" alt="before" src="https://github.com/user-attachments/assets/29cb05e4-3e99-4b54-9ce3-78710b307ce6" /> <img width="478" height="682" alt="before_1" src="https://github.com/user-attachments/assets/cff38b65-96dd-4ed7-b06b-7fbcd448fab9" /> * **After the fix:** (Candidate box stays fixed at the line start during typing, correctly jumps to new lines or follows active segments) <img width="918" height="675" alt="after" src="https://github.com/user-attachments/assets/4f50a710-dc0d-4959-a313-1184122ab759" /> <img width="478" height="683" alt="after_1" src="https://github.com/user-attachments/assets/8e9df121-ffcc-45ad-ba0b-f1ad3cef87bc" /> ### Linux Wayland * **Before / previous behavior** <img width="552" height="796" alt="before" src="https://github.com/user-attachments/assets/e3e84312-c626-41cb-945f-66c13aa96df6" /> * **After / current implementation** <img width="546" height="772" alt="after" src="https://github.com/user-attachments/assets/a01ab6d7-a3b6-4d56-a1cb-b2b112b6b914" /> --- ## Self-Review Checklist: * [x] I've reviewed my own diff for quality, security, and reliability * [x] Unsafe blocks (if any) have justifying comments * [x] The content is consistent with the UI/UX checklist * [x] Tests cover the new/changed behavior (manual verification only) * [x] Performance impact has been considered and is acceptable Release Notes: - N/A
…d-industries#55876) ## Summary This PR fixes horizontal jumping of the IME candidate window during text composition. On the shared `selected_bounds` path used by Windows, the candidate window no longer follows the preedit caret character-by-character. Instead, it anchors to the start of the current visual line, which keeps the candidate window stable while typing. Linux Wayland uses a different IME positioning path, so it was not affected by the original implementation in this PR. This PR now also includes a Wayland-specific adjustment so that Wayland uses the same visual-line anchoring behavior. ## Changes 1. **Shared / Windows path** Updated `selected_bounds` in `crates/gpui/src/platform.rs` to use a visual-line-aware anchor for preedit text. This removes the distracting horizontal movement of the candidate window while typing and keeps the anchor aligned with the active visual line. 2. **Linux Wayland path** Updated the Wayland IME area calculation in `crates/gpui_linux/src/linux/wayland/window.rs` to use the same visual-line-start anchoring strategy for preedit text. This brings Linux Wayland in line with the Windows behavior while preserving Wayland's platform-specific IME handling. ## Notes I had previously tried a separate Wayland-specific fix in b2eac94, but later reverted it in 8cfe7a2 because the behavior was not good enough and it regressed the original positioning behavior. The current Wayland implementation is a new, simpler approach that keeps the original behavior intact while also removing horizontal candidate-window jumping. ## Visuals ### Windows / shared path * **Before the fix:** (The candidate box moves along with the preedit text, which is distracting) <img width="918" height="675" alt="before" src="https://github.com/user-attachments/assets/29cb05e4-3e99-4b54-9ce3-78710b307ce6" /> <img width="478" height="682" alt="before_1" src="https://github.com/user-attachments/assets/cff38b65-96dd-4ed7-b06b-7fbcd448fab9" /> * **After the fix:** (Candidate box stays fixed at the line start during typing, correctly jumps to new lines or follows active segments) <img width="918" height="675" alt="after" src="https://github.com/user-attachments/assets/4f50a710-dc0d-4959-a313-1184122ab759" /> <img width="478" height="683" alt="after_1" src="https://github.com/user-attachments/assets/8e9df121-ffcc-45ad-ba0b-f1ad3cef87bc" /> ### Linux Wayland * **Before / previous behavior** <img width="552" height="796" alt="before" src="https://github.com/user-attachments/assets/e3e84312-c626-41cb-945f-66c13aa96df6" /> * **After / current implementation** <img width="546" height="772" alt="after" src="https://github.com/user-attachments/assets/a01ab6d7-a3b6-4d56-a1cb-b2b112b6b914" /> --- ## Self-Review Checklist: * [x] I've reviewed my own diff for quality, security, and reliability * [x] Unsafe blocks (if any) have justifying comments * [x] The content is consistent with the UI/UX checklist * [x] Tests cover the new/changed behavior (manual verification only) * [x] Performance impact has been considered and is acceptable Release Notes: - N/A
Summary
This PR fixes horizontal jumping of the IME candidate window during text composition.
On the shared
selected_boundspath used by Windows, the candidate window no longer follows the preedit caret character-by-character. Instead, it anchors to the start of the current visual line, which keeps the candidate window stable while typing.Linux Wayland uses a different IME positioning path, so it was not affected by the original implementation in this PR. This PR now also includes a Wayland-specific adjustment so that Wayland uses the same visual-line anchoring behavior.
Changes
Shared / Windows path
Updated
selected_boundsincrates/gpui/src/platform.rsto use a visual-line-aware anchor for preedit text.This removes the distracting horizontal movement of the candidate window while typing and keeps the anchor aligned with the active visual line.
Linux Wayland path
Updated the Wayland IME area calculation in
crates/gpui_linux/src/linux/wayland/window.rsto use the same visual-line-start anchoring strategy for preedit text.This brings Linux Wayland in line with the Windows behavior while preserving Wayland's platform-specific IME handling.
Notes
I had previously tried a separate Wayland-specific fix in 5d0c968, but later reverted it in 8cfe7a2 because the behavior was not good enough and it regressed the original positioning behavior.
The current Wayland implementation is a new, simpler approach that keeps the original behavior intact while also removing horizontal candidate-window jumping.
Visuals
Windows / shared path
Linux Wayland
Self-Review Checklist:
Release Notes: