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

parley: add IME support to text editor example #111

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tomcur
Copy link
Member

@tomcur tomcur commented Sep 4, 2024

This is a first pass at adding IME support to the editor example.

I don't speak languages that require the use of an IME, so I'm not myself a user of them. I think the implementation is in the right direction.

2024-09-04.20-56-28.mp4

@tomcur
Copy link
Member Author

tomcur commented Sep 4, 2024

It might be possible to handle compose keys similarly to IME. That's the initial reason I started to look into this. For example, with altgr composing, dead keys that are about to be composed could be shown. LibreOffice and Firefox do this.

Unfortunately, it seems the events given by winit are not very easy to use for that purpose. For example, when composing, the KeyboardInput event for " shows a dead key, but does not indicate it is the logical key " nor the text ".

@DJMcNab
Copy link
Member

DJMcNab commented Sep 4, 2024

What windowing system are you using? The compose behaviour is very platform specific.

I implemented this very carefully in linebender/glazier#119, which Winit should probably emulate for Gnome users.
(On KDE it's not possible to do better because the platform runs all the compose through the text input interface)

@tomcur
Copy link
Member Author

tomcur commented Sep 4, 2024

I've tested on Wayland compositor River 0.3.5 (wlroots). I can try whether Winit on X11 gives different results.

The following from your implementation is exactly what I was going for.

https://github.com/DJMcNab/glazier/blob/76c35367a9eb9f33e49754319ea1580abf0b7dfc/src/text.rs#L470-L484

I've read and also see your changes mention some platforms handle composing as IME – I was hoping that's how this composing behavior was implemented by applications, but unfortunately it doesn't seem to be that simple.

@DJMcNab
Copy link
Member

DJMcNab commented Sep 4, 2024

Winit absolutely should handle compose as IME, in my estimation (IME!). Ideally, that would lift-and-shift a lot of the code from that pull request being brought into winit.
I'm not going to do that myself, because:

  1. I have other priorities
  2. KDE, which I currently use, wouldn't use that code.

I do believe that the resulting UX is excellent, though.
I'd recommend trying out compose in the edit_text example in Glazier to see what you think.

Edit: Oh, interestingly it looks like this might have never been the case on Gnome, so it's actually no longer true, and everything uses ibus? Not sure. It's been more than a year since I deeply dug into this.

@xorgy
Copy link
Member

xorgy commented Sep 4, 2024

Thanks. One issue is that you have to replace the preedit text rather than continuing to insert it repeatedly when it gets updated.

@tomcur
Copy link
Member Author

tomcur commented Sep 6, 2024

Do you mean how in the above example the letters I'm typing are inserted one after the other as preedit text? Typing "ni hao.", the preedit events are:

Ime::Enabled
Ime::Preedit("", None)
Ime::Preedit("n", Some((0, 1)))
Ime::Preedit("", None)
Ime::Preedit("ni", Some((0, 2)))
Ime::Preedit("", None)
Ime::Commit("你")
Ime::Preedit("", None)
Ime::Preedit("", None)
Ime::Preedit("h", Some((0, 1)))
Ime::Preedit("", None)
Ime::Preedit("ha", Some((0, 2)))
Ime::Preedit("", None)
Ime::Preedit("hao", Some((0, 3)))
Ime::Preedit("", None)
Ime::Commit("好")
Ime::Preedit("", None)
Ime::Preedit("", None)
Ime::Commit("。")
Ime::Disabled

Displaying the full preedit text seems to be the intended behavior here.

(As a side note, I think the preedit events clearing the text in between subsequent preedits are sent by fcitx5. Winit does guarantee one Ime::Preedit("", None) to be sent prior to Ime::Commit.)

parley/src/layout/cursor.rs Outdated Show resolved Hide resolved
@tomcur
Copy link
Member Author

tomcur commented Sep 6, 2024

I've pushed some improvements. The preedit text now sticks to the selection anchor: if the selection is changed, the preedit text is moved to the new position (e.g., by pressing with the pointer somewhere). When the preedit is started, the selected text is deleted. When a selection is made while preediting, the selected text is deleted when the preedit is continued. When focus is lost (shown at the end of the video), or if the preedit is cleared, the original selection remains.

(This introduces some bugs with some other events like Ctrl+X, need to think about whether there is a nice way to handle all those events. Perhaps some events should just be noops while preediting is active? For example, Firefox blocks pasting while preediting.)

2024-09-06.22-41-01.mp4

@xorgy
Copy link
Member

xorgy commented Sep 6, 2024

(This introduces some bugs with some other events like Ctrl+X, need to think about whether there is a nice way to handle all those events. Perhaps some events should just be noops while preediting is active? For example, Firefox blocks pasting while preediting.)

Yes, copy and cut are meaningless during preedit; not sure about paste, but since it's sorta unclear what it should do if the cursor is inside the preedit region, blocking it during preedit probably makes sense.

- Use `String::replace_range`
- Select area indicated by IME
- Refactor to `Selection::from_cursors` rather than `Selection::extend_to_cursor`
@tomcur
Copy link
Member Author

tomcur commented Sep 11, 2024

Pushed some performance and selection handling improvements. IME preedit now blocks events other than selection change events (so cut/copy/paste, delete, backspace and text insertion—perhaps delete, backspace or text insertion are never sent by Winit during preedit, but I'm not sure).

Maybe selection change should also be blocked during preedit: then all pointer and keyboard events can simply be blocked. There's a case to be made for that:

https://github.com/linebender/xilem/blob/9a3c8e308c3f46f15bb1a5bd48b9119f0178ad98/masonry/src/text/reference/input_component.rs#L160-L166

@DJMcNab DJMcNab requested a review from xorgy September 17, 2024 18:57
@DJMcNab
Copy link
Member

DJMcNab commented Sep 18, 2024

This is running into linebender/vello#618 for me.

However, aside from that, I'm seeing really awful performance when inputting IME. That is, the app gets the "not responding" thing. I'll try to debug tomorrow morning

@tomcur
Copy link
Member Author

tomcur commented Sep 19, 2024

However, aside from that, I'm seeing really awful performance when inputting IME. That is, the app gets the "not responding" thing. I'll try to debug tomorrow morning

Perhaps on your platform it somehow gets into a loop where setting the IME box candidate area triggers an event that triggers setting the area?

@DJMcNab
Copy link
Member

DJMcNab commented Sep 19, 2024

Yeah, I think that's what's happening.

Worse, this is actually multiplying. That is, every frame an extra iteration appears to be added.

@DJMcNab
Copy link
Member

DJMcNab commented Oct 3, 2024

Before I can approve, I'd like to see the IME looping resolved. I probably won't have time to dig into that myself in the next couple of weeks, unfortunately. But if that becomes resolved, then I'll happily approve, as this code otherwise looks good!

We probably need to cache the last sent IME cursor area, and not send it again if we would be sending the same value. It is a bit stupid of my platform's IME implementation to send an IME preedit event in response to the movement, but we have to deal with that.

@tomcur
Copy link
Member Author

tomcur commented Oct 3, 2024

We probably need to cache the last sent IME cursor area, and not send it again if we would be sending the same value.

I'd be happy to make the suggested change. On my platform I cannot reproduce the issue, so I'll have to ping you to see if the issue is resolved.

Hopefully that process doesn't loop.

@tomcur
Copy link
Member Author

tomcur commented Oct 4, 2024

I have a patch with this solution, but working on it I realized it may also be resolved by checking on Ime::Preedit events whether the new selection is different from the old selection. If and only if so, send the new cursor area to the platform. If that works, we don't grow the memory footprint of Editor.

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 don't think this solution for placing the cursor area works for e.g. the windows emoji picker. That is, not all IME inputs would necessarily use a cursor area.

Comment on lines 174 to 176
// Find the smallest rectangle that contains the entire preedit text.
// Send that rectangle to the platform to suggest placement for the IME
// candidate box.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure that this is quite the right logic. For example, consider the Windows emoji picker; at least ideally it uses the cursor area, but doesn't add a pre-edit text.
I think that this whole statement needs to have an else branch, which uses something based on the selection.
My proposal would be to use one of:

  1. The entire selection area;
  2. The area of the focused cluster

Copy link
Member Author

@tomcur tomcur Oct 4, 2024

Choose a reason for hiding this comment

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

I think you're right. Perhaps the area should always be based on the selection (that's how I mistakenly remembered it actually, so my previous commit doesn't work as expected).

Either there is no preedit, and the selection is the only thing that makes sense, or there is a preedit, and the IME gave us a selection within it (Ime::Preedit(_, cursor)). Although, if the preedit wraps lines and the preedit cursor is a partial selection, the candidate box may then overlap.

I'm also a bit confused by the name Window::set_ime_cursor_area, as that seems to suggest the area of the IME cursor (as given by Ime::Preedit(_, cursor)) is to be given, whereas the documentation states: "an example of such area could be a input field in the UI or line in the editor."

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; this API is underdocumented from the winit side. I don't have good answers here.

I think the IME box should never cover the pre-edit text, so if there is a pre-edit, I think the current answer is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

The area is now calculated as the minimum rect containing the preedit text, if present. If not, it depends on whether the selection is collapsed. A collapsed selection sends the caret area, a spanning selection is the minimum rect containing the selection.

@tomcur
Copy link
Member Author

tomcur commented Oct 5, 2024

@DJMcNab as you mentioned IMEs that do not necessarily set preedit text (e.g., emoji pickers), I've made the IME state tracking a bit more complete. Editor now also tracks whether the IME is enabled, instead of only tracking whether there is a preedit.

If the IME is enabled, candidate areas are sent. Previously they were only sent if there was an active preedit (i.e., the IME was composing). I think the logic in the most recent commit is correct, but I don't have an easy way to test.

@xorgy xorgy added this to the 0.3 Release milestone Oct 10, 2024
@xorgy
Copy link
Member

xorgy commented Oct 10, 2024

I will add the requisite support in PlainEditor for Ime, and fix this branch up for that. 0.3 should be a reasonably quick followup.

if self.ime != ImeState::Disabled
&& !matches!(
event,
WindowEvent::RedrawRequested | WindowEvent::CursorMoved { .. }
Copy link
Member

Choose a reason for hiding this comment

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

Can CursorMoved change the selection if we're currently dragging?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Currently the selection is also changed if we click somewhere. The editor state remains consistent: the IME preedit is moved to the new anchor position, and the span of the selection itself is mostly a visual thing. The user can't delete, copy, paste, etc., while a preedit is active.

What we should do here is not clear. Moving the preedit text to the new anchor location feels a bit clunky, and that's even with the IME selecting the entire preedit. If only part of the preedit is selected, it'll probably feel even stranger.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean by this comment is that the pre-edit "no overlap" area is also set by the selection, but if the selection expands, then that won't be covered until it's updated by some other event.
Essentially, if the set of events is:

Mouse down
Move cursor (to expand selection)
Windows + . (to open the emoji picker)

I'm not certain that this will work correctly.
OTOH, that's such a niche case maybe we've decided it doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see what you mean. Indeed, I think that won't really work as intended. The comment I wrote in that section:

// TODO: this is a bit overzealous in recalculating the IME cursor area: there are
// cases where it can cheaply be known the area is unchanged. (If the calculated area
// is unchanged from the previous one sent, it is not re-sent to the platform, though).
//
// Ideally this is called only when the layout, selection, or preedit has changed, or
// if the IME has just been enabled.

(Clearly it's not only overzealous, it's also slightly buggy.)

Maybe the new PlainEditor should make it cheap to know whether the layout changed. I'd prefer calling out to the platform as conservatively as possible, as there's no control over the cost of those calls.

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