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

input: Replace open-coded types with ndk::event definitions #163

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

MarijnS95
Copy link
Member

As documented in // XXX comments, and discussed before, the copy-pasted input types exist because the ndk crate "wasn't set up" to allow changes to enums in a non-semver-breaking way (in particular, they were not marked #[non_exhaustive]). This issue was not reported upstream, and it took me to use android-activity before realizing so and fixing it in rust-mobile/ndk#459. Also, after comparing the copy-paste here against the ndk types, no new variants were ever added, completely defeating the purpose (even though new types were added to the ustream headers and will be added to te ndk crate).

Hence, remove all the open-coded types again and reexport them from the ndk crate. Upstream not only made all enums #[non_exhaustive], but also fixes the repr types to not use the u32 default for untyped enum constants, but in most cases i32 when all function signatures take a signed argument.

@MarijnS95 MarijnS95 requested a review from rib August 6, 2024 11:50
@MarijnS95
Copy link
Member Author

Will rebase this as soon as #164 is in!

Copy link
Member

@rib rib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this, I think in general this seems reasonable to do at this point.

Originally, a notable part of the purpose for these types was just to provide types that could be shared between the GameActivity and NativeActivity backend.

There have repeatedly been issues with directly exposing ndk input types in the android-activity API due to differences between GameActivity and NativeActivity and so I've generally been cautious about going back to exposing them. E.g. for a while the Winit Android backend wouldn't work with the game-activity backend because of stuff like this: https://github.com/rust-windowing/winit/blob/918430979f8219648daade44796c00893e42fdd8/src/platform_impl/android/mod.rs#L404

Since the input API is so central to the android-activity crate then in the past it's also been convenient to not be too tightly coupled to the ndk crate here, so I could make input API changes without being blocked on there being a new ndk release.

Maybe if there are some missing variants it's a good time to look at using shared types from the ndk crate if they can now be shared across both backends.

I think the main thing that seems to be missing atm are some CHANGELOG notes about the breaking changes. For completeness that should probably include a note about the repr changes

@MarijnS95
Copy link
Member Author

There have repeatedly been issues with directly exposing ndk input types in the android-activity API due to differences between GameActivity and NativeActivity

We discussed this before, and as highlighted in this PR that those compatibility issues weren't brought up "upstream" at the ndk crate before, nor really seem to exist when I put the effort in comparing these enums to replace them. If anything the ndk already has more variants for some enums, and has a change scheduled to add anything that's still missing.

E.g. for a while the Winit Android backend wouldn't work with the game-activity backend because of stuff like this: https://github.com/rust-windowing/winit/blob/918430979f8219648daade44796c00893e42fdd8/src/platform_impl/android/mod.rs#L404

We've since fixed that, which is specifically why this PR is open now so I don't think it's fair to dredge that back up.

Since the input API is so central to the android-activity crate then in the past it's also been convenient to not be too tightly coupled to the ndk crate here, so I could make input API changes without being blocked on there being a new ndk release.

Again, there are little to no differences at this time. I'm keeping the structs on your side and only replacing the enums and bitflags anyway.

Maybe if there are some missing variants it's a good time to look at using shared types from the ndk crate if they can now be shared across both backends.

That's exactly what this PR did :)

I think the main thing that seems to be missing atm are some CHANGELOG notes about the breaking changes. For completeness that should probably include a note about the repr changes

Fixed.

Copy link
Member

@rib rib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at all of this - it looks good to me!

@@ -6,6 +6,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

- input: Replaced custom types with their `ndk` crate equivalent.
> [!NOTE]
> These types existed because the `ndk` crate didn't provide them in an extensible way. Now that they have the `#[non_exhaustive]` flag and contain a `__Unknown(T)` variant to result in lossless conversions, and not to mention use an ABI type that how its used by most functions (when the original constants were defined in a "typeless" way), the `ndk` types are used and reexported once again.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> These types existed because the `ndk` crate didn't provide them in an extensible way. Now that they have the `#[non_exhaustive]` flag and contain a `__Unknown(T)` variant to result in lossless conversions, and not to mention use an ABI type that how its used by most functions (when the original constants were defined in a "typeless" way), the `ndk` types are used and reexported once again.
> These types existed because the `ndk` crate didn't provide them in an extensible way. Now that they have the `#[non_exhaustive]` flag and contain a `__Unknown(T)` variant to result in lossless conversions, and not to mention use an ABI type that matches how it is being used by most functions (when the original constants were defined in a "typeless" way), the `ndk` types are used and reexported once again.


> [!IMPORTANT]
> **Relevant breaking changes**:
> - `repr()` types for some `enum`s have changed to match the type that returned and consumed by functions interacting with this wrapper type.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> - `repr()` types for some `enum`s have changed to match the type that returned and consumed by functions interacting with this wrapper type.
> - `repr()` types for some `enum`s have changed to match the type that is returned from and consumed by functions interacting with this wrapper type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the word interacting here but haven't found a concise way to say "a function that returns or uses one of these wrapper types".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"referencing this wrapper type" perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems best to me:

  • repr() types for some enums have changed to match the ABI type that is used by most functions that are returning or consuming this wrapper type.

@MarijnS95 MarijnS95 merged commit 51d05d4 into main Jan 27, 2025
6 checks passed
@MarijnS95 MarijnS95 deleted the fix-enum-types branch January 27, 2025 17:12
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