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

multi(gestures): add keyTriggerClickRotate gesture, fix keyTriggerDragRotate gesture #1810

Draft
wants to merge 15 commits into
base: gesture-handling
Choose a base branch
from

Conversation

josxha
Copy link
Contributor

@josxha josxha commented Jan 23, 2024

Issues:

(2) Cursor keyboard rotation also no longer sets rotation to the angle of cursor immediately when clicking.

2024-01-22.20-38-47.mp4

(1) One bug with cursor keyboard rotation. Rotation is not tracking the cursor properly. I remember having a similar issue when implementing the algorithm initially. Maybe double checking that nothing in the implementaiton itself has changed, and every variable is being changed/reset at the correct points.


  • This pr adds the keyTriggerClickRotate gesture to the gesture rewrite.
  • The gesture takes slightly longer to execute than on master to filter out drag gestures while the CTRL/specified key is pressed.
  • The rotation should be animated, added this as an idea to [SPEC] gesture system enhancements #1748.

@josxha josxha self-assigned this Jan 23, 2024
@josxha josxha added this to the v7.0 milestone Jan 23, 2024
@JaffaKetchup
Copy link
Member

The gesture takes slightly longer to execute than on master to filter out drag gestures while the CTRL/specified key is pressed.

Can you elaborate here? If the trigger key is held, there should be no other gestures or onTap/onDoubleTaps captured, and therefore no delay.

@josxha josxha marked this pull request as draft January 25, 2024 17:55
@josxha
Copy link
Contributor Author

josxha commented Feb 1, 2024

Not sure if I should call this a feature or a limitation of the new implementation...
One of the biggest goals of the gesture rewrite was to be as high level as possible to avoid any potential platform specific bugs.
Because we are using the GestureDetector to listen for every gesture input it supports and GestureDetector has delays some callbacks until they are semi-verified or verified we have a slight delay here (the known double tap delay).
We could disable the double tap callbacks while the triggerKey is pressed, however this would require an additional widget rebuild. Additionally we would need a on-trigger-key-release callback that could lead to problems if the targeted windows switches before the key gets released.
While the delay is noticable at the moment I think it's not that bad once the rotation is animated. I think I've seen such delays for commerical maps as well if I'm not wrong 🤔

What do you think? Do you want to have some workaround code to have no delay at all?

@josxha josxha marked this pull request as ready for review February 1, 2024 20:22
@josxha josxha removed this from the v7.0 milestone Feb 1, 2024
@josxha josxha changed the title feat(gestures): add key trigger click rotate gesture multi(gestures): add keyTriggerClickRotate gesture, fix keyTriggerDragRotate gesture Feb 1, 2024
@josxha
Copy link
Contributor Author

josxha commented Feb 2, 2024

Added the keyTriggerDragRotate gesture fix to this pr because both are quite similar.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

We could disable the double tap callbacks while the triggerKey is pressed, however this would require an additional widget rebuild

I don't know, but this can't be too much of an issue. We're constantly doing this all the time (as far as I understand, if you mean setState).

Do you want to have some workaround code to have no delay at all?

Yes, as above. This delay makes the controls feel sluggish - gestures should give instant feedback, otherwise the user may attempt to perform them again.

@JaffaKetchup JaffaKetchup marked this pull request as draft March 13, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review in progress
Development

Successfully merging this pull request may close these issues.

2 participants