From 0d983cd87ab7e38346fec9bef8e501e3bdbed34e Mon Sep 17 00:00:00 2001 From: Hans Muller Date: Mon, 14 Dec 2015 15:48:58 -0800 Subject: [PATCH] CustomMultiChildLayout and CustomOneChildLayout now use their delegate's shouldRelayout() method instead of a "token" to decide if layout is needed. MultiChildLayoutDelegate and OnChildLayoutDelegate are now expected to be stateless, i.e. they'll typically be built each time their custom layout widget is built. If the identical layout delegate is provided to a new custom layout, layout will not happen. Revised the bottom sheet implementation per the new custom layout classes. Removed a SizeObserver. Fixes #899 --- .../lib/src/material/bottom_sheet.dart | 57 +++++++++++-------- .../flutter/lib/src/material/scaffold.dart | 15 +---- .../lib/src/rendering/custom_layout.dart | 6 +- .../lib/src/rendering/shifted_box.dart | 6 +- packages/flutter/lib/src/widgets/basic.dart | 17 +----- .../custom_multi_child_layout_test.dart | 53 ++++++++++++++--- .../widget/custom_one_child_layout_test.dart | 41 ++++++++++++- 7 files changed, 129 insertions(+), 66 deletions(-) diff --git a/packages/flutter/lib/src/material/bottom_sheet.dart b/packages/flutter/lib/src/material/bottom_sheet.dart index 382f69a9cf..efb8c2fab7 100644 --- a/packages/flutter/lib/src/material/bottom_sheet.dart +++ b/packages/flutter/lib/src/material/bottom_sheet.dart @@ -18,12 +18,11 @@ const double _kCloseProgressThreshold = 0.5; const Color _kTransparent = const Color(0x00000000); const Color _kBarrierColor = Colors.black54; -class BottomSheet extends StatelessComponent { +class BottomSheet extends StatefulComponent { BottomSheet({ Key key, this.performance, this.onClosing, - this.childHeight, this.builder }) : super(key: key) { assert(onClosing != null); @@ -34,37 +33,48 @@ class BottomSheet extends StatelessComponent { /// passive observer. final Performance performance; final VoidCallback onClosing; - final double childHeight; final WidgetBuilder builder; + _BottomSheetState createState() => new _BottomSheetState(); + static Performance createPerformanceController() { return new Performance( duration: _kBottomSheetDuration, debugLabel: 'BottomSheet' ); } +} - bool get _dismissUnderway => performance.direction == AnimationDirection.reverse; +class _BottomSheetState extends State { + + final _childKey = new GlobalKey(debugLabel: 'BottomSheet child'); + + double get _childHeight { + final RenderBox renderBox = _childKey.currentContext.findRenderObject(); + return renderBox.size.height; + } + + bool get _dismissUnderway => config.performance.direction == AnimationDirection.reverse; void _handleDragUpdate(double delta) { if (_dismissUnderway) return; - performance.progress -= delta / (childHeight ?? delta); + config.performance.progress -= delta / (_childHeight ?? delta); } void _handleDragEnd(Offset velocity) { if (_dismissUnderway) return; if (velocity.dy > _kMinFlingVelocity) { - double flingVelocity = -velocity.dy / childHeight; - performance.fling(velocity: flingVelocity); + double flingVelocity = -velocity.dy / _childHeight; + config.performance.fling(velocity: flingVelocity); if (flingVelocity < 0.0) - onClosing(); - } else if (performance.progress < _kCloseProgressThreshold) { - performance.fling(velocity: -1.0); - onClosing(); + config.onClosing(); + } else if (config.performance.progress < _kCloseProgressThreshold) { + config.performance.fling(velocity: -1.0); + config.onClosing(); } else { - performance.forward(); + config.performance.forward(); } } @@ -73,7 +83,8 @@ class BottomSheet extends StatelessComponent { onVerticalDragUpdate: _handleDragUpdate, onVerticalDragEnd: _handleDragEnd, child: new Material( - child: builder(context) + key: _childKey, + child: config.builder(context) ) ); } @@ -87,8 +98,9 @@ class BottomSheet extends StatelessComponent { // MODAL BOTTOM SHEETS class _ModalBottomSheetLayout extends OneChildLayoutDelegate { - // The distance from the bottom of the parent to the top of the BottomSheet child. - AnimatedValue childTop = new AnimatedValue(0.0); + _ModalBottomSheetLayout(this.progress); + + final double progress; BoxConstraints getConstraintsForChild(BoxConstraints constraints) { return new BoxConstraints( @@ -100,8 +112,11 @@ class _ModalBottomSheetLayout extends OneChildLayoutDelegate { } Point getPositionForChild(Size size, Size childSize) { - childTop.end = childSize.height; - return new Point(0.0, size.height - childTop.value); + return new Point(0.0, size.height - childSize.height * progress); + } + + bool shouldRelayout(_ModalBottomSheetLayout oldDelegate) { + return progress != oldDelegate.progress; } } @@ -114,24 +129,18 @@ class _ModalBottomSheet extends StatefulComponent { } class _ModalBottomSheetState extends State<_ModalBottomSheet> { - - final _ModalBottomSheetLayout _layout = new _ModalBottomSheetLayout(); - Widget build(BuildContext context) { return new GestureDetector( onTap: () => Navigator.pop(context), child: new BuilderTransition( performance: config.route.performance, - variables: >[_layout.childTop], builder: (BuildContext context) { return new ClipRect( child: new CustomOneChildLayout( - delegate: _layout, - token: _layout.childTop.value, + delegate: new _ModalBottomSheetLayout(config.route.performance.progress), child: new BottomSheet( performance: config.route.performance, onClosing: () => Navigator.pop(context), - childHeight: _layout.childTop.end, builder: config.route.builder ) ) diff --git a/packages/flutter/lib/src/material/scaffold.dart b/packages/flutter/lib/src/material/scaffold.dart index bba04ed247..6e28b8391c 100644 --- a/packages/flutter/lib/src/material/scaffold.dart +++ b/packages/flutter/lib/src/material/scaffold.dart @@ -95,8 +95,6 @@ class _ScaffoldLayout extends MultiChildLayoutDelegate { } } -final _ScaffoldLayout _scaffoldLayout = new _ScaffoldLayout(); - class Scaffold extends StatefulComponent { Scaffold({ Key key, @@ -336,9 +334,8 @@ class ScaffoldState extends State { )); } - return new CustomMultiChildLayout(children, delegate: _scaffoldLayout); + return new CustomMultiChildLayout(children, delegate: new _ScaffoldLayout()); } - } class ScaffoldFeatureController { @@ -397,13 +394,6 @@ class _PersistentBottomSheetState extends State<_PersistentBottomSheet> { config.onDismissed(); } - double _childHeight; - void _updateChildHeight(Size newSize) { - setState(() { - _childHeight = newSize.height; - }); - } - Widget build(BuildContext context) { return new AlignTransition( performance: config.performance, @@ -412,8 +402,7 @@ class _PersistentBottomSheetState extends State<_PersistentBottomSheet> { child: new BottomSheet( performance: config.performance, onClosing: config.onClosing, - childHeight: _childHeight, - builder: (BuildContext context) => new SizeObserver(child: config.builder(context), onSizeChanged: _updateChildHeight) + builder: config.builder ) ); } diff --git a/packages/flutter/lib/src/rendering/custom_layout.dart b/packages/flutter/lib/src/rendering/custom_layout.dart index 6c192e264c..74f81c04d1 100644 --- a/packages/flutter/lib/src/rendering/custom_layout.dart +++ b/packages/flutter/lib/src/rendering/custom_layout.dart @@ -91,6 +91,9 @@ abstract class MultiChildLayoutDelegate { } } + /// Override this method to return true when the children need to be laid out. + bool shouldRelayout(MultiChildLayoutDelegate oldDelegate) => true; + /// Layout and position all children given this widget's size and the specified /// constraints. This method must apply layoutChild() to each child. It should /// specify the final position of each child with positionChild(). @@ -126,8 +129,9 @@ class RenderCustomMultiChildLayoutBox extends RenderBox assert(newDelegate != null); if (_delegate == newDelegate) return; + if (newDelegate.runtimeType != _delegate.runtimeType || newDelegate.shouldRelayout(_delegate)) + markNeedsLayout(); _delegate = newDelegate; - markNeedsLayout(); } Size _getSize(BoxConstraints constraints) { diff --git a/packages/flutter/lib/src/rendering/shifted_box.dart b/packages/flutter/lib/src/rendering/shifted_box.dart index 66d56c6eeb..85ee3829ca 100644 --- a/packages/flutter/lib/src/rendering/shifted_box.dart +++ b/packages/flutter/lib/src/rendering/shifted_box.dart @@ -257,6 +257,9 @@ class OneChildLayoutDelegate { /// Returns the position where the child should be placed given the size of this object and the size of the child. Point getPositionForChild(Size size, Size childSize) => Point.origin; + + /// Override this method to return true when the child needs to be laid out. + bool shouldRelayout(OneChildLayoutDelegate oldDelegate) => true; } /// Defers the layout of its single child to a delegate. @@ -280,8 +283,9 @@ class RenderCustomOneChildLayoutBox extends RenderShiftedBox { assert(newDelegate != null); if (_delegate == newDelegate) return; + if (newDelegate.runtimeType != _delegate.runtimeType || newDelegate.shouldRelayout(_delegate)) + markNeedsLayout(); _delegate = newDelegate; - markNeedsLayout(); } Size _getSize(BoxConstraints constraints) { diff --git a/packages/flutter/lib/src/widgets/basic.dart b/packages/flutter/lib/src/widgets/basic.dart index d3ca5d8cf4..c6ffb64254 100644 --- a/packages/flutter/lib/src/widgets/basic.dart +++ b/packages/flutter/lib/src/widgets/basic.dart @@ -386,27 +386,16 @@ class CustomOneChildLayout extends OneChildRenderObjectWidget { CustomOneChildLayout({ Key key, this.delegate, - this.token, Widget child }) : super(key: key, child: child) { assert(delegate != null); } - /// A long-lived delegate that controls the layout of this widget. - /// - /// Whenever the delegate changes, we need to recompute the layout of this - /// widget, which means you might not want to create a new delegate instance - /// every time you build this widget. Instead, consider using a long-lived - /// deletate (perhaps held in a component's state) that you re-use every time - /// you build this widget. final OneChildLayoutDelegate delegate; - final Object token; RenderCustomOneChildLayoutBox createRenderObject() => new RenderCustomOneChildLayoutBox(delegate: delegate); void updateRenderObject(RenderCustomOneChildLayoutBox renderObject, CustomOneChildLayout oldWidget) { - if (oldWidget.token != token) - renderObject.markNeedsLayout(); renderObject.delegate = delegate; } } @@ -458,23 +447,19 @@ class LayoutId extends ParentDataWidget { class CustomMultiChildLayout extends MultiChildRenderObjectWidget { CustomMultiChildLayout(List children, { Key key, - this.delegate, - this.token + this.delegate }) : super(key: key, children: children) { assert(delegate != null); } /// The delegate that controls the layout of the children. final MultiChildLayoutDelegate delegate; - final Object token; RenderCustomMultiChildLayoutBox createRenderObject() { return new RenderCustomMultiChildLayoutBox(delegate: delegate); } void updateRenderObject(RenderCustomMultiChildLayoutBox renderObject, CustomMultiChildLayout oldWidget) { - if (oldWidget.token != token) - renderObject.markNeedsLayout(); renderObject.delegate = delegate; } } diff --git a/packages/unit/test/widget/custom_multi_child_layout_test.dart b/packages/unit/test/widget/custom_multi_child_layout_test.dart index 96c83a8de4..27a07f53c7 100644 --- a/packages/unit/test/widget/custom_multi_child_layout_test.dart +++ b/packages/unit/test/widget/custom_multi_child_layout_test.dart @@ -30,20 +30,32 @@ class TestMultiChildLayoutDelegate extends MultiChildLayoutDelegate { performLayoutIsChild = isChild('fred'); }, returnsNormally); } + + bool shouldRelayoutCalled = false; + bool shouldRelayoutValue = false; + bool shouldRelayout(_) { + shouldRelayoutCalled = true; + return shouldRelayoutValue; + } } +Widget buildFrame(MultiChildLayoutDelegate delegate) { + return new Center( + child: new CustomMultiChildLayout([ + new LayoutId(id: 0, child: new Container(width: 150.0, height: 100.0)), + new LayoutId(id: 1, child: new Container(width: 100.0, height: 200.0)), + ], + delegate: delegate + ) + ); +} + + void main() { test('Control test for CustomMultiChildLayout', () { testWidgets((WidgetTester tester) { TestMultiChildLayoutDelegate delegate = new TestMultiChildLayoutDelegate(); - tester.pumpWidget(new Center( - child: new CustomMultiChildLayout([ - new LayoutId(id: 0, child: new Container(width: 150.0, height: 100.0)), - new LayoutId(id: 1, child: new Container(width: 100.0, height: 200.0)), - ], - delegate: delegate - ) - )); + tester.pumpWidget(buildFrame(delegate)); expect(delegate.getSizeConstraints.minWidth, 0.0); expect(delegate.getSizeConstraints.maxWidth, 800.0); @@ -64,6 +76,31 @@ void main() { }); }); + test('Test MultiChildDelegate shouldRelayout method', () { + testWidgets((WidgetTester tester) { + TestMultiChildLayoutDelegate delegate = new TestMultiChildLayoutDelegate(); + tester.pumpWidget(buildFrame(delegate)); + + // Layout happened because the delegate was set. + expect(delegate.performLayoutSize, isNotNull); // i.e. layout happened + expect(delegate.shouldRelayoutCalled, isFalse); + + // Layout did not happen because shouldRelayout() returned false. + delegate = new TestMultiChildLayoutDelegate(); + delegate.shouldRelayoutValue = false; + tester.pumpWidget(buildFrame(delegate)); + expect(delegate.shouldRelayoutCalled, isTrue); + expect(delegate.performLayoutSize, isNull); + + // Layout happened because shouldRelayout() returned true. + delegate = new TestMultiChildLayoutDelegate(); + delegate.shouldRelayoutValue = true; + tester.pumpWidget(buildFrame(delegate)); + expect(delegate.shouldRelayoutCalled, isTrue); + expect(delegate.performLayoutSize, isNotNull); + }); + }); + test('Nested CustomMultiChildLayouts', () { testWidgets((WidgetTester tester) { TestMultiChildLayoutDelegate delegate = new TestMultiChildLayoutDelegate(); diff --git a/packages/unit/test/widget/custom_one_child_layout_test.dart b/packages/unit/test/widget/custom_one_child_layout_test.dart index 33dd94c0c8..a4990a246d 100644 --- a/packages/unit/test/widget/custom_one_child_layout_test.dart +++ b/packages/unit/test/widget/custom_one_child_layout_test.dart @@ -32,15 +32,24 @@ class TestOneChildLayoutDelegate extends OneChildLayoutDelegate { childSizeFromGetPositionForChild = childSize; return Point.origin; } + + bool shouldRelayoutCalled = false; + bool shouldRelayoutValue = false; + bool shouldRelayout(_) { + shouldRelayoutCalled = true; + return shouldRelayoutValue; + } +} + +Widget buildFrame(delegate) { + return new Center(child: new CustomOneChildLayout(delegate: delegate, child: new Container())); } void main() { test('Control test for CustomOneChildLayout', () { testWidgets((WidgetTester tester) { TestOneChildLayoutDelegate delegate = new TestOneChildLayoutDelegate(); - tester.pumpWidget(new Center( - child: new CustomOneChildLayout(delegate: delegate, child: new Container()) - )); + tester.pumpWidget(buildFrame(delegate)); expect(delegate.constraintsFromGetSize.minWidth, 0.0); expect(delegate.constraintsFromGetSize.maxWidth, 800.0); @@ -59,4 +68,30 @@ void main() { expect(delegate.childSizeFromGetPositionForChild.height, 400.0); }); }); + + test('Test OneChildDelegate shouldRelayout method', () { + testWidgets((WidgetTester tester) { + TestOneChildLayoutDelegate delegate = new TestOneChildLayoutDelegate(); + tester.pumpWidget(buildFrame(delegate)); + + // Layout happened because the delegate was set. + expect(delegate.constraintsFromGetConstraintsForChild, isNotNull); // i.e. layout happened + expect(delegate.shouldRelayoutCalled, isFalse); + + // Layout did not happen because shouldRelayout() returned false. + delegate = new TestOneChildLayoutDelegate(); + delegate.shouldRelayoutValue = false; + tester.pumpWidget(buildFrame(delegate)); + expect(delegate.shouldRelayoutCalled, isTrue); + expect(delegate.constraintsFromGetConstraintsForChild, isNull); + + // Layout happened because shouldRelayout() returned true. + delegate = new TestOneChildLayoutDelegate(); + delegate.shouldRelayoutValue = true; + tester.pumpWidget(buildFrame(delegate)); + expect(delegate.shouldRelayoutCalled, isTrue); + expect(delegate.constraintsFromGetConstraintsForChild, isNotNull); + }); + }); + }