rust-mobile / android-activity

Glue for building Rust applications on Android with NativeActivity or GameActivity
251 stars 49 forks source link

Seal the game-activity `KeyEvent` type #91

Closed rib closed 1 year ago

rib commented 1 year ago

These changes for Winit's Android backend that poked right into the backend-specific implementation details of KeyEvent were very surprising to me: https://github.com/rust-windowing/winit/commit/918430979f8219648daade44796c00893e42fdd8#diff-9169a22d6397a250be741006cd857b8a575f804c74cc07e3a4fb8f3341606d9b

Including an unsafe pointer cast that just flat out assumed how the type is implemented with the native-activity backend!?

It was certainly not my intent that it would be possible to dig into these kinds of backend-specific details and although it's not possible to stop code from using unsafe transmutes/casts of pointer it was an oversight that the Deref implementation in the game-activity backend publicly exposes the GameActivityKeyEvent type.

Although it would technically be a backwards incompatible change to seal this, I do tend to think that the code in Winit master (which was quickly removed) was likely the only code in the wild to do this and so it's probably acceptable to seal this in a patch release.

That said - it's quite likely we need to plan for a breaking release soonish, e.g. to update our ndk dep.

MarijnS95 commented 1 year ago
                        // We abuse the fact that `android_activity`'s `KeyEvent` is `repr(transparent)`
                        let event = (key as *const android_activity::input::KeyEvent<'_>).cast::<ndk::event::KeyEvent>();
                        // We use the unsafe function directly because we want to forward the
                        // keycode value even if it doesn't have a variant defined in the ndk
                        // crate.
                        (
                            AKeyEvent_getKeyCode((*event).ptr().as_ptr()) as u32,
                            (*event).scan_code() as u32
                        )

Wow :)

MarijnS95 commented 1 year ago

That said - it's quite likely we need to plan for a breaking release soonish, e.g. to update our ndk dep.

Yup, let's just have a big breaking-release chain for all these crates to work through a bunch of these issues at once.