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

Allow Android Rotation Gesture to continue after second Pointer Lift and Land #3057

Conversation

DmitriyGalyanov
Copy link
Contributor

@DmitriyGalyanov DmitriyGalyanov commented Aug 22, 2024

Description

On iOS Rotation Gesture is not getting finished/cancelled when one of two Pointers lifts up, allowing the Gesture to continue when a new second Pointer arrives

On Android this behaviour differs – Rotation Gesture finishes whenever one of two Pointers lifts up, requiring to land 2 completely new Pointers in order to utilise Rotation Gesture again

I understand that this may be closer to default Android Gesture Recogniser Behaviour, but find it unexpected and inconvenient

Thus in this PR I’m adding Ability to tell Rotation Gesture Builder to enable iOS-like Behaviour, which allows Gesture Pause/Remain when second Pointer lifts/lands

Additionally, I added some local toggleable Logs, which I find useful for Debugging, but I’m not sure if it aligns with this Repo Style Guide (or could noticeably reduce Performance) – please notify me, if it’s prohibited

I think this Change may be usable for Gesture Handler Users, thus trying to push it to the Gesture Handler (not only my own Fork) and looking forward to any Feedback regarding both Code – Implementation/Naming and/or local Style Guide – and Docs, which I might’ve misaligned with

Attached Gifs display what I mean (first one shows current Behaviour, second one – with new Modifier)

Test plan

  1. Use RotationGesture without new Modifier (secondPointerLiftFinishesGesture(value: boolean)) and see if it works as expected (as before – Gesture finishes when one of two key Pointers is lifted)
  2. Use RotationGesture with new Modifier (secondPointerLiftFinishesGesture(value: boolean)) and see if it works as expected (Gesture doesn't finish when second Pointer is lifted and continues when second Pointer is landed again)

@DmitriyGalyanov DmitriyGalyanov force-pushed the allow-Android-Rotation-Gesture-to-continue-after-second-Pointer-Lift-and-Land branch from 1684d91 to c72b3a9 Compare August 22, 2024 12:54
@DmitriyGalyanov DmitriyGalyanov force-pushed the allow-Android-Rotation-Gesture-to-continue-after-second-Pointer-Lift-and-Land branch from c72b3a9 to fec2867 Compare August 22, 2024 15:09
@DmitriyGalyanov
Copy link
Contributor Author

@wcandillon

Guys, does somebody see this?

Could you please tell me if it's not needed / aligned with your opinion or will be overseen later

@j-piasecki
Copy link
Member

Hi! Sorry, I didn't have much time for reviews lately. I should be able to spend more time on reviews soon(-ish, in a week, maybe two).

Copy link
Member

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

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

I understand that this may be closer to default Android Gesture Recogniser Behaviour, but find it unexpected and inconvenient

I'm not sure whether it's closer, as a native rotation recognizer doesn't exist on Android AFAIK. Moreover, I'd lean towards saying that it should've been working like this since the beginning and classify that as a bugfix. Instead of making it opt-in, I'd simply make it a default behavior. I'm not sure whether we even need to keep the old one behind a flag, current behavior seems very counterintuitive to me and I don't really see anyone relying on it.

Additionally, I added some local toggleable Logs, which I find useful for Debugging, but I’m not sure if it aligns with this Repo Style Guide (or could noticeably reduce Performance) – please notify me, if it’s prohibited

Prohibited is a strong word, but I'd remove the logs from the PR. We may consider adding them in the future but to the whole library instead of a single gesture.

Also, please run yarn format:android command before pushing to make sure it follows the code style.

Sorry for the delay on this 😅, there's been a lot going on recently.

@DmitriyGalyanov
Copy link
Contributor Author

@j-piasecki

Sorry for the delay on this 😅, there's been a lot going on recently.

Thank you for the reply!
Hopefully will update PR accordingly to your review this week

@DmitriyGalyanov
Copy link
Contributor Author

I'm not sure whether it's closer, as a native rotation recognizer doesn't exist on Android AFAIK. Moreover, I'd lean towards saying that it should've been working like this since the beginning and classify that as a bugfix. Instead of making it opt-in, I'd simply make it a default behavior. I'm not sure whether we even need to keep the old one behind a flag, current behavior seems very counterintuitive to me and I don't really see anyone relying on it.

Glad to see that!
And, since both of us think it's closer to a bugfix, I've removed flag as redundant

Copy link
Member

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

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

Thank you for working on it!

@j-piasecki j-piasecki merged commit 5730bf9 into software-mansion:main Oct 9, 2024
3 checks passed
@DmitriyGalyanov
Copy link
Contributor Author

@j-piasecki

Hello!

Could you, please, tell me when (at least approximately) would it be released?

@j-piasecki
Copy link
Member

@DmitriyGalyanov Most likely next week.

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