Add friction coefficient to InteractiveViewer #109443

frankteller-de · 2022-08-12T07:07:43Z

It would be nice if the kDrag parameter were public to change the scrolling behavior.

For example I've changed the kDrag var into a public parameter called scrollFriction.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

flutter-dashboard · 2022-08-12T07:07:47Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

google-cla · 2022-08-12T07:07:47Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

spaces removed

packages/flutter/lib/src/widgets/interactive_viewer.dart

alestiago · 2022-08-12T11:04:39Z

packages/flutter/lib/src/widgets/interactive_viewer.dart

+  /// Changes the scrolling behavior after end of a gesture.
+  ///
+  /// Used as the coefficient of friction in the inertial translation animation.
+  /// This value was eyeballed to give a feel similar to Google Photos.
+  ///
+  /// Defaults to 0.0000135.
+  ///
+  /// Cannot be null, and must be a finite number greater than zero


Can we add documentation on how the scroll changes when the value increases or decreases?

packages/flutter/lib/src/widgets/interactive_viewer.dart

alestiago · 2022-08-12T11:17:41Z

I agree with this change, it would be nice if the constant internal value could be changed. I think the solution is quite simple. Thanks for raising the PR!

I just added small nit comments regarding naming and documentation, overall looks good to me. Are you planning to add tests?

frankteller-de · 2022-08-12T11:33:43Z

Thank you @alestiago! I can add some tests but first I have to look at the documentation what I have to do to add a test. It was my first PR.

scrollFriction renamed to scrollFrictionCoefficient Co-authored-by: Alejandro Santiago <dev@alestiago.com>

rename scrollFriction

* added a test

frankteller-de · 2022-08-12T12:47:07Z

I've added a test but I don't know what I should test exactly. That parameter is very random.

spaces removed

alestiago · 2022-08-12T19:06:25Z

You could test how the amount of scroll differs when a value x is provided compared to when a value y is given.

justinmc

Thanks for opening this PR, I'm on board but I want to make sure there's not a more generic way to solve this.

This makes me think of Scrollable's physics parameter. What if we allowed users to pass in the entire Simulation and defaulted to the current FrictionSimulation? Or even more high level, maybe InteractiveViewer should be using ScrollPhysics?

@Piinks You're working on 2D scrolling, are you using regular ScrollPhysics for that, or is there anything that could also apply here?

I'm not saying that scrollFrictionCoefficient isn't the best solution here, I just want to think through these other options.

alestiago · 2022-08-13T01:03:17Z

I agree with @justinmc making use of a subclass of ScrollPhysics to achieve the effect is an interesting and more consistent solution with the framework and other Scrollables. Definitely a great suggestion!

I'm also glad to hear @justinmc is onboard with the aims of this PR. Thanks for hopping in 💙

I'm curious about @Piinks insights and opinion on this PR to 👀

Piinks

I would love to incorporate more scrolling into interactive viewer, but I think it is more complicated than adding some properties that emulate scrolling. I've been exploring this in #64210. Ultimately, under the hood InteractiveViewer is not a scrolling widget. We may be able to rework it to become one, but that may require breaking changes, which is why I have been working on a separate 2D scrollable that would avoid breaking existing code.

I don't think adding this is a bad idea, I would just avoid referring to it as scrolling.

Piinks · 2022-08-15T21:30:32Z

packages/flutter/lib/src/widgets/interactive_viewer.dart

    // use cases.
    this.maxScale = 2.5,
    this.minScale = 0.8,
+    this.scrollFrictionCoefficient = _kDefaultScrollFrictionCoefficient,


This feels a bit misleading, InteractiveViewer is not a scrolling widget. If we use scrolling terms, users expect other scrolling conventions to apply, like having a ScrollPosition, and ScrollNotifications.

It's a good point. How about we call it interactionEndFrictionCoefficient or dragEndFrictionCoefficient?

That sounds good to me! :)

justinmc · 2022-08-17T18:47:11Z

Thanks @Piinks.

So using ScrollPhysics is off the table, but does it make sense to allow customizing the entire Simulation (here)? Or maybe that would still be far too complicated and we should just stick with the coefficient?

Piinks · 2022-08-17T23:00:00Z

does it make sense to allow customizing the entire Simulation (here)? Or maybe that would still be far too complicated and we should just stick with the coefficient?

@codeforce-dev is there an open issue that this is seeking to resolve? It would be very helpful in answering these questions and better understanding the request.

frankteller-de · 2022-08-18T08:48:27Z

The coefficient would be the simplest way to optimize the "scrolling" behaviour for some purposes. A more complex solution is not necessary. For example, pdf_render uses InterativeViewer to handle the user interaction. But in my opinion, the default drag behaviour with a value of 0.0000135 feels not good for that purpose.

FrictionCoefficient
Screen 1 = 0.0000135 | Screen 2 = 0.01

justinmc

It sounds to me like a custom Simulation is too confusing and low-level for use cases like the one given by @codeforce-dev. I say let's just go forward with exposing the coefficent of friction like in this PR 👍.

A few comments below but I think this is almost ready. Thanks for dealing with all of my questions on making sure this is the best solution!

justinmc · 2022-08-19T17:51:12Z

packages/flutter/lib/src/widgets/interactive_viewer.dart

 // late TransformationController _transformationController;
 // Widget child = const Placeholder();

+/// Default coefficient of friction.


I think the original comments on _kDrag had some important details about where the value came from:

// Used as the coefficient of friction in the inertial translation animation. // This value was eyeballed to give a feel similar to Google Photos.

justinmc · 2022-08-19T17:56:28Z

packages/flutter/lib/src/widgets/interactive_viewer.dart

+  /// Used as the coefficient of friction in the inertial translation animation.
+  /// This value was eyeballed to give a feel similar to Google Photos.
+  ///
+  /// Defaults to [_kDefaultInteractionEndFrictionCoefficient].


This constant is private. I think we should explicitly give the 0.0000135 value here, because readers will want to know whether to give a value greater than or less than that for their own use cases.

justinmc · 2022-08-19T18:05:40Z

packages/flutter/test/widgets/interactive_viewer_test.dart

+      await tester.dragFrom(const Offset(100.0, 100.0), const Offset(0.0, 50.0));
+      final Vector3 translation2 = transformationController.value.getTranslation();
+
+      expect(translation2.y, equals(0));


I think this isn't properly testing the feature. It looks like in the first drag, the expectation happens immediately after the drag, before any inertia animation has happened, so the y value is exactly the same as the length of the gesture (-50.0). In the second drag, it goes in the opposite direction and ends up not moving at all (because it hit the boundary?).

What if you pumpAndSettle after each drag and then get the translation? I imagine then you would see more translation because the inertia animation has happened. (And I think make sure the second drag goes in the same direction as the first.)

Lastly, I think the most important expectation is that translation1.y > translation2.y, because that shows that a greater coefficient of friction slows down the animation more quickly.

Thanks for your reply. I've already tried dragFrom and moveBy with pumpAndSettle but unfortunately it has no effect on it. I always get -50.

final TestGesture gesture = await tester.startGesture(const Offset(100, 100)); await gesture.moveBy(const Offset(0, -50)); await tester.pumpAndSettle(); final Vector3 translation = transformationController.value.getTranslation(); // translation.y = -50;

In that case, flingFrom is the solution.

await tester.flingFrom(const Offset(100, 100), const Offset(0, -50), 100.0); await tester.pumpAndSettle();

_kDefaultInteractionEndFrictionCoefficient constant removed for more readability

Piinks

LGTM with a few nits - thank you!

packages/flutter/lib/src/widgets/interactive_viewer.dart

Piinks · 2022-09-14T21:06:19Z

packages/flutter/lib/src/widgets/interactive_viewer.dart

+  /// Changes the scrolling behavior after end of a gesture.
+  ///
+  /// Used as the coefficient of friction in the inertial translation animation.
+  /// This value was eyeballed to give a feel similar to Google Photos.


Can you remove this? This could change at any time, but also does not account for how Google Photos behaves on different devices.

+1. Keep it in the comments below, but remove it from the public docs. You could make it "Google Photos on Android 12" since I think that's what I was using when I wrote that comment.

justinmc

LGTM with nits as well. Let me know if you want to fix those and we can get this merged. 👍

packages/flutter/lib/src/widgets/interactive_viewer.dart

justinmc · 2022-09-14T23:08:48Z

packages/flutter/lib/src/widgets/interactive_viewer.dart

+  /// Changes the scrolling behavior after end of a gesture.
+  ///
+  /// Used as the coefficient of friction in the inertial translation animation.
+  /// This value was eyeballed to give a feel similar to Google Photos.


+1. Keep it in the comments below, but remove it from the public docs. You could make it "Google Photos on Android 12" since I think that's what I was using when I wrote that comment.

justinmc · 2022-09-14T23:10:35Z

packages/flutter/test/widgets/interactive_viewer_test.dart

+        MaterialApp(
+          home: Scaffold(
+            body: SizedBox(
+              width: 200, height: 200,


Separate these onto different lines.

justinmc · 2022-09-14T23:12:43Z

packages/flutter/test/widgets/interactive_viewer_test.dart

+      await tester.pumpAndSettle();
+      final Vector3 translation2 = transformationController2.value.getTranslation();
+
+      // Test 2 movement must be greater than in Test 1.


Change this to: The coefficient 0.01 is greater than the default of 0.0000135, so the translation comes to a stop more quickly.

Piinks · 2022-09-20T18:01:52Z

Hi @codeforce-dev, would you like to address the outstanding comments from @justinmc so this can land? Thanks!

…r#109443)

scrollFriction added to InteractiveViewer

e51c419

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 12, 2022

Update interactive_viewer.dart

b3b5763

spaces removed

alestiago reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/interactive_viewer.dart Outdated Show resolved Hide resolved

alestiago reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/interactive_viewer.dart Outdated Show resolved Hide resolved

alestiago reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/interactive_viewer.dart Show resolved Hide resolved

alestiago reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/interactive_viewer.dart Outdated Show resolved Hide resolved

alestiago reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/interactive_viewer.dart Outdated Show resolved Hide resolved

alestiago reviewed Aug 12, 2022

View reviewed changes

packages/flutter/lib/src/widgets/interactive_viewer.dart Outdated Show resolved Hide resolved

frankteller-de and others added 6 commits August 12, 2022 13:48

Apply suggestions from code review

a2bfd15

scrollFriction renamed to scrollFrictionCoefficient Co-authored-by: Alejandro Santiago <dev@alestiago.com>

Update interactive_viewer.dart

d2a7868

rename scrollFriction

update

15676ec

update

ca36786

* added _kDefaultScrollFrictionCoefficient

0649892

* added a test

Merge branch 'flutter:master' into master

096a0e8

Update interactive_viewer_test.dart

1544747

spaces removed

HansMuller requested a review from justinmc August 12, 2022 22:14

justinmc reviewed Aug 12, 2022

View reviewed changes

frankteller-de added 2 commits August 15, 2022 10:55

scrollFrictionCoefficient test update

4866626

Merge branch 'master' of https://github.com/codeforce-dev/flutter-v3.0.5

00e441b

Piinks reviewed Aug 15, 2022

View reviewed changes

scrollFrictionCoefficient changed to interactionEndFrictionCoefficient

4d8f821

scrollFrictionCoefficient changed to interactionEndFrictionCoefficient

909322a

frankteller-de changed the title ~~Add scrollFriction to InteractiveViewer~~ Add friction coefficient to InteractiveViewer Aug 17, 2022

frankteller-de added 2 commits August 17, 2022 15:25

update

ee97fb9

update

9042a43

justinmc reviewed Aug 19, 2022

View reviewed changes

frankteller-de added 3 commits August 19, 2022 20:49

Update interactive_viewer.dart

0577bd6

_kDefaultInteractionEndFrictionCoefficient constant removed for more readability

interactionEndFrictionCoefficient test modified

6570d0f

Merge branch 'flutter:master' into master

75254f0

goderbauer requested a review from justinmc August 23, 2022 22:17

frankteller-de requested review from Piinks and alestiago and removed request for alestiago and justinmc September 1, 2022 21:03

Piinks approved these changes Sep 14, 2022

View reviewed changes

justinmc approved these changes Sep 14, 2022

View reviewed changes

frankteller-de and others added 5 commits September 20, 2022 20:22

update

5fe26b0

Merge branch 'flutter:master' into master

1909afd

Merge branch 'master' of https://github.com/codeforce-dev/flutter-v3.0.5

44eedfb

Merge branch 'flutter:master' into master

ecb9837

Update packages/flutter/lib/src/widgets/interactive_viewer.dart

bb43621

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 20, 2022

auto-submit bot merged commit 4aea2f9 into flutter:master Sep 20, 2022

This was referenced Sep 21, 2022

Roll Flutter from b55bbb7bffc7 to cf01ecd19e61 (29 revisions) flutter/packages#2632

Merged

Roll Flutter from b55bbb7bffc7 to cf01ecd19e61 (29 revisions) flutter/plugins#6458

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 21, 2022

4aea2f9 Add friction coefficient to InteractiveViewer (flutter/flutte…

9eba8a1

…r#109443)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 21, 2022

4aea2f9 Add friction coefficient to InteractiveViewer (flutter/flutte…

ff88a7f

…r#109443)

Add friction coefficient to InteractiveViewer #109443

Add friction coefficient to InteractiveViewer #109443

Uh oh!

Conversation

frankteller-de commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 12, 2022

Uh oh!

google-cla bot commented Aug 12, 2022

Uh oh!

Uh oh!

alestiago Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alestiago commented Aug 12, 2022

Uh oh!

frankteller-de commented Aug 12, 2022

Uh oh!

frankteller-de commented Aug 12, 2022

Uh oh!

alestiago commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

alestiago commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Aug 17, 2022

Uh oh!

Piinks commented Aug 17, 2022

Uh oh!

frankteller-de commented Aug 18, 2022

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankteller-de Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

frankteller-de commented Aug 12, 2022 •

edited

Loading

alestiago Aug 12, 2022 •

edited

Loading

alestiago commented Aug 12, 2022 •

edited

Loading

alestiago commented Aug 13, 2022 •

edited

Loading

frankteller-de Aug 19, 2022 •

edited

Loading