Closed MarijnS95 closed 7 months ago
Nice, this must have been quite a chore to go through and update - kudos!
Thanks! Besides reviewing the changes here, I hope you've also cross-checked that I didn't miss out on some enum
s?
Are you okay with the proposed idea of both having non_exhaustive
and a hidden __Unknown
?
I'll process the review ASAP and get this in to unblock many other changes, many thanks for the review!
I hope you've also cross-checked that I didn't miss out on some
enum
s?
I didn't, but can take a pass to cross check now...
Going through cross checking all enums, it could be good go add non_exhaustive
to InputEvent
potentially.
At least in android-activity
I defined InputEvent
as non_exhaustive
considering also exposing Text
/IME events but maybe for the ndk API it's ok to just assume the types of InputQueue events won't get extended?
Are you okay with the proposed idea of both having non_exhaustive and a hidden __Unknown?
Yeah I think it makes sense. This is at least what I settled on with android-activity after trying to ponder the trade offs. It's maybe not the perfect solution - ideally the compiler could give us a true repr(i32)
enum but it seems like a pretty decent trade off for allowing extension of enums via Android updates.
I hope you've also cross-checked that I didn't miss out on some
enum
s?I didn't, but can take a pass to cross check now...
okey, I've taken a pass through all source for the ndk
crate to try and find any enums missed and left a few comments above and haven't found anything else.
Going through cross checking all enums, it could be good go add
non_exhaustive
toInputEvent
potentially.At least in
android-activity
I definedInputEvent
asnon_exhaustive
considering also exposingText
/IME events but maybe for the ndk API it's ok to just assume the types of InputQueue events won't get extended?
Good point. I ended up glossing over this but there are definitely more input types, though currently none has a type and specific functions. Let's mark this as non_exhaustive
.
pub const AINPUT_EVENT_TYPE_KEY: _bindgen_ty_19 = 1;
pub const AINPUT_EVENT_TYPE_MOTION: _bindgen_ty_19 = 2;
pub const AINPUT_EVENT_TYPE_FOCUS: _bindgen_ty_19 = 3;
pub const AINPUT_EVENT_TYPE_CAPTURE: _bindgen_ty_19 = 4;
pub const AINPUT_EVENT_TYPE_DRAG: _bindgen_ty_19 = 5;
pub const AINPUT_EVENT_TYPE_TOUCH_MODE: _bindgen_ty_19 = 6;
It probably makes sense to add an
__Unknown
variant toFamilyVariant
I think
Whoops, looks like I made an exception for an input-only enum here, to encourage NDK updates when needing to pass new values. Didn't do that anywhere else I think, so let's add it.
Thanks a lot @rib! I know it's a big ask to go through all enums, it's quite a bit to process. But glad to have it resolved in the end.
Who knows, once I import the new values, if we can just replace the enums in android-activity
with these again.
Depends on #458
We're currently in a painful situation, both when it comes to correctness of the wrapper functions that take or return enum values, as well as convenience and ability to upgrade. This is a summation of issues, and reasons why external crates like
android-activity
are copy-pasting and fixing up our implementation:_UNKNOWN
constants. This makes the API lossy as the original unknown value does not have the KNOWN value that is assigned to the_UNKNOWN
constant (typically0
);#[non_exhaustive]
in most places, not allowing us to add new and missing constants toenum
;#[repr(u32)]
that follows the defaultu32
type used bybindgen
when processing an "untyped" enum, rather than considering the type (i32
,i8
, etc) that is actually used by every function taking or returning this "integer constant";Err
type (i.e.AudioError::UnknownValue
orTryFromPrimitiveError
);Unknown = ffi::XXX_UNKNOWN
variant;Unknown(xxx)
variant.To solve this we have to make the following changes to every
pub enum
with custom discriminants in the NDK crate:repr(u32)
to use the common type used by every function that takes or returns this enum value. Conversions always happen via.into()
fromIntoPrimitive
(needed to automatically process__Unknown
);enum
s with#[non_exhaustive]
. This requires callers to have ax => todo!("New constant {x:?}")
or similar handling in amatch
block. It will neatly print the value for__Unknown
added below;TryFromPrimitive
withFromPrimitive
;__Unknown
variant, that is#[doc(hidden)]
. This:TryIntoPrimitiveError
or customErr
type;#[doc(hidden)]
, as a lot of enums are currently lagging behind their upstream representation;IntoPrimitive
to all enums that were using manualmatch
es.FromPrimitive
is not derived, and similarly an__Unknown
variant is not added. This does not allow custom values to be passed, but it does again entice crate updates when there are new values.TODO
AudioResult
/AudioError
, implementError
on them immediately like onMediaError
. This is possible now that the customUnknownValue
variant is gone.DataSpace
is fairly recent and I don't remember why I didn't follow the above guidelines right from the get-go.In a followup PR missing constants will be added to various
enum
s.