rename CompostionEvent to IMEPreeditEvent #1622

garasubo · 2020-07-10T07:34:56Z

To avoid confusion from users, rename CompositionEvent to IMEPreeditEvent to make it clear that those events are related to IME.
Also, added documents

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality
Updated feature matrix, if new features were added or implemented

Osspial

I’m in favor of renaming these events from CompositionEvent, but I don’t like the name IMEPreeditEvent. Is there anything wrong with just IMEEvent or ImeEvent?

garasubo · 2020-07-31T07:40:39Z

@Osspial it makes sense for me. Will rename them.

garasubo · 2020-08-14T05:52:26Z

@Osspial Could you check my change? I renamed them. One CI check was failed, but I think this is not related to this change.

kchibisov

I've rebased composition-event branch.

I also think that we can apply the proposed changes(about event restructure) from the issue I've mentioned in that PR, right now we only have X11 backend, so that's why I think it should be done here.

If you have any questions or concerns let me know.

kchibisov · 2020-10-10T17:36:09Z

src/event.rs

-    CompositionStart(String),
-    CompositionUpdate(String, usize),
-    CompositionEnd(String),
+pub enum PreeditState {


I think we can name it a bit differently, since it's not only about preedit state, since you have commit, etc. I'd also like to see adoption of

pub enum CompositionEvent { CompositionEnabled, CompositionPreedit(String, Option<usize>, Option<usize>), CompositionCommit(String), CompositionDisabled, }

That I've mentioned on #1497 if you don't have any objects, unfortunately I don't really know much about X11, so it's a bit hard for me to update its code myself.

Also we can name rename PreeditState to IMEEvent, and have

pub enum IMEEvent { Enabled, Preedit(String, Option<usize>, Option<usize>), Commit(String), Disabled, }

So I guess Start and Update should be transformed into Preedit, and End should be transformed into Commit.

Enabled and Disabled should be implemented, and I guess you have that information from X server, at least I can see something like that in xim docs.

I think you miss Start (or maybe PreeditStart?) event. Other than that, I think it looks fine.

I think you miss Start (or maybe PreeditStart?) event. Other than that, I think it looks fine.

Could you explain why PreeditStart is required? on Wayland for example everything starts with just Preedit.

In Wep API, CompositionStart exist in the specification. And in x11, every IME session starts with preedit start event. So, I believe something to tell us when a new IME session starts is necessary.

By start you mean that input method got open or just new input? Since Wayland has a concept of 'Enabled', when user uses its IME, like they opened 'fcitx' or 'ibus' and focused your app. But there's no notion of 'Start', and everything is communicated via 'Preedit' and 'Commit'.

My question was about start, since it looked like Preedit("", None, None) to me.

So, your suggestion is the application can detect the session start by checking the second argument is empty?

But why do you need to detect session start at all? The start is when you've got any IME event.

I mean, I don't understand how end users will handle Start differently to Preedit? Could you give an example where handling of those 2 are different and it makes sense? I get that it's common API for X11 and Web, but it really feels like someone decided to add redundant event that you can mirror with other events.

Hmm, I don't have an idea of specific usage for Start event for now, but Start event looks relatively common, so I'm afraid that there is some needs.
But if there is no special use case, it might be OK to combine them.

I mean, I don't understand how end users will handle Start differently to Preedit? Could you give an example where handling of those 2 are different and it makes sense?

An example I met before was, I need to know whether I should handle ReceiveCharacter or not. When I get PreeditStart, I can set a flag like isPreediting. If isPreediting is true, I will ignore ReceiveCharacter event.

(However, we can do it in winit though: when preediting, we don't trigger ReceiveCharacter.)

I think it may be useful when user need to do stuff depend on IME state.

If we do preedit we don't need to send receivedchar, since the data is being send via preedit.

I'm not sure, but there may be other events in the similar situation. For example, user may also want to ignore KeyboardInput when do preedit (IME will handle arrow keys and backspace, so user may want to ignore them). We can't take into account all the cases I suppose.

kchibisov · 2020-10-19T01:53:19Z

src/event.rs

+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub enum IMEEvent {
+    /// The user enables IME
+    Enabled,


Does X11 have an event to tell you when you actually have IME running or how to you obtain that information? Also does it tell you when disable happens, since if X11 doesn't have those events they should be issued at some points, since users may only handle IME things after you've actually got those, since IME is user initiated, I guess.

Or it's not the case for X11 and it just always works?

Oh, sorry. This is WIP commit. I think we can get those status with XIMPreeditStateNotifyCallback, so will implement that and test it

garasubo · 2020-10-23T08:35:10Z

I tried to implement Enable/Disable event, but it seems difficult in X11.
I assumed that we could implement them with XNPreeditStateNotifyCallback, but it doesn't work in my environment when I switched the IME.
I investigated some other projects, and found that in Vim, they don't implement since they couldn't find a good way in X11.
Ref: https://groups.google.com/g/vim_dev/c/9j76Z8hd23E?pli=1

@kchibisov do you think we should give up enable/disable event only on X11?

kchibisov · 2020-10-23T08:41:50Z

do you think we should give up enable/disable event only on X11?

Have you looked into something like that gtk on that? It seems strange that there's no indication that IME got opened, though, we can just always send enable when creating IME object and send Disabledon close or never send Disabled if it's too complicated to implement now. The point is to send Enabled, if it won't be optimal we won't lose anything, since right now downstream assumes that IME is always enabled.

The thing is that we must send it, but what we use to indicate that could be something not really optimal, if implementing it is something that complicated.

garasubo · 2020-10-26T13:23:49Z

Yeah, GTK doesn't support enable events either (and won't support XIM at all in GTK4). In X11, we don't destory IME object basically, so the current solution would be never sending Disable event and send Enable event at the beginning.

kchibisov · 2020-10-26T13:28:36Z

so the current solution would be never sending Disable event and send Enable event at the beginning.

Looks fine to me, I guess.

kchibisov · 2020-11-13T11:22:02Z

I've looked into things again, and I think we can go for now with Disabled/Enabled events tbqh, so just strip them form your impl and we should be good to go.

garasubo · 2020-11-13T15:39:57Z

Sorry for late response. I think I can update the change in this weekend.

kchibisov · 2020-11-16T14:05:17Z

Since you've kept enable and removed disable is that correct that there's no way to know that something got disabled? Also, would you mind rebasing the branch?

garasubo · 2020-11-16T14:34:43Z

Is that correct that there's no way to know that something got disabled?

Yes. Also, actually, it doesn't know IME enabled. It just sends enable event when it creates the context object, as we discussed before.

Also, would you mind rebasing the branch?

Sure. Will do it later

garasubo · 2020-11-18T13:00:38Z

Squash and rebased

kchibisov

You should also fix formatting. After that it should be good to go, I think.

kchibisov · 2020-11-18T13:06:32Z

src/event.rs

+    Preedit(String, Option<usize>, Option<usize>),
+
+    /// The user completes the current IME session with the value.
+    Commit(String),


New line here.

kchibisov · 2020-11-18T13:06:40Z

src/event.rs

-    CompositionEnd(String),
+pub enum IMEEvent {
+    /// The user enables IME
+    Enabled,


New line here.

kchibisov · 2020-11-18T13:13:07Z

src/event.rs

+    /// The value represents a pair of the preedit string and the cursor position.
+    /// The cursor position is byte-wise indexed.


We should document that the first argument is a cursor begin and second is cursor end. Also if your cursor being and end are equal you should send both of them the same, so in your code you should always send Some(position), Some(position)?

Also, both None should represent a hidden cursor, we should state it as well.

Added those description

kchibisov · 2020-11-18T13:13:59Z

src/platform_impl/linux/x11/event_processor.rs

-                            event: WindowEvent::Composition(CompositionEvent::CompositionUpdate(
-                                text, position,
-                            )),
+                            event: WindowEvent::IME(IMEEvent::Preedit(text, Some(position), None)),


Also, is there a way to inform that cursor is hidden, thus shouldn't be drawn, if there's such thing we should inform about that via sending None and None?

Suggested change

event: WindowEvent::IME(IMEEvent::Preedit(text, Some(position), None)),

event: WindowEvent::IME(IMEEvent::Preedit(text, Some(position), Some(position))),

Fixed.

In X11, I cannot find any event related to hidden cursor.

kchibisov

@garasubo I've decided to rename the event one more time, and call it IME, since event suffix seems redundant here. I've also slightly updated the docs, are you fine with proposed changes?

kchibisov · 2020-11-21T22:27:08Z

src/platform_impl/linux/x11/ime/context.rs

@@ -89,7 +89,7 @@ extern "C" fn preedit_draw_callback(
            .collect()
    };
    let mut old_text_tail = client_data.text.split_off(chg_range.end);
-    client_data.text.split_off(chg_range.start);
+    let _ = client_data.text.split_off(chg_range.start);


This is ignore is intentional right?

kaiwk · 2020-11-22T16:38:10Z

I tried to implement Enable/Disable event, but it seems difficult in X11.
I assumed that we could implement them with XNPreeditStateNotifyCallback, but it doesn't work in my environment when I switched the IME.
I investigated some other projects, and found that in Vim, they don't implement since they couldn't find a good way in X11.
Ref: https://groups.google.com/g/vim_dev/c/9j76Z8hd23E?pli=1

@kchibisov do you think we should give up enable/disable event only on X11?

From this guide:

If IME is available on focused elements, we call that state "enabled". If IME is not fully available(i.e., user cannot enable IME), we call this state "disabled".

Could we just simply think that when winit window gets focused, send Enabled and when window loses focus, send Disabled? I didn't find anything similar on Mac, except NSInputContext has activate/deactivate, but it is invoked by the system only.

kchibisov · 2020-11-22T18:07:09Z

Could we just simply think that when winit window gets focused, send Enabled and when window loses focus, send Disabled?

The problem is on Wayland it works differently, the system tells you that IME enabled only when you get Enabled event from the system, it's not related to a focus, and can be triggered without focus change, we can model that on X11 around a focus events if it's preferred.

So I wanted to expose this, since if you don't have text-input you won't ever get Enabled, thus there's no point in requesting ime updates all the time.

garasubo · 2020-11-23T02:44:06Z

@kchibisov Thanks. LGTM.

garasubo mentioned this pull request Jul 10, 2020

Tracking issue for IME / composition support #1497

Open

12 tasks

Osspial suggested changes Jul 15, 2020

View reviewed changes

kchibisov force-pushed the composition-event branch from d3bcee3 to 84f26b0 Compare October 10, 2020 16:42

kchibisov requested changes Oct 10, 2020

View reviewed changes

kchibisov mentioned this pull request Oct 10, 2020

add composition event support for Mac #1669

Closed

9 tasks

kchibisov reviewed Oct 19, 2020

View reviewed changes

rename events

d5220bb

garasubo force-pushed the composition-event_rename-events branch from 0684ebd to d5220bb Compare November 18, 2020 13:00

kchibisov requested changes Nov 18, 2020

View reviewed changes

garasubo added 2 commits November 20, 2020 21:33

improve fmt

73970c8

fmt

12d4cf1

kchibisov self-requested a review November 21, 2020 19:45

Rename IME event one more time

5a4cc4c

kchibisov requested changes Nov 21, 2020

View reviewed changes

kchibisov approved these changes Nov 23, 2020

View reviewed changes

kchibisov merged commit 65cf069 into rust-windowing:composition-event Nov 23, 2020

		/// The value represents a pair of the preedit string and the cursor position.
		/// The cursor position is byte-wise indexed.

	event: WindowEvent::IME(IMEEvent::Preedit(text, Some(position), None)),
	event: WindowEvent::IME(IMEEvent::Preedit(text, Some(position), Some(position))),

rename CompostionEvent to IMEPreeditEvent #1622

rename CompostionEvent to IMEPreeditEvent #1622

Conversation

garasubo commented Jul 10, 2020 • edited Loading

Osspial left a comment

Choose a reason for hiding this comment

garasubo commented Jul 31, 2020

garasubo commented Aug 14, 2020

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiwk Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiwk Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garasubo commented Oct 23, 2020

kchibisov commented Oct 23, 2020

garasubo commented Oct 26, 2020 • edited Loading

kchibisov commented Oct 26, 2020

kchibisov commented Nov 13, 2020

garasubo commented Nov 13, 2020

kchibisov commented Nov 16, 2020

garasubo commented Nov 16, 2020

garasubo commented Nov 18, 2020

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiwk commented Nov 22, 2020

kchibisov commented Nov 22, 2020

garasubo commented Nov 23, 2020

garasubo commented Jul 10, 2020 •

edited

Loading

kaiwk Dec 1, 2020 •

edited

Loading

kaiwk Dec 2, 2020 •

edited

Loading

garasubo commented Oct 26, 2020 •

edited

Loading