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

Rework input_events API and expose KeyCharacterMap bindings #102

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

rib
Copy link
Collaborator

@rib rib commented Aug 3, 2023

(Note: I've marked as draft while I still need to update the docs more)

With the way events are delivered via an InputQueue with NativeActivity there is no direct access to the underlying KeyEvent and MotionEvent Java objects and no ndk API that supports the equivalent of KeyEvent.getUnicodeChar()

What getUnicodeChar does under the hood though is lookup into a KeyCharacterMap for the corresponding InputDevice based on the event's key_code and meta_state - which we can do via some JNI bindings for KeyCharacterMap.

Although it's still awkward to expose an API like
key_event.get_unicode_char() we can instead provide an API that lets you look up a KeyCharacterMap for any device_id and applications can then use that for character mapping.

This approach is also more general than the getUnicodeChar utility since it exposes other useful state, such as being able to check what kind of keyboard input events are coming from (such as a full physical keyboard vs a virtual / 'predictive' keyboard)

For consistency this exposes the same API through the game-activity backend, even though the game-activity backend is technically able to support unicode lookups via getUnicodeChar (since it has access to the Java KeyEvent object).

This highlighted a need to be able to use other AndroidApp APIs while processing input, which wasn't possible with the .input_events() API design because the AndroidApp held a lock over the backend while iterating events.

This changes input_events() to input_events_iter() which now returns a form of lending iterator and instead of taking a callback that gets called repeatedly by input_events() a similar callback is now passed to iter.next(callback).

Code that iterates events now looks something like:

match app.input_events_iter() {
    Ok(mut iter) => {
        loop {
            let read_input = iter.next(|event| {
                let handled = match event {
                    InputEvent::KeyEvent(key_event) => {
                        // Snip
                    }
                    InputEvent::MotionEvent(motion_event) => {
                        // Snip
                    }
                    event => {
                        // Snip
                    }
                };

                handled
            });

            if !read_input {
                break;
            }
        }
    }
    Err(err) => {
        log::error!("Failed to get input events iterator: {err:?}");
    }
}

Code to handle unicode character mapping, including handling dead key handling would look something like:

let mut combining_accent = None;
// Snip


let combined_key_char = if let Ok(map) = app.device_key_character_map(device_id) {
    match map.get(key_event.key_code(), key_event.meta_state()) {
        Ok(KeyMapChar::Unicode(unicode)) => {
            let combined_unicode = if let Some(accent) = combining_accent {
                match map.get_dead_key(accent, unicode) {
                    Ok(Some(key)) => {
                        info!("KeyEvent: Combined '{unicode}' with accent '{accent}' to give '{key}'");
                        Some(key)
                    }
                    Ok(None) => None,
                    Err(err) => {
                        log::error!("KeyEvent: Failed to combine 'dead key' accent '{accent}' with '{unicode}': {err:?}");
                        None
                    }
                }
            } else {
                info!("KeyEvent: Pressed '{unicode}'");
                Some(unicode)
            };
            combining_accent = None;
            combined_unicode.map(|unicode| KeyMapChar::Unicode(unicode))
        }
        Ok(KeyMapChar::CombiningAccent(accent)) => {
            info!("KeyEvent: Pressed 'dead key' combining accent '{accent}'");
            combining_accent = Some(accent);
            Some(KeyMapChar::CombiningAccent(accent))
        }
        Ok(KeyMapChar::None) => {
            info!("KeyEvent: Pressed non-unicode key");
            combining_accent = None;
            None
        }
        Err(err) => {
            log::error!("KeyEvent: Failed to get key map character: {err:?}");
            combining_accent = None;
            None
        }
    }
} else {
    None
};

The API isn't as ergonomic as I would have liked, considering that lending iterators aren't a standard feature for Rust yet but also since we still want to have the handling for each individual event go via a callback that can report whether an event was "handled". I think the slightly awkward ergonomics are acceptable though considering that the API will generally be used as an implementation detail within middleware frameworks like Winit.

Since this is the first example where we're creating non-trivial Java bindings for an Android SDK API this adds some JNI utilities and establishes a pattern for how we can implement a class binding.

There is now a public Error and Result type that can convey JNI errors (but without exposing any jni-rs types in the public API) as well as failures to get an input iterator (which could happen if an app attempted to get more than one iterator at the same time).

It's an implementation detail but with how I wrote the binding I tried to keep in mind the possibility of creating a procmacro later that would generate some of the JNI boilerplate involved.

@rib rib requested a review from MarijnS95 August 3, 2023 11:44
@rib rib force-pushed the rib/pr/input-api-rework-with-key-character-maps branch from 6a986fe to 57e506a Compare August 3, 2023 11:46
@rib
Copy link
Collaborator Author

rib commented Aug 3, 2023

Cc: @lucasmerlin it could be good if you'd also be able to take a look at this, considering the overlap with the recent GameTextInput changes.

@rib
Copy link
Collaborator Author

rib commented Aug 3, 2023

Although this is a fairly big change I'm really hoping to land this relatively quickly ideally so we can land corresponding support in Winit 0.29 🤞 (currently Winit just has a hacky mapping from raw key codes to characters which is embarrassingly limited).

rib added a commit that referenced this pull request Aug 3, 2023
- Lets us build with cargo ndk 3+
- Lets us remove suppression for false-negative clippy warning about unsafe
  blocks in unsafe functions
- Should unblock CI for #102

- 1.68.0 notably also builds the standard library with a newer r25 NDK
  toolchain which avoid the need for awkward libgcc workarounds, so it's
  anyway a desirable baseline for Android projects.
@rib rib mentioned this pull request Aug 3, 2023
@rib rib force-pushed the rib/pr/input-api-rework-with-key-character-maps branch 2 times, most recently from ef335b7 to 62f3158 Compare August 4, 2023 20:57
@rib rib changed the base branch from main to rib/pr/revert-msrv-bump-for-winit August 4, 2023 20:58
@rib rib force-pushed the rib/pr/input-api-rework-with-key-character-maps branch 2 times, most recently from ae546f6 to bb0f1af Compare August 7, 2023 15:46
@rib rib changed the base branch from rib/pr/revert-msrv-bump-for-winit to rib/pr/agdk-mainloop-update-v2.0.2 August 7, 2023 15:46
@lucasmerlin
Copy link

I had a look over the changes and everything seems good 👍 I am not a keyboard input expert though.

@rib rib force-pushed the rib/pr/input-api-rework-with-key-character-maps branch from bb0f1af to cb85782 Compare August 7, 2023 17:25
@rib rib force-pushed the rib/pr/agdk-mainloop-update-v2.0.2 branch from a2fe446 to d0f10a0 Compare August 7, 2023 17:25
@rib
Copy link
Collaborator Author

rib commented Aug 7, 2023

Cool, thanks for taking a look @lucasmerlin

I've just pushed some documentation updates and also renamed get_dead_key to get_dead_char to match the underlying Android SDK API.

I'm currently planning to merge once it passes CI

@rib rib marked this pull request as ready for review August 7, 2023 17:28
@rib rib force-pushed the rib/pr/input-api-rework-with-key-character-maps branch from cb85782 to 26b450d Compare August 7, 2023 17:33
With the way events are delivered via an `InputQueue` with
`NativeActivity` there is no direct access to the underlying KeyEvent
and MotionEvent Java objects and no `ndk` API that supports the
equivalent of `KeyEvent.getUnicodeChar()`

What `getUnicodeChar` does under the hood though is to do lookups into a
`KeyCharacterMap` for the corresponding `InputDevice` based on the
event's `key_code` and `meta_state` - which are things we can do via
some JNI bindings for `KeyCharacterMap`.

Although it's still awkward to expose an API like
`key_event.get_unicode_char()` we can instead provide an API that
lets you look up a `KeyCharacterMap` for any `device_id` and
applications can then use that for character mapping.

This approach is also more general than the `getUnicodeChar` utility
since it exposes other useful state, such as being able to check what
kind of keyboard input events are coming from (such as a full physical
keyboard vs a virtual / 'predictive' keyboard)

For consistency this exposes the same API through the game-activity
backend, even though the game-activity backend is technically able to
support unicode lookups via `getUnicodeChar` (since it has access to the
Java `KeyEvent` object).

This highlighted a need to be able to use other `AndroidApp` APIs while
processing input, which wasn't possible with the `.input_events()` API
design because the `AndroidApp` held a lock over the backend while
iterating events.

This changes `input_events()` to `input_events_iter()` which now returns
a form of lending iterator and instead of taking a callback that gets
called repeatedly by `input_events()` a similar callback is now passed
to `iter.next(callback)`.

The API isn't as ergonomic as I would have liked, considering that
lending iterators aren't a standard feature for Rust yet but also since
we still want to have the handling for each individual event go via a
callback that can report whether an event was "handled". I think the
slightly awkward ergonomics are acceptable though considering that
the API will generally be used as an implementation detail within
middleware frameworks like Winit.

Since this is the first example where we're creating non-trivial Java
bindings for an Android SDK API this adds some JNI utilities and
establishes a pattern for how we can implement a class binding.

It's an implementation detail but with how I wrote the binding I tried
to keep in mind the possibility of creating a procmacro later that would
generate some of the JNI boilerplate involved.
@rib rib force-pushed the rib/pr/input-api-rework-with-key-character-maps branch from 26b450d to af331e3 Compare August 7, 2023 17:38
@rib rib changed the base branch from rib/pr/agdk-mainloop-update-v2.0.2 to main August 7, 2023 17:38
@rib rib merged commit b4cf0ee into main Aug 7, 2023
3 checks passed
@rib rib mentioned this pull request Aug 7, 2023
@rib rib deleted the rib/pr/input-api-rework-with-key-character-maps branch August 7, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants