Pan and zoom gallery demo by justinmc · Pull Request #25164 · flutter/flutter

justinmc · 2018-12-10T19:01:03Z

Adding a demo to the flutter_gallery app that shows how to do some basic canvas operations like panning, pinch to zoom, and hit detection.

Relevant issues: #8272, #12878

This PR won't close those issues, but after this goes in I'll open a new PR to move my code into a reusable widget that is more like what people are asking for.

justinmc · 2018-12-10T19:01:50Z

TODO:

app

HansMuller

Mostly superficial feedback about the code.

HansMuller · 2019-03-25T19:23:44Z

+import 'pan_and_zoom_demo_transform_interaction.dart';
+
+class PanAndZoomDemo extends StatefulWidget {
+  const PanAndZoomDemo({Key key}) : super(key: key);


({ Key key })

HansMuller · 2019-03-25T19:42:36Z

+    final BoardPainter painter = BoardPainter(
+      board: _board,
+    );
+    final Size screenSize = MediaQuery.of(context).size;


We need to get away from doing this kind of thing because it doesn't work on tablets or desktops. Better to use a LayoutBuilder to build this widget and then pass the "screen" size along as a parameter to this widget.

Sounds good, I wasn't aware of that pattern but I like it a lot more.

I may revisit the size and visibleRect params when I bring this widget into Flutter. It might be better if this is as big as it can be and users can constrain the size with a parent widget.

HansMuller · 2019-03-25T19:43:27Z

+
+    final FloatingActionButton floatingActionButton = _board.selected == null
+      ? FloatingActionButton(
+        onPressed: () => setState(() {


indent two spaces here and below

We usually reserve the => notation for one-liners that return a value

HansMuller · 2019-03-25T19:49:16Z

+          size: Size.infinite,
+          painter: painter,
+        ),
+        visibleRect: Rect.fromLTWH(


Maybe (someday) we should change the Rect.fromPoints parameters from Offset to OffsetBase so that you could write: visibleRect: Rect.fromPoints(-visibleSize / 2, visibleSize)

That would work for me!

HansMuller · 2019-03-25T20:00:45Z

+// CustomPainter is what is passed to CustomPaint and actually draws the scene
+// when its `paint` method is called.
+class BoardPainter extends CustomPainter {
+  BoardPainter({


Looks like should be const BorderPainter({ this.board }) etc

HansMuller · 2019-03-26T01:50:18Z

+  // Return a new matrix representing the given matrix after applying the given
+  // translation.
+  Matrix4 matrixTranslate(Matrix4 matrix, Offset translation) {
+    if (widget.disableTranslation) {


asserts that prove the parameter values aren't insane might be useful here.

HansMuller · 2019-03-26T01:51:38Z

+  }
+  void _onScaleUpdate(ScaleUpdateDetails details) {
+    widget.onScaleUpdate?.call(fromViewport(details.focalPoint, _transform));
+    double scale = _transform.getMaxScaleOnAxis();


Can scale be zero? That's not going to work below, where foo / scale

HansMuller · 2019-03-26T01:52:35Z

+  // Return a new matrix representing the given matrix after applying the given
+  // scale transform.
+  Matrix4 matrixScale(Matrix4 matrix, double scale) {
+    if (widget.disableScale) {


Are we done if scale is zero?

HansMuller · 2019-03-26T01:53:51Z

+    return matrix;
+  }
+
+  // Return the scene point underneath the viewport point given.


underneath the viewport point given. => at the given viewport point

HansMuller · 2019-03-26T02:02:05Z

+      onDoubleTap: widget.onDoubleTap,
+      onLongPress: widget.onLongPress,
+      onLongPressUp: widget.onLongPressUp,
+      onVerticalDragDown: widget.onVerticalDragDown == null ? null : (DragDownDetails details) => widget.onVerticalDragDown(fromViewport(details.globalPosition - getOffset(context), _transform)),


It might be clearer to factor out a generic method that produces these closure values:

T _callbackValue<T>(T callback) { if (callback == null) return callback; return (T details) => callback(fromViewport(details.globalPosition - getOffset(context), _transform)); }

And then:

onVerticalDragDown: callbackValue<DragDownDetails>(widget.onVerticalDragDown),

This is assuming that you can define a local generic method. Otherwise you'd have to pass along more parameters.

I can't seem to get this to work. It won't let me write details.globalPosition when details is an unknown type. Unfortunately there is no common parent class or interface for all of these different Details objects.

When I first wrote this, I was also trying things like having TransformInteraction inherit from GestureDetector, but I couldn't get anything like that working either.

Any thing else to try?

Sorry, here's a version that should work:

ValueChanged<T> _callbackValue<T>(ValueChanged<T> callback) { if (callback == null) return callback; return (T details) => callback(fromViewport(details.globalPosition - getOffset(context), _transform)); }

I need your help implementing this. These callbacks have changed a bit so it needs to be slightly different again. I'll show you later.

xster · 2019-03-27T18:39:58Z

+  static const double _kHexagonRadius = 32.0;
+  static const double _kHexagonMargin = 1.0;
+  static const int _kBorderRadius = 8;
+


add code comment for what this is

xster · 2019-03-27T18:41:13Z

+      board: _board,
+    );
+    final FloatingActionButton floatingActionButton = _board.selected == null
+      ? FloatingActionButton(


The reader can realize there are 2 alternative FABs but can't tell what the intents are. It's also a bit hard to read. Put them into small functions and give them self-documenting names that describe their purposes.

xster · 2019-03-27T18:43:10Z

+    // The scene is drawn by a CustomPaint, but user interaction is handled by
+    // the TransformInteraction parent widget.
+    return Scaffold(
+      appBar: AppBar(),


somewhat surprised you don't need a title here

I guess not. I only added this because some smoke tests look for an app bar with a back button. Maybe I should add a title...

xster · 2019-03-27T18:46:23Z

+      body: LayoutBuilder(
+        builder: (BuildContext context, BoxConstraints constraints) {
+          final Size size = Size(constraints.maxWidth, constraints.maxHeight);
+          final Size visibleSize = Size(size.width * 3, size.height * 2);


This line needs a comment. I assume this is just an arbitrary size of the board you're defining?

Yeah, this is how much the user can see beyond the size of the widget by panning around before hitting a "wall". This size just feels nice for the demo. I'll add a comment.

xster · 2019-03-27T18:47:15Z

@@ -10,6 +10,7 @@ export 'cupertino/cupertino.dart';
 export 'fortnightly/fortnightly.dart';


what's the file above? is it intentional?

Fortnightly is a news app demo in the gallery. I didn't change anything about it.

Not fortnightly, I can't comment on the file but it's default.profraw

Oh got it. Good catch! I must have accidentally added it. Will remove.

xster · 2019-03-27T19:04:48Z

+  final Widget child;
+  final Size size;
+  final bool reset;
+  final Function onTapDown;


Reuse the signatures of the recognizers instead of blank functions.

Whoops, good call.

xster · 2019-03-27T19:05:22Z

+    this.onScaleEnd,
+  }) :
+    assert(!reset || onResetEnd != null, 'Must implement onResetEnd to use reset.');
+


What does null min/max scales, disableTranslations, initialTranslations etc etc mean?

xster · 2019-03-27T19:05:57Z

+  @override TransformInteractionState createState() => TransformInteractionState();
+}
+
+enum GestureType {


Why is this public? Document what this is for.

xster · 2019-03-27T19:06:27Z

+  rotate,
+}
+
+class TransformInteractionState extends State<TransformInteraction> with TickerProviderStateMixin {


Why is this public? Who is expected to call its APIs? Document them

I just realized that I made this public so that I can access it in a unit test. Is that valid idea or is there a better way?

xster · 2019-03-27T19:14:18Z

+      }
+    });
+  }
+  void _onScaleEnd(ScaleEndDetails details) {


…ranslation at boundary (not ideal)

review fixes

justinmc · 2019-04-01T21:35:15Z

Ready for rereview.

@HansMuller one outstanding thing: I need some help using your _callbackValue function now that the surrounding code has changed.

And @xster: See comment #25164 (comment)

Hixie · 2019-04-01T21:39:01Z

You can make the State private and grab it via "dynamic". If you make the State public you are committing to that as a public API.

…

On Mon, Apr 1, 2019, 14:29 Justin McCandless ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/flutter_gallery/lib/demo/pan_and_zoom_demo_transform_interaction.dart <#25164 (comment)>: > + final bool disableScale; + final bool disableRotation; + final Offset initialTranslation; + final double initialScale; + final double initialRotation; + + @OverRide TransformInteractionState createState() => TransformInteractionState(); +} + +enum GestureType { + translate, + scale, + rotate, +} + +class TransformInteractionState extends State<TransformInteraction> with TickerProviderStateMixin { I just realized that I made this public so that I can access it in a unit test. Is that valid idea or is there a better way? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25164 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAhpHNP87xt3eb7MSbWn6wCsWwb12n4Kks5vcnpKgaJpZM4ZL-pn> .

HansMuller

This is really close; just a handful of trivial feedback and a suggestion about factoring GestureTransformableState

HansMuller · 2019-04-02T16:54:14Z

+    if (q <= 0) {
+      rStart = -boardRadius - q;
+      rEnd = boardRadius;
+    } else if (q > 0) {


I think this is just "else"

HansMuller · 2019-04-02T16:59:41Z

+    @required this.selected,
+    this.onTap,
+  }) : assert(color != null),
+      assert(selected != null);


one more space

HansMuller · 2019-04-02T17:00:57Z

+    @required this.boardPoint,
+    this.onColorSelection,
+  }) : assert(boardPoint != null),
+      super(key: key);


one more space

HansMuller · 2019-04-02T17:42:42Z

+import 'transformations_demo_inertial_motion.dart';
+
+// Empty function type for the onResetEnd parameter.
+typedef _ResetEndCallback = void Function();


This isn't needed, you can use VoidCallback

Thanks, I didn't know about that.

HansMuller · 2019-04-02T17:43:55Z

+    this.onScaleStart,
+    this.onScaleUpdate,
+    this.onScaleEnd,
+  }) :


Sadly, conventional formatting for the asserts is:

.... this.onScaleEnd, }) : assert(child != null), assert(size != null), ....

I really had trouble with my assert indentation in this PR...

HansMuller · 2019-04-02T17:45:08Z

+// original position of the child.
+@immutable
+class GestureTransformable extends StatefulWidget {
+  const GestureTransformable({


Widget classes should usually start with a Key key named parameter that's passed along via super(key: key)

HansMuller · 2019-04-02T17:46:25Z

+  }
+
+  // Get the offset of the current widget from the global screen coordinates.
+  static Offset getOffset(BuildContext context) {


Eventually we'll have to explain when it's safe to call this (not at build time).

At build time you wouldn't have a context, so is that enough discouragement from calling this at the wrong time?

At build(BuildContext context) time is exactly when you might be tempted to call it.

At the moment this is just a private entry point within a demo. No need to worry about this yet.

Oh I see what you mean! I'll add a TODO so I remember this later.

HansMuller · 2019-04-02T17:51:25Z

+    // Translation is reversed (a positive translation moves the scene to the
+    // right, viewport to the left).
+    final Rect translationBoundaries = Rect.fromLTRB(
+      viewportBoundaries.right * -1 * scale,


Maybe:

-scale * viewportBoundaries.right,

etc

HansMuller · 2019-04-02T17:56:12Z

+import 'package:flutter/material.dart';
+import 'package:vector_math/vector_math.dart' show Vector2;
+
+// Provides calculations for an object moving with inertia and friction using


Eventually a version of this should probably exist in the physics library.

Now that you mention it I see we have friction_simulation.dart. I could probably reuse or extend that.

HansMuller · 2019-04-02T18:02:55Z

+
+  @override
+  Widget build (BuildContext context) {
+    if (widget.reset && _animationReset == null) {


This looks like work that should be done at initStateTime() and in didUpdateWidget() (when the widget.reset flag changes).

… these by moving function to another class

HansMuller · 2019-04-02T22:39:19Z

+    _controllerReset = AnimationController(
+      vsync: this,
+    );
+    if (widget.reset && _animationReset == null) {


This method only runs once when the State is constructed so _animationReset has to be null.

Good catch.

HansMuller

LGTM. Hooray!

justinmc self-assigned this Dec 10, 2018

googlebot added the cla: yes label Dec 10, 2018

zoechi added the customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. label Dec 10, 2018

zoechi added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Dec 18, 2018

This was referenced Jan 2, 2019

Make it easy to override Scrollable's gesture detectors, e.g. to enable scale+scroll #15011

Open

Request: Bi-directional scroll view #19207

Closed

justinmc force-pushed the pan-and-zoom-gallery-demo branch 2 times, most recently from 03f0fc3 to c9ec50e Compare January 7, 2019 08:48

zoechi mentioned this pull request Jan 9, 2019

Pan and zoom widget #12878

Closed

justinmc force-pushed the pan-and-zoom-gallery-demo branch 8 times, most recently from 0155eb6 to 204ea54 Compare January 17, 2019 16:37

justinmc added 12 commits January 17, 2019 12:24

Basic pan/zoom working by copying in and simplifying code from my demo

5be946e

app

Intuitive pinch-zoom and panning without scale problems

140bec1

Hex grid is back

a5cee13

Padding between hexagons

04f6532

Fix analyze error

8eb9343

Hacky inertial dragging without using animations

30239cf

Use an animation controller (incorrectly) instead of a timer loop

a22c7b4

Use animation values instead of measuring time

d4badb8

animation variables are private

d01b0f9

Animate between points instead of time

31acba7

Simplify animation and motion calculation code

915656f

Separate inertial motion calculation

857625e

HansMuller reviewed Mar 26, 2019

View reviewed changes

justinmc added 5 commits March 26, 2019 11:10

Code review cleanup in main demo file

0194ebb

Fix code review in Board and tighten up immutability

f494153

Code review improvements in colorpicker

22238bb

Overall code review fixes

874d645

Fix analyze errors

bb22596

xster reviewed Mar 27, 2019

View reviewed changes

justinmc added 2 commits March 27, 2019 15:09

Fix bouncing when colliding with boundary while rotated by stopping t…

e9b202a

…ranslation at boundary (not ideal)

Use original GestureDetector callback parameter objects, and other code

a3fd435

review fixes

justinmc force-pushed the pan-and-zoom-gallery-demo branch from eae6e10 to a3fd435 Compare April 1, 2019 20:21

justinmc added 5 commits April 1, 2019 13:31

Punctuation in comments

31234f1

Code review fixes

0d77b8c

Remove accidentally added file

3df3759

Rename TransformInteraction to GestureTransformable

39d824d

Rename demo to 'Transfomations'

5d2a1a2

HansMuller reviewed Apr 2, 2019

View reviewed changes

justinmc added 2 commits April 2, 2019 12:53

Code review cleanup

799b6c7

Remove unit test for now. Next PR needs much more testing. Can revive…

89e38ab

… these by moving function to another class

justinmc force-pushed the pan-and-zoom-gallery-demo branch from 98c71ae to 89e38ab Compare April 2, 2019 20:53

Handle on and off case for reset coming from parent

1375d67

justinmc force-pushed the pan-and-zoom-gallery-demo branch from a139c52 to 1375d67 Compare April 2, 2019 22:26

HansMuller reviewed Apr 2, 2019

View reviewed changes

HansMuller approved these changes Apr 2, 2019

View reviewed changes

Remove unnecessary condition

8328eee

justinmc merged commit 3ada502 into flutter:master Apr 4, 2019

justinmc deleted the pan-and-zoom-gallery-demo branch April 4, 2019 18:59

justinmc mentioned this pull request May 27, 2019

WIP GestureTransformable Widget #33382

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 7, 2021

		@@ -10,6 +10,7 @@ export 'cupertino/cupertino.dart';
		export 'fortnightly/fortnightly.dart';

Conversation

justinmc commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansMuller 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!

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!

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!

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!

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!

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!

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 Apr 1, 2019

Uh oh!

Hixie commented Apr 1, 2019 via email

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

justinmc commented Dec 10, 2018 •

edited

Loading

justinmc commented Dec 10, 2018 •

edited

Loading