linebender / parley

Rich text layout library
Apache License 2.0
228 stars 28 forks source link

parley: add IME support to text editor example #111

Open tomcur opened 2 months ago

tomcur commented 2 months ago

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.

https://github.com/user-attachments/assets/863677fd-111c-4d39-8e62-b791113a3d68

tomcur commented 2 months ago

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 commented 2 months ago

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

I implemented this very carefully in https://github.com/linebender/glazier/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

tomcur commented 2 months ago

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.)

tomcur commented 2 months ago

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.)

https://github.com/user-attachments/assets/982978ff-6a0b-4acd-91a2-650ce00c798f

xorgy commented 2 months ago

(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.

tomcur commented 2 months ago

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 commented 2 months ago

This is running into https://github.com/linebender/vello/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

tomcur commented 1 month ago

@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 commented 1 month ago

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.