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

Both scaleable and draggable chart implemented #1728

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

adamsocrat
Copy link
Contributor

@adamsocrat adamsocrat commented Aug 15, 2024

Implements #1720

I used the idea from #1721 and implemented FlScaleEvent but without explicitly specifying detectScale to disable drag fully when scaling. detectScale could've been set dynamically but it was combursome and couldn't get it working. Implementing a new FLTouchEvent seemed better option.

The drawback was as in #1721 mentioned PanGestureRecognizer and ScaleGestureRecognizer does not like to be used together. But ScaleGestureRecognizer event details also feeds the PanGestureRecognizer event details (except onDown) see, so instead of disabling dragging fully when you want to scale, I notify ScaleEvent when there are two points and drag when one point is in contact with the screen. Same idea of this comment

! Details feeding into FlPan...Event have been switched to Scale details. Testing may be needed.

You can get TouchEvent from touchcallback as

touchCallback: (FlTouchEvent touchEvent, lineTouchRsp) {
              if (touchEvent is FlScaleStartEvent) {
                       // https://api.flutter.dev/flutter/gestures/ScaleStartDetails-class.html
                       }
             if (touchEvent is FlScaleUpdateEvent) {
                      // You can see the details of touch event with:  touchEvent.details
                     // https://api.flutter.dev/flutter/gestures/ScaleUpdateDetails-class.html
                       }
             if (touchEvent is FlPanUpdateEvent) {
                     // NOT https://api.flutter.dev/flutter/gestures/DragUpdateDetails-class.html 
                     // https://api.flutter.dev/flutter/gestures/ScaleUpdateDetails-class.html
                       }
}

with the FlScaleEvent I am able to set minX and maxX to zoom in/out chart without the scale is being also interpreted as drag. See #71

@TheOlajos
Copy link

Would be great if this could be merged for the next implementation.

@adamsocrat adamsocrat marked this pull request as draft August 16, 2024 12:43
@adamsocrat adamsocrat marked this pull request as ready for review August 19, 2024 12:20

R? response;
if (pointerCount > 1) {
_touchCallback!(event, response);
Copy link
Owner

Choose a reason for hiding this comment

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

You can just set a null as the second parameter (instead of having a nullable R?, which is always null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

_scaleGestureRecognizer
..onStart = (details) {
_notifyScaleEvent(FlScaleStartEvent(details));
_notifyTouchEvent(FlPanDownEvent(details));
Copy link
Owner

Choose a reason for hiding this comment

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

Is it okay to call both FlPanDown and FlPanStart at the same time?

Copy link
Contributor Author

@adamsocrat adamsocrat Sep 12, 2024

Choose a reason for hiding this comment

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

Probably not, drag down details and scalestart details will be different and be a breaking change. Thinking of another solution

_scaleGestureRecognizer = ScaleGestureRecognizer();
_scaleGestureRecognizer
..onStart = (details) {
_notifyScaleEvent(FlScaleStartEvent(details));
Copy link
Owner

Choose a reason for hiding this comment

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

Does it get called for the second finger (when you already have your first finger, and try to add the second finger to do the scale gesture)?

Copy link
Owner

Choose a reason for hiding this comment

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

In this case, I think we should have something like fingerIndex or something like that

_notifyTouchEvent(FlPanStartEvent(dragStartDetails));
..onUpdate = (details) {
_notifyScaleEvent(FlScaleUpdateEvent(details));
_notifyTouchEvent(FlPanUpdateEvent(details));
Copy link
Owner

Choose a reason for hiding this comment

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

And also for here, (something like fingerIndex)

@eladm-ultrawis
Copy link

Hi @adamsocrat @imaNNeo , joining the conversation here.

I think it might be a bit inconsistent with the rest of the flutter framework to raise scale events for 2 fingers and pan for 1 finger.

To my limited understanding, the idea behind this is that - scale is also a pan. with the scale events, you could implement pan because it already has all the necessary information.
I think whatever implementation you guys end choosing, should be consistent with how GestureDetector works in this scenario.

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (15c66cf) to head (2db829e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1728   +/-   ##
=======================================
  Coverage   89.17%   89.17%           
=======================================
  Files          44       44           
  Lines        3031     3031           
=======================================
  Hits         2703     2703           
  Misses        328      328           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

4 participants