Input method (soft keyboard) support

[rib]

Add input method support based on the GameTextInput library (only supported with game-activity currently)

This is based on discussion for #18 and here

[rib]

Trying to test this, it doesn't currently seem to be working as intended.

I tried modifying agdk-mainloop so that it handles touches to the top-left of the screen by showing (toggling) the onscreen keyboard and printing input method state updates.

I see an input method 'commit' for every key press for some reason and the attempt to initialize the text state with some "Lorem ipsum" text and a selection didn't seem to work.

Once the keyboard is visible then other input handling seems to somehow fall apart, like touch events are being grabbed.

I may just look at landing the APIs for showing/hiding the keyboard first and a change that ensures that android-activity owns the (extensible, '#[non_exhastive]) InputEvent type so then it will hopefully be possible to get this working afterwards without needing a semver bump.

[rib]

I ended up making incremental PRs for the show/hide_soft_input APIs and InputEvent enum as above so this updated PR just focuses on exposing a TextEvent and APIs for getting/setting the input method state.

[lucasmerlin]

During my testing, leaving end None and providing only start, caused a android exception, so I think having start and end just be usize and then Option in TextInputState might be better?

[rib]

I think I originally based this on these docs:

/**
 * This struct holds a span within a region of text from start (inclusive) to
 * end (exclusive). An empty span or cursor position is specified with
 * start==end. An undefined span is specified with start = end = SPAN_UNDEFINED.
 */
typedef struct GameTextInputSpan {
    /** The start of the region (inclusive). */
    int32_t start;
    /** The end of the region (exclusive). */
    int32_t end;
} GameTextInputSpan;

I think this issue is exactly what I got stuck on last time I was investigating this but didn't fully dig into the details.

Following the flow of data here...

• This data gets passed to GameActivity_setTextInputState with None mapped to -1 based on the above comment.

• GameActivity_setTextInputState passes this to GameTextInput_setState via a CMD_SET_SOFT_INPUT_STATE callback.

GameTextInput_setState calld GameTextInput::setState which does a JNI call to gametextinput.InputConnection.setState in Java (https://android.googlesource.com/platform/frameworks/opt/gamesdk/+/refs/heads/main/game-text-input/src/main/java/com/google/androidgamesdk/gametextinput/InputConnection.java#196)

setState is a utility that then effectively calls InputConnection.setSelection and InputConnection.setComposingRegion which are standard APIs documented here: https://developer.android.com/reference/android/view/inputmethod/InputConnection#setComposingRegion(int,%20int)

It looks like InputConnection is expected to clamp indices to being in-bounds and also not be fussy about wither start comes after end. At least the docs for setComposingRegions say "The passed indices are clipped to the contents bounds" which suggests it should be ok to pass None/-1 for the start/end indices.

Maybe the java GameTextInput code isn't validating its inputs properly.

E.g. looking at https://android.googlesource.com/platform/frameworks/opt/gamesdk/+/refs/heads/main/game-text-input/src/main/java/com/google/androidgamesdk/gametextinput/GameTextInput.java#69

It look like it should also be considering indices that are out of bounds because they are negative.

Or maybe only setComposingRegions is designed to clamp/clip the indices and you're expected to keep them in-bounds for setSelection.

Conceptually it looks like there is more of a special case for setComposingRegion which specifically recognises a start index of -1 and will map that to removeComposingRegion.

Maybe the comments that were written for GameTextInputSpan about using SPAN_UNDEFINED only really apply for the composing region.

That would maybe make sense considering that the selection becomes a cursor if the start/end are the same but you can't completely remove the selection and the cursor too.

As you suggested - moving the Options up a level probably makes sense.

I'm currently think that maybe only the compose_region should be optional though.

For the selection then if it was set to None then we'd have to arbitrarily choose to either put the cursor at the start of end I guess but it seems like that should be explicitly decided by the app probably.

[lucasmerlin]

My updated egui code looks a bit cleaner now, so I think this makes sense. I'm not 100% sure though on whether selection should be optional or not.

[rib]

Okey, I've rebased this on GameActivity 2.0.2 and I've made it so TextSpan just has usize start/end values but the composite_region is now an Option.

@lucasmerlin it'd be great if you're able to test this based on your Winit / egui changes.

[lucasmerlin]

My updated egui code looks a bit cleaner now, so I think this makes sense. I'm not 100% sure though on whether selection should be optional or not.

[rib]

I'm not 100% sure though on whether selection should be optional or not.

Yeah I'm not 100% sure about the handling of the optional spans.

Looking at the docs for InputConnection.setSelection and .setComposingRegion (which are basically what these spans are for) then technically neither of these APIs support a special case of an unset/undefined regions (they both just recognise when the start == end)

• https://developer.android.com/reference/android/view/inputmethod/InputConnection#setSelection(int,%20int)
• https://developer.android.com/reference/android/view/inputmethod/InputConnection#setComposingRegion(int,%20int)

so from that pov it could also be reasonable that the composing_region wouldn't be optional either but because the GameActivity-specific subclass implement's a special case that recognises start == -1 that's pretty much the only reason I can currently see for supporting an optional composing_region.

Tbh it seems odd to me that the GameActivity implementation of InputConnection has this special case which isn't documented in the context of GameTextInputState or GameTextInput_setState.

Passing None for the composing_region will result in editable.removeSpan() for the private / dummy editable that the GameActivity InputConnection has internally.

Considering that it's really just a placeholder editable it doesn't matter much I expect that this will remove the span.

I'm fairly sure that the Java implementation should also remove the span in case an empty region is specified, considering the SDK docs for setComposingRegion

If the resulting region is zero-sized, no region is marked and the effect is the same as that of calling finishComposingText().

So actually maybe the right thing to do in android-activity is to recognise zero-sized composing regions ourselves and then normalize that into a -1,-1 region, which will result in editable.removeSpan(), which looks like a better interpretation of what the connection should be doing anyway.

This would mean we can remove the Option for the composing_region too.

[rib]

Although it could make sense for the composing_region to be non-optional when setting the region, it would be awkward when querying the state because it needs to be optional when reading the state since the start/end will be -1 if no region has been set yet or if it's is cleared.

It would probably still be best to represent an unset composing region with None instead of picking an arbitrary start==end pair.

[rib]

oh, oops, sorry I just re-requested review @lucasmerlin but I forgot I was going to change it so we check for an empty compose region in Rust and normalize that as -1,-1 - will do that now

[rib]

oh, oops, sorry I just re-requested review @lucasmerlin but I forgot I was going to change it so we check for an empty compose region in Rust and normalize that as -1,-1 - will do that now

okey, done

I'm hoping that if you're perhaps able to smoke test this with your winit/egui branch @lucasmerlin then we can perhaps land this so we can potentially look at getting the corresponding winit changes into 0.29 if lucky

[lucasmerlin]

Works perfectly now!
Only missing thing now is some way to set the keyboard flags, but I think that can be done in a separate PR.
I currently use these for Autocorrect + Sentence capitalization:

            ffi::GameActivity_setImeEditorInfo(activity, 1 | 16384, 0, 0);

[rib]

Cool, thanks for checking, and yeah we can look at adding API for keyboard flags via a follow up PR

[lucasmerlin]

I've opened draft PRs to implement this for winit and egui