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

native-activity/input: AND with EVENT_ACTION_MASK when extracting action #147

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

ArthurCose
Copy link
Contributor

Partially fixes #146. Possibly fully fixes the issue as I haven't encountered any issues with multitouch positions with winit on Android.

@MarijnS95 MarijnS95 changed the title Fix multitouch MotionActions processing as unknown in native activities native-activity/input: OR with EVENT_ACTION_MASK when extracting action Dec 25, 2023
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Agreed, this was wrongly converted from the NDK. 0xff is the action, 0xff00 contains the pointer index (which is using the underlying NDK function).

@rib regarding:

// XXX: we use `AInputEvent_getAction` directly since we have our own
// `KeyAction` enum that we share between backends, which may also
// capture unknown variants added in new versions of Android.

I desperately want to fix this in the NDK as there are new values in r26 that I cannot add without an API break. A few things:

  • I want to make these non_exhaustive to allow adding new values without API breaks;
  • The NDK wrongly converts not-yet-handled values to Unknown which always has a special _UNKNOWN variant (with value 0). I.e. the conversion is lossy;
  • I don't really want to have an Unknown(i32) case to provoke updating the NDK instead. I don't know how common it is (for e.g. custom Android distros) to have custom/unknown values;
    (One big issue I face now is that there are a bunch of new - and super useful! - functions in Android 14 that are not yet in a released NDK, so I might generate from the latest sysroot as was suggested to me before);
  • But that requires having a Result with TryFromPrimitiveError;
  • Hence I do like that you've used the combination of non_exhaustive with an Unknown case that is #[doc(hidden)].

After that is done, how do you feel about using the ndk types again, assuming NativeActivity and GameActivity expose the same values? If not, are there any known mismatching / missing values?

@MarijnS95 MarijnS95 requested a review from rib December 25, 2023 11:53
Copy link
Collaborator

@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.

Sorry for the delay with reviewing this.

This looks good to me - thanks!

@rib rib merged commit 4ffa3ac into rust-mobile:main Jan 30, 2024
3 checks passed
@torokati44
Copy link

Pardon me, just passing by, but... why is OR in the PR title, when the added operator is &? 🤔

@rib
Copy link
Collaborator

rib commented Jan 30, 2024

Pardon me, just passing by, but... why is OR in the PR title, when the added operator is &? 🤔

hmm, yeah title should have been updated and then I also copy & pasted that for the changelog, oops.

@MarijnS95 MarijnS95 changed the title native-activity/input: OR with EVENT_ACTION_MASK when extracting action native-activity/input: AND with EVENT_ACTION_MASK when extracting action Jan 31, 2024
@MarijnS95
Copy link
Member

MarijnS95 commented Jan 31, 2024

Yeah so the commit title in the repo and changelog will be wrong, but the PR title and GitHub release are fixed to mention AND at least :)

@rib side-note: the changelog uses [0.2] link labels, but those tags are never specified to point to a link, e.g. [0.2]: https://github.com/rust-mobile/android-activity/releases/tag/v0.2.0.

@rib
Copy link
Collaborator

rib commented Feb 2, 2024

@rib side-note: the changelog uses [0.2] link labels, but those tags are never specified to point to a link, e.g. [0.2]: https://github.com/rust-mobile/android-activity/releases/tag/v0.2.0.

yeah I guess we could turn those into links like that. The Keep a Changelog reference that's linked in the changelog does that, so would be nice to be consistent with that.

Yeah so the commit title in the repo and changelog will be wrong, but the PR title and GitHub release are fixed to mention AND at least :)

we can also fly-by fix up the changelog the next time that gets touched.

@MarijnS95
Copy link
Member

Exactly that, yes :)

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.

Can't get multitouch to work with native activity
4 participants