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

Implement IME primitives for PlainEditor. #136

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xorgy
Copy link
Member

@xorgy xorgy commented Oct 11, 2024

No description provided.

#[derive(Clone, Default)]
struct ComposeState {
selection: Option<(usize, usize)>,
text: Arc<str>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be a String. Then:

  • We could mutate it and re-use the allocation when updating the pre-edit
  • Setting it to empty would be allocation free

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Setting it to empty would be allocation free

it may not matter, but it's never empty

// Winit on some platforms delivers an empty Preedit after Commit
// so don't lock into compose when preedit is empty.
Ime::Preedit(text, None) if text.is_empty() => vec![
PlainEditorOp::SetCompose("".into(), None),
Copy link
Contributor

Choose a reason for hiding this comment

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

A dedicated ClearCompose Op for setting to empty might be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair, and definitely the ergonomic choice when moving to methods instead of ops.

@xorgy xorgy force-pushed the plain-editor-ime branch 6 times, most recently from 9a6f8f6 to d400210 Compare October 11, 2024 19:20
Comment on lines 183 to 198
CommitCompose => {
if let Some(ComposeState { text, .. }) = self.compose.clone() {
let new_insert = self.selection.insertion_index() + text.len();
self.selection = if new_insert == self.buffer.len() {
Selection::from_index(
&self.layout,
new_insert.saturating_sub(1),
Affinity::Upstream,
)
} else {
Selection::from_index(&self.layout, new_insert, Affinity::Downstream)
};
}
self.compose = None;
layout_after = true;
}
Copy link
Member

@tomcur tomcur Oct 21, 2024

Choose a reason for hiding this comment

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

It may not matter too much, but this does some additional work on each commit, as Winit delivers a Preedit("", None) before sending Commit. On commit, vello_editor in this PR currently clears the compose state, to immediately set it again, to then commit it. If CommitCompose takes the committed text, some of those buffer operations become unnecessary.

There also appear to be some IMEs that commit without going into compose first, so perhaps it makes sense to have SetCompose(Arc<str>, Option<(usize, usize)>), ClearCompose, and Commit(Arc<str>).

Commit(Arc<str>) would guarantee it also clears the compose state. Otherwise it's effectively the same as InsertOrReplaceSelection(Arc<str>).

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, that will be the final command set as it has been requested by both you and Nico, and it clearly makes sense.

@xorgy xorgy force-pushed the plain-editor-ime branch 2 times, most recently from b8c7909 to 3ce25f2 Compare October 23, 2024 20:16
@xorgy
Copy link
Member Author

xorgy commented Nov 21, 2024

This will be updated on the new method interface, and again once #170 enters.

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.

3 participants