-
Notifications
You must be signed in to change notification settings - Fork 932
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
Add android ime support #2993
Add android ime support #2993
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on that, even though I'm not familiar with android, but I do use IME with winit and would like to see it improved.
I don't like that we have a separate event for no real reason and that we have IME separate with your approach, which shouldn't be the case.
Could you look at https://wayland.app/protocols/text-input-unstable-v3#zwp_text_input_v3:request:set_surrounding_text and https://wayland.app/protocols/text-input-unstable-v3#zwp_text_input_v3:event:preedit_string ?
I think it's pretty much what you've added, except for the compose
key? We could extent the Preedit
API if it matters or just adjust IME
API to better account for Android here.
In general, if you'd need help with other platforms I could implement some of them (Wayland/macOS/X11/and maybe Windows).
src/window.rs
Outdated
/// Opens the IME input (soft keyboard) if the platform supports it. | ||
/// Currently only supported on Android. | ||
#[inline] | ||
pub fn begin_ime_input(&self) { | ||
self.window.begin_ime_input(); | ||
} | ||
|
||
/// Hides the IME input (soft keyboard). | ||
#[inline] | ||
pub fn end_ime_input(&self) { | ||
self.window.end_ime_input(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks redundant. We have set_ime_allowed
where true
means that the soft board will be opened, because we allow ime. false
will hide the IME, this works the same on Wayland btw and its softboards, so this API is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, egui just calls set_ime_allowed once on application start, I think that confused me. I've changed this in my egui branch and removed the begin_ime_input and end_ime_input functions
src/window.rs
Outdated
pub fn set_text_input_state(&self, state: TextInputState) { | ||
self.window.set_text_input_state(state); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you get a bit more details on what this particular function tries to do? In general, that's true that we'd nee a way to set an IME state, like position of the ime, surrounding text, etc.
I think what we probably should do is to have a set_ime_state(state: Option<ImeState>)
where you can set that information + position of the input method, content hint, and similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced this with a set_ime_surrounding_text function similar to the one from wayland you sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does android have a notion of atomic state changes? Because wayland has for example, and using some big InputState
would be preferred for it, but I can refactor it myself later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean, but I'd guess not. The android game text input library this relies on is quite simple. The idea is that this is called once when a text field is first focused and when the selection gets updated by the user.
Thanks for the review @kchibisov! |
I'm just saying that nothing stops us from extending the existing event, especially given that what you've added looks similar to it. |
I've updated my code and added a Ime::Replace event. It looks pretty similar to the Preedit event but works differently: When this event is called the whole text (the one that was previously set with set_surrounding_text) should be replaced with the updated text. I can't use the Preedit and Commit events, since the android game text input has no such thing as a commit. Whenever the user enters text, we get the complete updated text and a compose and selection region. One could maybe estimate whether a commit has happened based on the existence of a compose region and by diffing the updated text but I don't think that'd work reliably. |
On Wayland we have a similar concept, it's called https://wayland.app/protocols/text-input-unstable-v3#zwp_text_input_v3:event:delete_surrounding_text . Besides, users can't rely on the replace event alone, we need The key thing here is that Basically you should send However, maybe Android should |
I cloud in theory do something like this, basically diff the current and previous TextInputState to guess whether there should be a preedit or commit event: let events = if let Some(compose_region) = &ime_state.compose_region {
vec![event::Ime::Preedit(ime_state.text[compose_region.start..compose_region.end].to_string(), Some((compose_region.start, compose_region.end)))]
} else {
if let Some(previous_text_input_state) = &self.previous_text_input_state {
if let Some(prev_compose_region) = &previous_text_input_state.compose_region {
vec![
event::Ime::Preedit("".to_string(), None),
event::Ime::Commit(previous_text_input_state.text[prev_compose_region.start..prev_compose_region.end].to_string()),
]
} else {
vec![]
}
} else {
vec![]
}
}; But I don't think this would ever work reliably, no matter how many edgecases are handled. That's why I added the Replace event, which would replace the Commit event for android. If the application doesn't handle the Replace event, it won't have keyboard input support on android. Maybe Replace is a bad name, I guess it could be called CommitReplace or something like that. Since this event would likely be android specific, maybe it could also be a android exclusive platform extension, if there is such a thing for events?
That would be an idea. While typing the whole text could be sent as a Preedit. This would mean the whole text is highlighted while typing though, instead of the current compose region. Also when the keyboard is hidden the text field has already lost focus, so it would probably not get the final commit event and the text would be dismissed?
I see how that would be a problem. I think if you don't set any keyboard flags(disable autocomplete/autocorrect), you should reliably be able to get the last typed character from the end of the string in the Ime::Replace event. By comparing the length you could see if the user pressed backspace. But that would also be a horrible hack. I have no better idea though. |
The issue with the
|
Hm, maybe we could do that differently, could you compute the difference to replace based on the surrounding text provided by user, send replace event telling it to The only issue I think is the selection or something like that, but it probably should be transparent to the user? It just looks like the
|
pub fn set_ime_allowed(&self, allowed: bool) { | ||
if allowed { | ||
self.app.show_soft_input(true); | ||
} else { | ||
self.app.hide_soft_input(true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you send this change separately? I can send it myself though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make a separate PR with this change, it won't be very useful without the rest of these changes though, since android unfortunately doesn't send key events for the soft keyboard (See the first note here). So the keyboard would open but nothing will happen when the user starts typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think it does so with the game activity and recent changes merged here #3004 , no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, those changes are only relevant to Hardware Keyboards (e.g. usb keyboards plugged in your phone), but maybe I'm missing something. @rib can you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unicode mapping support from #3004 is also for soft keyboards which also generate KeyEvents, though I do tend to think of IME vs KeyEvents handling as two entirely separate input handling modes (and that's the case for other platforms too) and I'm not sure that it'd be ideal to make this a common API for those separate modes.
Overloading set_ime_allowed
for showing the keyboard in situations where you are happy to handle KeyEvents
instead of IME events seems like it could lead to non-portable code because calling this (in the same app) for other platforms will enable IME handling / events which might not be desirable.
It seems like ideally there should be a separate API for being able to show/hide mobile soft keyboards which would have no effect on platforms that don't need a virtual keyboard (but they may otherwise have IME support that the app is not trying to enable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, on Wayland it's tied to the set_ime_allowed
as well, and it's tied to that on the protocol level though. It's not written in the spec, but that's the defacto how it works. I guess if you don't set surrounding text you won't get any sort of ime-ish input, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern isn't so much whether you'd get IME events on Android, (GameActivity doesn't differentiate whether you've set some prior text or not, so you would get IME events on Android) but that you'd be enabling IMEs on other platforms when you don't necessarily want that (if your app is cross-platform).
If you want to write portable code that currently works in terms Key events (e.g. for work our engine doesn't currently have full IME support) then you may want to be able to show a virtual keyboard at times on Android (and handle corresponding Key events) but wouldn't want to enable IMEs generally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rib you enable IME when you write text, on Android you can't really write text without a soft keyboard. The same applies for others, if you write text you generally want an IME enabled, unless you're using the keyboard input where any sort of IME input is disallowed.
Like for example a game using soft keyboard must provide me a way to input IME, because it's likely some field and what if I want to write some script?
On desktop you just sometimes want to remove IME when you actually game, but not chat, which is not the case on Android because you're using on screen controls.
On macOS for example not enabling IME won't let you type in some cases anything meaningful.
Also, enabling IME doesn't mean that you'll receive only IME events, you could still receive regular keys, it's just you indicate that you can receive extra IME events and you can provide a feedback for IME when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm just cautious about conflating the two different use cases because I'm aware that it would affect our engine currently since we don't have any IME support right now but do show/hide the virtual keyboard on Android.
It might be OK (or at least it wouldn't be too tricky to add a cfg guard for android if necessary).
If we still get Key events on other platforms while IME is enabled (that's the case on Android) that probably generally makes this less of an issue but for desktop IMEs you would normally specify the position of the overlay window and we currently don't have any support for doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is also for soft keyboards which also generate KeyEvents
I just tried this, and none of the soft keyboards I have on my phone triggered any key events, only the TextEvent is triggered.
Yeah, something like that could work. I could use the Delete Event to delete the current text and use commit to replace it with the newly received text. Then we would need a new event to update the cursor and compose position though, or else the cursor would always be shown at the end of the text. I could give this a go. In the end, this solution would still require a non standard event (update cursor position, haven't found anything like that in the wayland events) to be added though, so I'm not sure if this is better than the replace solution. I tried to implement a diffing algorithm, but I haven't gotten it to work. Since the android keyboard could in theory randomly replace text independent of the cursor position, I feel like a solution that uses diffing would always have some edge cases and bugs. @rib do you maybe have an idea how this could be solved? I think you originally also tried to use the existing ime events? |
We can commit the cursor along side the commit, it's not a big deal. You also don't have to diff actually, you could just delete everything that was before and commit new, so it's exactly like replace, you just send 2 events. |
@kchibisov I removed the Replace event and added a DeleteSurroundingText event, this seems to work well. Is this what you had in mind? I think it might actually also be possible to use the Preedit event instead of Surrounding Text, if I set the text to "" and the range to 0 and usize::MAX, the application should be deleting the current text, if I understand correctly? But this seems really hacky to me. |
Sorry for the delay following up here. I do generally agree with the direction of trying to work with the Winit Ime events if we can make it work (based on adding a DeleteSurroundingText event). It's unfortunate that the GameActivity interface we get doesn't give us more fine grained events but the way Android's InputConnection works isn't totally dissimilar to other IME protocols so I feel like we should try to avoid leaking the current limitations of GameActivity if at all possible. It would be nice too if it's not necessary for a toolkit like Egui to have to treat Android as a special case when it comes to input method handling.
Curious that you see no KeyEvents at all. I certainly see KeyEvents for at least basic virtual keyboard input (essentially just ascii characters). We currently rely on these events in our engine while we haven't hooked up IME support yet but it seems like it may really depend on the implementation of different virtual keyboards as to what keys might generate KeyEvents. I had actually thought more keys would be supported so we could rely on KeyEvents as a stop gap for a little longer but yeah it looks like it's not really something that can be relied on - especially if some virtual keyboards in fact don't emit any KeyEvents :/ |
event::Ime::DeleteSurroundingText { | ||
before_length: usize::MAX, | ||
after_length: usize::MAX, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting surrounding text with usize::MAX might be a foot gun at some point.
IME edits are relative so the current cursor because conceptually they could be used within a large body of text (e.g. think 10k word Google Doc) so I'm a bit concerned that asking the toolkit to delete like this could potentially result in it deleting someone's larger document unless the toolkit additionally tracks the bounds of the surrounding context it has given Winit and treats that like a sandbox (but that's not currently clearly specified afik).
Maybe we can reliably track the size of the current text so we can more precisely delete just the text from the last update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that, doing delete all is sort of weird. Given that it's all about the feedback loop with setSurroundingText
request, maybe we can base everything on it, since
IME shouldn't touch anything outside of the user set surrounding text, no?
DeleteSurroundingText { | ||
before_length: usize, | ||
after_length: usize, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're exposing a selection region it seems like we need to clarify what the before/after are relative to.
On Android the before would be relative to the start of the selection and the after is relative to the end of the selection.
The Winit docs don't currently say whether we're following the Android notion that the "cursor" is just a special case of a selection where start == end.
With that model though we also need to consider how we will delete / update the selection region?
With the InputConnection API there is a separate setSelection that would make it possible to set the selection start==end for being able to delete all the text.
I'm currently thinking we might need to add a SetSelection
event for being able to just update the start/end indices of the selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the InputConnection API there is a separate setSelection that would make it possible to set the selection start==end for being able to delete all the text.
I'm currently thinking we might need to add a SetSelection event for being able to just update the start/end indices of the selection.
The selection should be mapped to the concept of the cursor, so it's effectively the same as in preedit. The Commit
could set the cursor as well, but I guess the virtual keyboard could move the cursor on its own? Yeah, it would be fine to have a separate method to advance the selection like you suggest, but we need to make it general on what selection is and probably encapsulate it into a separate type ImeSelection
or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides we should also communicate the Enabled/Disabled
state somehow, maybe we should just sent Enabled
initially and call it a day?
before_length: usize, | ||
after_length: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must specify the unit in which those are, because it's not clear whether it's a byte or unicode codepoint based indecies. Some API we have already is in bytes, not code points. So logically this API should be in bytes as well. Like for example Wayland uses bytes for that as well, and we can't really convert to code points in wayland case, because we simply don't know anything about the text around.
Maybe we should have a variant like enum Amount { Byte(usize), Codepoint(usize) }
, so the client can deal with all of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels odd to expose the offsets as bytes. I can sort of see why the wayland protocol would use bytes but forcing toolkits like Egui to have to somehow deal with byte offsets within Rust Strings sounds like it would be horrible to deal with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wayland backend should be able to map the byte offsets to the corresponding utf8 codepoint and can somehow treat it as an error / undefined behaviour if you ever get a byte offset into the middle of a utf8 character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rib it can't do so for delete surrounding text. Unless we stash surrounding text around. The rust string a sliced with bytes though, so it works pretty much ok.
/// If compose_region is Some, the compose region should be updated after replacing the | ||
/// text. | ||
compose_region: Option<(usize, usize)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand what the user should even do with that, our docs must be clear here, it's not something common on any other backend/ime stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android shows a underline under the word thats currently being edited. That is the compose region. But since no other platform has this (even iOS doesn't seem to have something like this) I think we could also remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Others usually have a preedit and the update is like pending to be inserted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Android's compose region is just different terminology for the "preedit" string.
The preedit string is the transient text that would be shown that may preview a tentative completion e.g. for kanji.
I think there are essentially three high level spans of text:
- the "surrounding" text
- the "preedit" text (or "compose region" for Android)
- the "selection" or "cursor" (empty selection)
I think the main thing that's changing here is that we need to introduce the notion that there is surrounding text which Winit didn't previously expose - though this isn't something unique to Android either.
Android has a slightly more general decoupling of the selection/cursor and the preedit string - while other IMEs might only track a cursor (can be considered an empty selection) or the cursor/selection will always be a subset of the preedit string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of the Winit public API we should probably rename "compose_region" here to "preedit_region" or "preedit_span" perhaps.
Conceptually this would be committing some surrounding text and also updating the span for the selection and the span for the preedit string in one event.
/// If selection is Some, the selection / cursor position should be updated after | ||
/// placing the `content`. | ||
selection: Option<(usize, usize)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably having a separate Selection
type at this point unified with Preedit
would be the way forward. Like the selection is used throughout the docs consistently, we should also decide on metrics we use and be explicit, I think on android it's unicode codepoints, but everything else we have is bytes?
Hey @lucasmerlin just wanted to let you know that I am resurrecting this work. I'm going to split off a version without the API changes first. I'll try to make sure the commits retain your attribution. :+ ) If you're still interested in this, I'll rope you in to the review. |
Awesome, good luck @xorgy! |
I tried to follow the discussion, what's the next steps here? I would be happy to help since I am currently using it for my android egui app. |
@jb55 Maybe a you could try to get basic text entry working with winit's existing api. There wouldn't be autocomplete support but it would be a first step and maybe enough for some apps. Also the people from linebender are very interested in android keyboard support, theres a long discussion here: https://xi.zulipchat.com/#narrow/stream/351333-glazier/topic/Comparisons.20against.20Winit I think @xorgy also looked into this but I'm not sure if he made any progress |
The API could be extended, it's just certain aspects should be shared. Like the actual insert to widget part should be shared, etc, so so users will write some kind of cross platform code. |
FYI with current implementation on Android you can not get input from non-english languages as keycodes, at Java side you can get them at Also I have problem with back button when keyboard appears after call of |
I have this now rebased (I can't update @lucasmerlin 's branch myself) But Android 11 (API 30) and later make it more painful to get the soft keyboard to show up, as others have discovered. |
So I have managed to get the soft keyboard to show up, but the only way I was able to do it was by calling into the screen-20240529-162246.mp4 |
And just to clarify, #3787 only opens the soft keyboard (which can send key events). Real IME on Android is looking like it will require specific support from the |
Fyi this is actively beeing prototyped and worked on. Will be in (It can also be hacked into a Rust app with the |
Nothing stops winit form using the |
I don't think |
This adds Support for opening the Soft Keyboard on android and passing through
TextInputState
events to the application.TextInputState contains the currently edited text as string, the current selection / cursor range and a optional compose region.
This enables applications to support text input on android including autocomplete and autocorrect.
While the TextInputState would currently be exclusive to android, I think it should be possible to implement the same api on iOS so there is a unified api for soft keyboards.
I implemented this as a new event because I think the current Ime event wouldn't work well for mobile text input, but it might also be possible to make it work with some tweaks. There would at least need to be some way to set the current text field content, so android has some context on what to predict for autocomplete.
I implemented this api for egui here: lucasmerlin/egui@d47677a
See it in action
Screen_Recording_20230708_144153.mp4
A disadvantage of this api is, that it'd probably be difficult if not impossible to reliably edit rich text with this. The alternative would be to implement some kind of shared abstraction over InputConnection and UITextInput but these apis are a lot more complicated and for android it'd be difficult to implement it with the game-activity approach.
I'm bad at naming things, so there is probably a better name for the event than
TextInputState
.Some other things missing to allow a great keyboard experience on mobile devices:
Part of #1823.
CHANGELOG.md
if knowledge of this change could be valuable to users