Skip to content

fix: refresh IME cursor area after redraw#10443

Open
bet4it wants to merge 2 commits intowarpdotdev:masterfrom
bet4it:bet4it/fix-ime-redraw-position
Open

fix: refresh IME cursor area after redraw#10443
bet4it wants to merge 2 commits intowarpdotdev:masterfrom
bet4it:bet4it/fix-ime-redraw-position

Conversation

@bet4it
Copy link
Copy Markdown

@bet4it bet4it commented May 8, 2026

Description

This change refreshes the IME cursor area after a successful redraw when IME is enabled.

That keeps the IME preedit/candidate UI aligned with the terminal cursor during redraw-heavy terminal apps.

Linked Issue

  • The linked issue is labeled ready-to-spec or ready-to-implement.
  • Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes).

Testing

  • I have manually tested my changes locally with ./script/run

Screenshots / Videos

Before this PR:
Screenshot1
After this PR:
Screenshot2

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 8, 2026

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @bet4it on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment @cla-bot check to trigger another check.

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 8, 2026

@bet4it

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I reviewed this pull request and requested human review from: @vorporeal.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@github-actions github-actions Bot added the external-contributor Indicates that a PR has been opened by someone outside the Warp team. label May 8, 2026
@bet4it
Copy link
Copy Markdown
Author

bet4it commented May 8, 2026

@cla-bot check

@cla-bot cla-bot Bot added the cla-signed label May 8, 2026
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 8, 2026

The cla-bot has been summoned, and re-checked this pull request!

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR updates the winit redraw success path to notify frame_drawn and then refresh the IME cursor area when IME is enabled, keeping preedit/candidate UI aligned after redraws.

Concerns

  • No blocking correctness or security concerns found in the changed hunk.

Verdict

Found: 0 critical, 0 important, 0 suggestions

Approve

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@oz-for-oss oz-for-oss Bot requested a review from vorporeal May 8, 2026 05:37
Copy link
Copy Markdown
Contributor

@vorporeal vorporeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my main question is whether this is the correct place to be doing this. feels a bit odd that we're unconditionally attempting to update the IME position after every frame we render, as opposed to this being reactive to a change in cursor position?

we do seem to have this reactive logic (the ActiveCursorPositionUpdated event) - do you have a sense as to why that event isn't sufficient?

@vorporeal
Copy link
Copy Markdown
Contributor

i guess in some ways it ends up being a question of whether this is a flow that should be reactive (i.e.: focus and cursor position changes trigger a call into platform code to update the IME position) or whether we want to "poll" for an updated position on every frame when the IME is open. i'm doing some quick testing on macOS to see what the update behavior/frequency is like there, to get a better sense of what might be reasonable to do on linux.

@bet4it bet4it force-pushed the bet4it/fix-ime-redraw-position branch from decf331 to 46e6c9b Compare May 9, 2026 04:19
@bet4it
Copy link
Copy Markdown
Author

bet4it commented May 9, 2026

I’ve updated the patch to remove the redraw-side IME update entirely. The winit event loop now only reacts to ActiveCursorPositionUpdated; the actual cursor-position change detection lives in AppContext::build_scene(), where the rebuilt layout has the final active cursor rect. That path compares the newly computed cursor position against the last observed one and only emits the event when it actually changes.

The old event path wasn’t sufficient for terminal-style cursor movement because that movement doesn’t currently surface as a separate signal before layout/rendering. build_scene() is the first place where the layout-derived cursor position is reliably available, so that’s where I moved the comparison. On Linux/FreeBSD, the platform IME call is now strictly event-driven again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed external-contributor Indicates that a PR has been opened by someone outside the Warp team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants