Upgrade to ndk-sys 0.6.0 and ndk 0.9.0
#155
Conversation
MarijnS95
force-pushed
the
ndk-0.9
branch
2 times, most recently
from
f2d17c5 to
7f8e2b1
Compare
MarijnS95
changed the title
ndk crate to 0.9.0ndk-sys 0.6.0 and ndk 0.9.0
The next breaking `ndk` release puts a lot of emphasis in improving `enum`s to finally be marked `non_exhaustive`, and carry possible future values in `__Unknown(i32)` variants. This removes the lossy conversions that previously required `android-activity` to redefine its types, which could all be removed again. The `repr()` types have also been updated, as `enum` constants in C are translated to `u32` by default in `bindgen` even though they're commonly passed as `int` to every API function that consumes them.
| @@ -507,6 +507,7 @@ impl<'a> InputIteratorInner<'a> { | |||
| ndk::event::InputEvent::KeyEvent(e) => { | |||
| input::InputEvent::KeyEvent(input::KeyEvent::new(e)) | |||
| } | |||
| _ => todo!("NDK added a new type"), | |||
There was a problem hiding this comment.
hmmm, we probably shouldn't panic here.
It would be a backwards compatible change to extend the ndk enum, and if we don't know about some new event in android-activity then we should skip calling callback()
I guess for now lets be lazy and just keep this todo!() though since it's maybe a bit of a faff to rejig to be able to skip the callback() but still call queue.finish_event()
and it would presumably make sense to handle new events here but without knowing what kind of event might be added it's not obvious that a panic is reasonable here.
There was a problem hiding this comment.
Yeah fair enough, we're currently only mapping Key and Motion here, and disregarding Focus/Capture/Drag/TouchMode. Those don't have any custom functions, I'm not sure if they're used at all or only with the very limited generic AInputEvent_ functions that provide next to no info?
| let value = value as i32; | ||
| self.ndk_pointer.axis_value(value.into()) |
There was a problem hiding this comment.
By the way, I wondered if maybe the repr type should have been fixed as game-activity is converting them as well. That matches what I did in the NDK to match the enum type to the function(s) that consume/generate it, not to the default u32 that happens to be assigned to untyped enums.
At some point that'll be fixed upstream anyway as they're retroactively fixing things to be enum foo : int32_t now that the NDK is always compiled with clang.
There was a problem hiding this comment.
But if I read things like this in android-activity again:
// XXX: we use `AInputEvent_getSource` directly (instead of calling
// ndk_event.source()) since we have our own `Source` enum that we
// share between backends, which may also capture unknown variants
// added in new versions of Android.It feels like most if not all enums that are redefined are not to capture differences with game-activity, but merely because the ndk crate was lagging behind Android (what the comment literally says).
Instead of tediously spending a lot of time doing the same "type rectification" here, I'd rather extend the ndk with all the missing variants (which we're now allowed to do non-breaking thanks to using the exact same #[non_exhaustive] enum setup as android-activity), compare with android-activity to make sure we're not missing out, and finally reduce some NIH :)
The next breaking
ndkrelease puts a lot of emphasis in improvingenums to finally be markednon_exhaustive, and carry possible future values in__Unknown(i32)variants. This removes the lossy conversions that previously requiredandroid-activityto redefine its types, which could all be removed again.The
repr()types have also been updated, asenumconstants in C are translated tou32by default inbindgeneven though they're commonly passed asintto every API function that consumes them.