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

Add android ime support #2993

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -888,6 +888,19 @@ pub enum Ime {
/// Right before this event winit will send empty [`Self::Preedit`] event.
Commit(String),

/// Notifies when the complete text should be replaced.
/// This event should be used in combination with
/// [`Window::set_ime_surrounding_text`] to set the initial text of the
/// currently selected input field.
///
/// ## Platform-specific
/// This will only be fired on **Android**.
Replace {
text: String,
selection: (usize, usize),
compose_region: Option<(usize, usize)>,
},
Copy link
Contributor

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.

Copy link
Member

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.


/// Notifies when the IME was disabled.
///
/// After receiving this event you won't get any more [`Preedit`](Self::Preedit) or
Expand Down
39 changes: 38 additions & 1 deletion src/platform_impl/android/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,24 @@ impl<T: 'static> EventLoop<T> {
}
}
}
InputEvent::TextEvent(ime_state) => {
let event = event::Event::WindowEvent {
window_id: window::WindowId(WindowId),
event: event::WindowEvent::Ime(
event::Ime::Replace {
text: ime_state.text.to_string(),
selection: (ime_state.selection.start, ime_state.selection.end),
compose_region: ime_state.compose_region.map(|region| (region.start, region.end))
}
)
};
sticky_exit_callback(
event,
self.window_target(),
control_flow,
callback
);
}
_ => {
warn!("Unknown android_activity input event {event:?}")
}
Expand Down Expand Up @@ -901,10 +919,28 @@ impl Window {

pub fn set_ime_cursor_area(&self, _position: Position, _size: Size) {}

pub fn set_ime_allowed(&self, _allowed: bool) {}
pub fn set_ime_allowed(&self, allowed: bool) {
if allowed {
self.app.show_soft_input(true);
} else {
self.app.hide_soft_input(true);
}
}
Comment on lines +933 to +939
Copy link
Member

@kchibisov kchibisov Aug 7, 2023

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.


pub fn set_ime_purpose(&self, _purpose: ImePurpose) {}

pub fn set_ime_surrounding_text(&self, text: String, selection: (usize, usize)) {
self.app
.set_text_input_state(android_activity::input::TextInputState {
text,
selection: android_activity::input::TextSpan {
start: selection.0,
end: selection.1,
},
compose_region: None,
});
}

pub fn focus_window(&self) {}

pub fn request_user_attention(&self, _request_type: Option<window::UserAttentionType>) {}
Expand Down Expand Up @@ -987,6 +1023,7 @@ impl Window {
pub struct OsError;

use std::fmt::{self, Display, Formatter};

impl Display for OsError {
fn fmt(&self, fmt: &mut Formatter<'_>) -> Result<(), fmt::Error> {
write!(fmt, "Android OS Error")
Expand Down
4 changes: 4 additions & 0 deletions src/platform_impl/ios/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,10 @@ impl Inner {
warn!("`Window::set_ime_allowed` is ignored on iOS")
}

pub fn set_ime_surrounding_text(&self, _text: String, _selection: (usize, usize)) {
warn!("`Window::set_ime_surrounding_text` is ignored on iOS")
}

pub fn focus_window(&self) {
warn!("`Window::set_focus` is ignored on iOS")
}
Expand Down
3 changes: 3 additions & 0 deletions src/platform_impl/linux/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,9 @@ impl Window {
x11_or_wayland!(match self; Window(w) => w.set_ime_purpose(purpose))
}

#[inline]
pub fn set_ime_surrounding_text(&self, _text: String, _selection: (usize, usize)) {}

#[inline]
pub fn focus_window(&self) {
match self {
Expand Down
3 changes: 3 additions & 0 deletions src/platform_impl/macos/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1199,6 +1199,9 @@ impl WinitWindow {
#[inline]
pub fn set_ime_purpose(&self, _purpose: ImePurpose) {}

#[inline]
pub fn set_ime_surrounding_text(&self, _text: String, _selection: (usize, usize)) {}

#[inline]
pub fn focus_window(&self) {
let is_minimized = self.isMiniaturized();
Expand Down
3 changes: 3 additions & 0 deletions src/platform_impl/orbital/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@ impl Window {
#[inline]
pub fn set_ime_purpose(&self, _purpose: ImePurpose) {}

#[inline]
pub fn set_ime_surrounding_text(&self, _text: String, _selection: (usize, usize)) {}

#[inline]
pub fn focus_window(&self) {}

Expand Down
3 changes: 3 additions & 0 deletions src/platform_impl/web/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,9 @@ impl Window {
// Currently not implemented
}

#[inline]
pub fn set_ime_surrounding_text(&self, _text: String, _selection: (usize, usize)) {}

#[inline]
pub fn focus_window(&self) {
self.inner.dispatch(|inner| {
Expand Down
3 changes: 3 additions & 0 deletions src/platform_impl/windows/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,9 @@ impl Window {
#[inline]
pub fn set_ime_purpose(&self, _purpose: ImePurpose) {}

#[inline]
pub fn set_ime_surrounding_text(&self, _text: String, _selection: (usize, usize)) {}

#[inline]
pub fn request_user_attention(&self, request_type: Option<UserAttentionType>) {
let window = self.window.clone();
Expand Down
10 changes: 10 additions & 0 deletions src/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1111,6 +1111,16 @@ impl Window {
self.window.set_ime_purpose(purpose);
}

/// Sets the surrounding text for IME.
///
/// ## Platform-specific
/// - **Android**: should be set when a textfield is focused, so the keyboard has context
/// for autocomplete. If this is not set, the text will be cleared when the user starts typing.
/// - **iOS / Web / Windows / X11 / macOS / Orbital:** Unsupported.
pub fn set_ime_surrounding_text(&self, text: String, selection: (usize, usize)) {
self.window.set_ime_surrounding_text(text, selection);
}

/// Brings the window to the front and sets input focus. Has no effect if the window is
/// already in focus, minimized, or not visible.
///
Expand Down