Repository navigation
Conversation
huacnlee
left a comment
There was a problem hiding this comment.
This PR addresses a valid problem: a completion popup can retain the previous caret position because it reads persisted geometry before the input publishes the current frame's layout. I confirmed that GPUI's drawing-time notification does not deliver the observer notification or request the follow-up frame in this path.
However, please revise the approach before merging:
- The next-frame notification is installed in the shared Input implementation for every geometry change, including input bounds, scroll size, caret bounds, and line height. This requests an additional frame even for ordinary inputs with no caret-anchored popup. It also leaves popup positioning dependent on a second frame rather than using the current frame's geometry.
- The new test checks that an observer receives a notification, not that the completion popup actually follows the caret. It could pass while the popup remains incorrectly positioned.
Please resolve caret-anchored popup positioning during the current frame: make the necessary caret/input/scroll geometry available after input prepaint, and read it when the popup is positioned in deferred prepaint rather than capturing the previous position during render. Keep the change narrowly focused on positioning geometry rather than moving all of the existing paint-time state updates. Share the positioning logic between completion and code-action menus, and keep popup content, hitboxes, accessibility bounds, and width/layout decisions consistent with the resolved position.
Please add a regression test that verifies the popup follows the caret on the first rendered frame after editing, with coverage for wrapping/scrolling as appropriate, and that ordinary inputs do not acquire unnecessary follow-up frame requests.
Validation for this review: inspected the PR diff, Input and popup rendering paths, and the pinned GPUI notification/frame scheduling implementation. I did not run the tests or perform UI validation.
6b5b3f2 to
cb95759
Compare
|
Reworked per your feedback:
Only the positioning geometry is stashed in prepaint; the existing paint-time state updates are untouched. |
The popup position was resolved in render() from the previous frame's paint layout, so a completion menu opened on the keystroke's own frame stayed where the caret was until the next frame. The input now stashes fresh caret geometry (PrepaintCaretGeometry) during prepaint. Popups live inside deferred(), so their prepaint runs after the input's prepaint; a new CaretAnchoredPopup element resolves the anchor there via the shared caret_popup_anchor() and builds the popup content from it, keeping position, hitboxes, accessibility, and width decisions consistent with the resolved position. No follow-up frame is requested; ordinary inputs are unaffected.
cb95759 to
c10d183
Compare
Keep the code-action popup hitbox sized to its contents, encapsulate prepaint caret geometry, and verify completion positioning on the first frame including wrapping and scrolling. AI-assisted change using Codex.
A completion popup opened by a keystroke stays where the caret was: the menu resolves its position in
render()from the input's previous frame paint layout, but the caret's new position is only known after the current frame's prepaint.This change resolves the popup position during the current frame instead of requesting a follow-up frame:
TextElement::prepaintstashes fresh caret geometry (PrepaintCaretGeometry: cursor bounds, line height, input bounds, clamped scroll offset) in the state. Only the positioning geometry is stashed; the existing paint-time state updates are untouched.deferred(), so their prepaint runs after the input's prepaint. A newCaretAnchoredPopupelement resolves the anchor there via the sharedcaret_popup_anchor()and builds the popup content from it — position, hitboxes, accessibility bounds, and width/layout decisions (max_width, vertical vs horizontal docs) are all consistent with the resolved position.origin()methods are removed.No extra frame is requested for any input, ordinary or otherwise.
Two regression tests in
crates/kit/tests/input/completions.rs:completion_popup_follows_caret_on_first_frame— types a character, renders exactly one frame, and asserts the popup moved right with the caret and sits at the freshly laid-out caret position (not just moved).ordinary_input_schedules_no_followup_frame— types in a plain input, settles, and assertssimulate_next_frameschedules nothing.Public API
gpui-base
PrepaintCaretGeometry(doc-hidden) — caret positioning geometry from the input's latest prepaint: cursor bounds, line height, input bounds, and the clamped scroll offset.InputBaseState::prepaint_caret_geometry(&self) -> Option<PrepaintCaretGeometry>(doc-hidden) — reads the stashed geometry for caret-anchored popups.No public API changes; behavior-only fix otherwise.
This PR is 100% AI-generated: originally drafted with Claude on 2026-10-04, reworked per review feedback, rebased onto current main, reviewed and validated by the contributor:
cargo fmt --check,cargo clippy -p gpui-base -p gpui-component --all-targets -- --deny warnings, and the input test suite (168 passed, including the two new regression tests) are all green.