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

Android: Improved multi-touch #1783

Merged
merged 7 commits into from Dec 2, 2020
Merged

Android: Improved multi-touch #1783

merged 7 commits into from Dec 2, 2020

Conversation

MarnixKuijs
Copy link
Contributor

@MarnixKuijs MarnixKuijs commented Nov 30, 2020

In the previous implementation of multi-touch there was a issue. When having two fingers on the screen and you would release one it would result in ended events being generated for all fingers even though not all fingers were lifted. This implementation will generate started and ended events for the right finger id's. Furthermore this implementation will generate a cancelled event with different defaults. These defaults should be according to spec since the user shouldn't preform any action it would normally perform: "You will not receive any more points in it. You should treat this as an up event, but not perform any action that you normally would" according to the ndk documentation.

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Updated feature matrix, if new features were added or implemented

@MarnixKuijs MarnixKuijs changed the title Improved multi-touch Android: Improved multi-touch Nov 30, 2020
@MarnixKuijs MarnixKuijs marked this pull request as ready for review November 30, 2020 19:53
@msiglreith
Copy link
Member

Thanks, but I'm not totally sure what's the motivation behind this one. Please add a small description. I assume it refers to the default values for cancelled touch events?

@MarnixKuijs
Copy link
Contributor Author

Updated the description to provide some detail.

@MarnixKuijs MarnixKuijs marked this pull request as draft December 1, 2020 10:08
@MarnixKuijs
Copy link
Contributor Author

Converted back to draft to make some extra changes

@MarnixKuijs
Copy link
Contributor Author

Seems one check failed due to network error and not my code changes.

@MarnixKuijs
Copy link
Contributor Author

PR should be ready now. Cancelled events are generated for every pointer now. This seems to be the better approach, since it is more clear to the user that the moment a cancelled event is generated its generated for all fingers instead of a non existing one.

@MarnixKuijs MarnixKuijs marked this pull request as ready for review December 1, 2020 12:04
Copy link
Member

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Looks good overall, a few points are open and I would be also interested a bit in compatibility with other backends (ie iOS) regarding semantics.

  • If possible is there a way to shrink the code a bit as the branches often only differ in small details?
  • Do you know by chance how the cancelled case is handled on iOS? Single event vs multiple events, default values, etc?

cc @francesca64

src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved
src/platform_impl/android/mod.rs Outdated Show resolved Hide resolved
FEATURES.md Outdated Show resolved Hide resolved
@MarnixKuijs
Copy link
Contributor Author

Did some clean up to reduce code bloat and changed the feature matrix back to its original formatting. I guess I should leave cancelled how it is?

Copy link
Member

@francesca64 francesca64 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 the PR! The semantics look good. Could you add a CHANGELOG entry, so users know about this cool change?

@MarnixKuijs
Copy link
Contributor Author

So there already was a mention of Android multi-touch in the change log and since it was still in unreleased I just added some detail instead of making an extra entry.

@francesca64
Copy link
Member

Sorry, I didn't realize there was already an unreleased entry for multi-touch! I think it'd actually be better not to touch the CHANGELOG then, since the issue in question was never released.

@MarnixKuijs
Copy link
Contributor Author

Ooh okay, let me undo my change.

Copy link
Member

@francesca64 francesca64 left a comment

Choose a reason for hiding this comment

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

Looks great! We'll merge once CI finishes.

@francesca64 francesca64 merged commit 8fb7aa5 into rust-windowing:master Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants