Introduce a vertical/horizontal split layout #1261

DaveShuckerow · 2019-10-25T23:23:58Z

TODO:

Use a proper decoration for the divider thumb
Test to be sure we don't impose negative constraints on children in vertical or horizontal mode

jacob314 · 2019-10-26T17:29:21Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

@@ -113,3 +116,163 @@ class DefaultTaggedText extends StatelessWidget {
    );
  }
 }
+


maybe move this widget over to its own file under src/flutter. It is complex enough to be worth its own file. I would propose keeping comm_widgets.dart for widgets that don't really have complex business logic but are good to share.

SGTM, moved.

jacob314 · 2019-10-26T17:31:21Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

@@ -113,3 +116,163 @@ class DefaultTaggedText extends StatelessWidget {
    );
  }
 }
+
+/// A widget that takes two children and lays them out along one axis.


This summary is missing the key bit that the user resize the widgets along that axis.

Good catch, done.

jacob314 · 2019-10-26T17:33:06Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+  /// [firstChild] will be placed before [secondChild] in a [Row].
+  const Split.horizontal({
+    Key key,
+    @required Widget firstChild,


instead of firstChild and second child, why not use children:
If you only support two children you can always assert that there are exactly two children.

firstChild and secondChild explain exactly what the code does without a need for asserts.

It's simpler, and this pattern has prior art.

Unless we plan on supporting n != 2 children, I'd like to leave this as-is.

my guess is at some point we will want to support n > 2 children.

jacob314 · 2019-10-26T17:34:05Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+    Key key,
+    @required Widget firstChild,
+    @required Widget secondChild,
+    double initialFirstFraction,


you also need to track the minimum size allowed for each split component. that is critical to ensure that users don't accidentally get the UI in a weird state by collapsing one side of the split too much.

I've left a TODO to follow up on this.

jacob314 · 2019-10-26T17:35:42Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+          height: isHorizontal ? height : Split.dividerMainAxisSize,
+          child: const Center(
+            child: Text(
+              ':::::::',


use the existing png for this instead of using text.
Alternately, find a more material design appropriate splitter icon.

Naturally we aren't going to land that :D

Do you know the name of the png file for the splitter grip?

The splitter icons are currently stored as data urls in the app as that is the typical way they are included as the images are tiny.
You can also find the pngs here.
https://github.com/nathancahill/split/tree/master/packages/splitjs/grips

.gutter.gutter-horizontal { background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAeCAYAAADkftS9AAAAIklEQVQoU2M4c+bMfxAGAgYYmwGrIIiDjrELjpo5aiZeMwF+yNnOs5KSvgAAAABJRU5ErkJggg=='); cursor: col-resize; height: 100%; } .gutter.gutter-vertical { background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAB4AAAAFAQMAAABo7865AAAABlBMVEVHcEzMzMzyAv2sAAAAAXRSTlMAQObYZgAAABBJREFUeF5jOAMEEAIEEFwAn3kMwcB6I2AAAAAASUVORK5CYII='); cursor: row-resize; width: 100%; }

jacob314 · 2019-10-26T17:37:48Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+        child: widget.secondChild,
+      ),
+    ];
+    if (widget.axis == Axis.horizontal) {


this can be replaced with
Flex(direction: widget.axis, children: children)

Column and Row are just sugar around Flex with a fixed direction.

Good suggestion, done.

jacob314 · 2019-10-26T17:38:19Z

packages/devtools_app/test/flutter/split_test.dart

+      });
+    });
+
+    group('drags properly', () {


Great to see a test of this!

jacob314 · 2019-10-26T17:39:31Z

packages/devtools_app/test/flutter/split_test.dart

+  });
+}
+
+Widget _w1, _w2;


nit: instead of making _w1, _w2, and _split
surprise side effects of
buildSplitAndChildren why not use keys and lookup the widgets bsed on keys instead of using
find.byWidget?

jacob314 · 2019-10-26T17:40:28Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+      final fractionalDelta = delta / axisSize;
+      setState(() {
+        // Update the fraction of space consumed by the children,
+        // being sure not to allocate any of them negative space.


here is where we need to instead clamp so no children violate their minSize constraint.

jacob314

feel free to apply the feature request to specify a minWidth for each column as a followup cl.

kenzieschmoll · 2019-10-28T14:28:57Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+    this.firstChild,
+    this.secondChild,
+    double initialFirstFraction,
+  )   : initialFirstFraction = initialFirstFraction ?? 0.5,


Does ??= work for this case?

No. This is equivalent to:

this.initialFirstFraction = initialFirstFraction ?? 0.5

The two variables are different, even though they have the same name.

kenzieschmoll · 2019-10-28T14:35:27Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+    const halfDivider = Split.dividerMainAxisSize / 2.0;
+    // The fraction of the layout the divider needs to take up from each child.
+    final halfDividerFraction = halfDivider / axisSize;
+


Why are we using fractions instead of just subtracting Split.dividerMainAxisSize / 2.0?

Good suggestion, I've removed this field.

kenzieschmoll · 2019-10-28T14:36:26Z

packages/devtools_app/lib/src/flutter/common_widgets.dart

+    final halfDividerFraction = halfDivider / axisSize;
+
+    final sanitizedFirstFraction =
+        firstFraction.clamp(halfDividerFraction, 1.0 - halfDividerFraction);


Shouldn't the lower bound of the clamp be 0?

We need to make sure there is enough space to take halfDivider from each child, no matter how far we have slid the divider.

To do that, we have to clamp the available space to at least halfDivider for both children, then we can remove the halfDivider space.

I've updated the comments to explain, let me know if you have more questions. The geometry took a while to explain even explaining it to myself, so feedback here is appreciated.

jacob314 · 2019-10-28T17:21:15Z

One other high level comment. It would be nice to change the cursor matching how the cursor is changed by the existing UI when interacting with the spiller on desktop and web. If you want to do it with a follow up cl, feel free to add a tracking bug.

DaveShuckerow · 2019-10-29T22:11:27Z

One other high level comment. It would be nice to change the cursor matching how the cursor is changed by the existing UI when interacting with the spiller on desktop and web. If you want to do it with a follow up cl, feel free to add a tracking bug.

#1265

DaveShuckerow · 2019-10-29T22:13:01Z

Test run failures were caused by #1267 and #1266. This doesn't affect either, so I'm going to merge.

Dave Shuckerow added 5 commits October 25, 2019 15:49

Introduce a split widget

6ad3669

Introduce a spacer

6567c00

Add in dragging to the spacer

91583b0

Add in dragging to the spacer

4eb822d

Add in dragging to the spacer

f2d1ddf

DaveShuckerow added the hummingbird Hummingbird implementation-related label Oct 25, 2019

Dave Shuckerow added 3 commits October 25, 2019 16:43

Cleanup lints

604e152

Add tests

0a8ea92

Add resizing tests

1f5008b

jacob314 reviewed Oct 26, 2019

View reviewed changes

packages/devtools_app/test/flutter/split_test.dart

});

});

group('drags properly', () {

Copy link

Contributor

jacob314 Oct 26, 2019

Choose a reason for hiding this comment

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

Great to see a test of this!

jacob314 reviewed Oct 26, 2019

View reviewed changes

kenzieschmoll reviewed Oct 28, 2019

View reviewed changes

Dave Shuckerow added 5 commits October 29, 2019 12:10

Move split into its own file

6561838

Simplify construction of splits

0eac47a

Simplify construction of splits and add a drag indicator widget.

8fd4004

Merge branch 'split' of github.com:DaveShuckerow/devtools into split

e5c116d

Explain better how the layout is calculated

960f58e

DaveShuckerow merged commit 4384ed3 into flutter:master Oct 29, 2019

DaveShuckerow deleted the split branch October 31, 2019 23:09

DaveShuckerow added this to Done in Hummingbird Dashboard Nov 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a vertical/horizontal split layout #1261

Introduce a vertical/horizontal split layout #1261

DaveShuckerow commented Oct 25, 2019 •

edited

jacob314 Oct 26, 2019

DaveShuckerow Oct 29, 2019

jacob314 Oct 26, 2019

DaveShuckerow Oct 29, 2019

jacob314 Oct 26, 2019

DaveShuckerow Oct 29, 2019

jacob314 Oct 29, 2019

jacob314 Oct 26, 2019

DaveShuckerow Oct 29, 2019

jacob314 Oct 26, 2019

DaveShuckerow Oct 29, 2019

jacob314 Oct 29, 2019

jacob314 Oct 26, 2019

DaveShuckerow Oct 29, 2019

jacob314 Oct 26, 2019

jacob314 Oct 26, 2019

jacob314 Oct 26, 2019

jacob314 left a comment

kenzieschmoll Oct 28, 2019

DaveShuckerow Oct 29, 2019

kenzieschmoll Oct 28, 2019

DaveShuckerow Oct 29, 2019

kenzieschmoll Oct 28, 2019

DaveShuckerow Oct 29, 2019

jacob314 commented Oct 28, 2019

DaveShuckerow commented Oct 29, 2019

DaveShuckerow commented Oct 29, 2019

@@ @@ -113,3 +116,163 @@ class DefaultTaggedText extends StatelessWidget { @@
                   );
                 }
               }

Introduce a vertical/horizontal split layout #1261

Introduce a vertical/horizontal split layout #1261

Conversation

DaveShuckerow commented Oct 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 commented Oct 28, 2019

DaveShuckerow commented Oct 29, 2019

DaveShuckerow commented Oct 29, 2019

DaveShuckerow commented Oct 25, 2019 •

edited