Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
17 changes: 16 additions & 1 deletion src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,22 @@ pub enum Ime {
/// Notifies when text should be inserted into the editor widget.
///
/// Right before this event winit will send empty [`Self::Preedit`] event.
Commit(String),
Commit {
content: String,
/// If selection is Some, the selection / cursor position should be updated after
/// placing the `content`.
selection: Option<(usize, usize)>,
Comment on lines +891 to +893
Copy link
Member

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?

/// If compose_region is Some, the compose region should be updated after replacing the
/// text.
compose_region: Option<(usize, usize)>,
Comment on lines +894 to +896
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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:

  1. the "surrounding" text
  2. the "preedit" text (or "compose region" for Android)
  3. 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.

Copy link
Contributor

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.

},

/// Notifies when the text around the cursor should be deleted.
/// If `before_length` and `after_length` are [usize::MAX], the entire text should be deleted.
DeleteSurroundingText {
before_length: usize,
after_length: usize,
Comment on lines +902 to +903
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member

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.

},
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.
///
Expand Down
50 changes: 49 additions & 1 deletion src/platform_impl/android/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,35 @@ impl<T: 'static> EventLoop<T> {
}
}
}
InputEvent::TextEvent(ime_state) => {
let events = [
// Send a preedit event so the application knows to expect a commit event
event::Ime::Preedit("".to_string(), None),
// Delete all of the current text
event::Ime::DeleteSurroundingText {
before_length: usize::MAX,
after_length: usize::MAX,
},
Copy link
Contributor

@rib rib Aug 10, 2023

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?

Copy link
Member

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?

// Replace the previously deleted text with our updated text, and set the cursor and compose region
event::Ime::Commit {
content: ime_state.text.to_string(),
selection: Some((ime_state.selection.start, ime_state.selection.end)),
compose_region: ime_state.compose_region.map(|region| (region.start, region.end)),
}
];

events.into_iter().for_each(|event| {
sticky_exit_callback(
event::Event::WindowEvent {
window_id: window::WindowId(WindowId),
event: event::WindowEvent::Ime(event),
},
self.window_target(),
control_flow,
callback,
);
});
}
_ => {
warn!("Unknown android_activity input event {event:?}")
}
Expand Down Expand Up @@ -901,10 +930,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 +1034,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
11 changes: 8 additions & 3 deletions src/platform_impl/linux/wayland/seat/text_input/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,14 @@ impl Dispatch<ZwpTextInputV3, TextInputData, WinitState> for TextInputState {

// Send `Commit`.
if let Some(text) = text_input_data.pending_commit.take() {
state
.events_sink
.push_window_event(WindowEvent::Ime(Ime::Commit(text)), window_id);
state.events_sink.push_window_event(
WindowEvent::Ime(Ime::Commit {
content: text,
selection: None,
compose_region: None,
}),
window_id,
);
}

// Send preedit.
Expand Down
6 changes: 5 additions & 1 deletion src/platform_impl/linux/x11/event_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,11 @@ impl<T: 'static> EventProcessor<T> {

let event = Event::WindowEvent {
window_id,
event: WindowEvent::Ime(Ime::Commit(written)),
event: WindowEvent::Ime(Ime::Commit {
content: written,
selection: None,
compose_region: None,
}),
};

self.is_composing = false;
Expand Down
6 changes: 5 additions & 1 deletion src/platform_impl/macos/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,11 @@ declare_class!(
// Commit only if we have marked text.
if self.hasMarkedText() && self.is_ime_enabled() && !is_control {
self.queue_event(WindowEvent::Ime(Ime::Preedit(String::new(), None)));
self.queue_event(WindowEvent::Ime(Ime::Commit(string)));
self.queue_event(WindowEvent::Ime(Ime::Commit {
content: string,
selection: None,
compose_region: None,
}));
self.state.ime_state.set(ImeState::Commited);
}
}
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
6 changes: 5 additions & 1 deletion src/platform_impl/orbital/event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,11 @@ impl<T: 'static> EventLoop<T> {
});
event_handler(event::Event::WindowEvent {
window_id: RootWindowId(window_id),
event: event::WindowEvent::Ime(Ime::Commit(character.into())),
event: event::WindowEvent::Ime(Ime::Commit {
content: character.into(),
selection: None,
compose_region: None,
}),
});
}
EventOption::Mouse(MouseEvent { x, y }) => {
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
12 changes: 10 additions & 2 deletions src/platform_impl/windows/event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1323,7 +1323,11 @@ unsafe fn public_window_callback_inner<T: 'static>(
});
userdata.send_event(Event::WindowEvent {
window_id: RootWindowId(WindowId(window)),
event: WindowEvent::Ime(Ime::Commit(text)),
event: WindowEvent::Ime(Ime::Commit {
content: text,
selection: None,
compose_region: None,
}),
});
}
}
Expand Down Expand Up @@ -1363,7 +1367,11 @@ unsafe fn public_window_callback_inner<T: 'static>(
});
userdata.send_event(Event::WindowEvent {
window_id: RootWindowId(WindowId(window)),
event: WindowEvent::Ime(Ime::Commit(text)),
event: WindowEvent::Ime(Ime::Commit {
content: text,
selection: None,
compose_region: None,
}),
});
}
}
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