Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear compose in TextArea::reset_text #768

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

tomcur
Copy link
Member

@tomcur tomcur commented Dec 3, 2024

The user may be composing when the TextArea text is programmatically changed. This change clears the compose state in order to adhere to the PlainEditor requirements. It is disruptive, but we already warn about disruption (and about IME in particular) in the documentation of TextArea::reset_text.

This doesn't reset the platform's IME state, we could do, but that's potentially even more disruptive, and looks like it's a bit harder to wire up. We could also re-insert the preedit text. That'd be nicest to do if we can query PlainEditor for the current preedit text, which requires an API change there.

The user may be composing when the `TextArea` text is programmatically
changed. This change clears the compose state in order to adhere to the
`PlainEditor` requirements. It is disruptive, but we already warn about
disruption (and about IME in particular) in the documentation of
`TextArea::reset_text`.

This doesn't reset the platform's IME state, we could do, but that's
potentially even more disruptive, and looks like it's a bit harder to
wire up. We could also re-insert the preedit text. That'd be nicest to
do if we can query `PlainEditor` for the current preedit text, which
requires an API change there.
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I do already have this in #766, but I think landing it now also makes sense.

@tomcur
Copy link
Member Author

tomcur commented Dec 3, 2024

Ah, seems like we crossed each other. But yeah, let's land this now, as it's quite easy to cause crashes in the current state.

@tomcur tomcur enabled auto-merge December 3, 2024 16:35
@tomcur
Copy link
Member Author

tomcur commented Dec 3, 2024

Note for posterity: there's a separate issue in Xilem where views like Textbox use PlainEditor::text() to check whether to use reset_text. Because PlainEditor::text() currently returns the text including the preedit, whereas Masonry hides the preedit from Xilem, the resetting is a bit overzealous.

@tomcur tomcur added this pull request to the merge queue Dec 3, 2024
Merged via the queue into linebender:main with commit 89085cf Dec 3, 2024
17 checks passed
@tomcur tomcur deleted the clear-compose branch December 3, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants