-
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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:?}") | ||
} | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. Well, on Wayland it's tied to the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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>) {} | ||
|
@@ -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") | ||
|
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.
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 typeImeSelection
or something.