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

Handle IME selection cursor hiding #198

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Conversation

tomcur
Copy link
Member

@tomcur tomcur commented Dec 5, 2024

When the IME indicates a cursor of None, it means we should hide the cursor. This does that by collapsing the selection, hiding the caret, and clearing the text selection from the AccessKit node.

When the IME indicates a cursor of `None`, it means we should hide the
cursor. This does that by collapsing the selection, hiding the caret,
and clearing the text selection from the AccessKit node.
Comment on lines 1025 to 1033
if self.compose.as_ref().map(|c| c.show_cursor).unwrap_or(true) {
if let Some(selection) = self
.selection
.to_access_selection(&self.layout, &self.layout_access)
{
node.set_text_selection(selection);
}
} else {
node.clear_text_selection();
Copy link
Member Author

Choose a reason for hiding this comment

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

Is clearing the text selection, in response to the IME hiding the cursor, the right thing to do?

(I also wonder (unrelated to this PR), whether doing nothing when to_access_selection is None is right, though it appears that only happens when in a strange state.)

Comment on lines 93 to 100
/// IME compose state.
#[derive(Clone)]
struct Compose {
/// Byte offsets of IME composing preedit text in the text buffer.
preedit_range: Range<usize>,
/// Whether the cursor should be shown. The IME can request to hide the cursor.
show_cursor: bool,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

As a style note, I could be convinced to move both of these to PlainEditor... It's a trade-off between type-level correctness and ergonomics. In fact, "not-composing" could be encoded with an empty preedit_range, which with the current layout would shave 8 bytes off PlainEditor.

Copy link
Member

Choose a reason for hiding this comment

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

self.compose.as_ref().map(|c| c.show_cursor).unwrap_or(true) is so ugly (even self.compose.as_ref().is_none_or(|c| c.show_cursor) is still pretty bad) that I think flattening this is a win. It's effectively only set in two functions anyway, so it's unlikely to get out-of-sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

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'm surprised that this isn't a breaking change. I think flattening the new struct is nicer, because the current ergonomics are impressively bad.

self.selection.geometry(&self.layout)
}

/// Get a rectangle representing the current caret cursor position.
///
/// There is not always a caret. For example, the IME may have indicated the caret should be
/// hidden.
pub fn cursor_geometry(&self, size: f32) -> Option<Rect> {
Copy link
Member

Choose a reason for hiding this comment

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

Handy that this was already an option...

Comment on lines 93 to 100
/// IME compose state.
#[derive(Clone)]
struct Compose {
/// Byte offsets of IME composing preedit text in the text buffer.
preedit_range: Range<usize>,
/// Whether the cursor should be shown. The IME can request to hide the cursor.
show_cursor: bool,
}
Copy link
Member

Choose a reason for hiding this comment

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

self.compose.as_ref().map(|c| c.show_cursor).unwrap_or(true) is so ugly (even self.compose.as_ref().is_none_or(|c| c.show_cursor) is still pretty bad) that I think flattening this is a win. It's effectively only set in two functions anyway, so it's unlikely to get out-of-sync.

@tomcur tomcur enabled auto-merge December 6, 2024 11:03
@tomcur tomcur added this pull request to the merge queue Dec 6, 2024
Merged via the queue into linebender:main with commit 426e9ef Dec 6, 2024
20 checks passed
@tomcur tomcur deleted the ime-hide-cursor branch December 6, 2024 11:07
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