From 9bb9dc0b282deed3022e9c42c49425e51ae2cd05 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Tue, 16 Aug 2016 16:37:06 -0700 Subject: [PATCH] Handle changing clip delegates (#5444) Turns out that previously we weren't updating the clip if the layout didn't change, even if the delegate was different. --- .../flutter/lib/src/rendering/proxy_box.dart | 52 ++++--- packages/flutter/test/widget/clip_test.dart | 145 +++++++++++++++++- 2 files changed, 175 insertions(+), 22 deletions(-) diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart index 585129d534..65d2d433cd 100644 --- a/packages/flutter/lib/src/rendering/proxy_box.dart +++ b/packages/flutter/lib/src/rendering/proxy_box.dart @@ -881,13 +881,11 @@ abstract class _RenderCustomClip extends RenderProxyBox { return; CustomClipper oldClipper = _clipper; _clipper = newClipper; - if (newClipper == null) { - assert(oldClipper != null); - markNeedsPaint(); - markNeedsSemanticsUpdate(onlyChanges: true); - } else if (oldClipper == null || + assert(newClipper != null || oldClipper != null); + if (newClipper == null || oldClipper == null || oldClipper.runtimeType != oldClipper.runtimeType || newClipper.shouldRepaint(oldClipper)) { + _clip = null; markNeedsPaint(); markNeedsSemanticsUpdate(onlyChanges: true); } @@ -898,12 +896,20 @@ abstract class _RenderCustomClip extends RenderProxyBox { @override void performLayout() { + final Size oldSize = hasSize ? size : null; super.performLayout(); - _clip = _clipper?.getClip(size) ?? _defaultClip; + if (oldSize != size) + _clip = null; + } + + void _updateClip() { + _clip ??= _clipper?.getClip(size) ?? _defaultClip; } @override - Rect describeApproximatePaintClip(RenderObject child) => _clipper?.getApproximateClipRect(size) ?? Point.origin & size; + Rect describeApproximatePaintClip(RenderObject child) { + return _clipper?.getApproximateClipRect(size) ?? Point.origin & size; + } } /// Clips its child using a rectangle. @@ -927,8 +933,8 @@ class RenderClipRect extends _RenderCustomClip { @override bool hitTest(HitTestResult result, { Point position }) { if (_clipper != null) { - Rect clipRect = _clip; - if (!clipRect.contains(position)) + assert(_clip != null); + if (!_clip.contains(position)) return false; } return super.hitTest(result, position: position); @@ -936,8 +942,10 @@ class RenderClipRect extends _RenderCustomClip { @override void paint(PaintingContext context, Offset offset) { - if (child != null) + if (child != null) { + _updateClip(); context.pushClipRect(needsCompositing, offset, _clip, super.paint); + } } } @@ -974,6 +982,7 @@ class RenderClipRRect extends RenderProxyBox { // TODO(ianh): either convert this to the CustomClipper world, or // TODO(ianh): implement describeApproximatePaintClip for this class + // TODO(ianh): implement hit testing for this class @override void paint(PaintingContext context, Offset offset) { @@ -1016,11 +1025,11 @@ class RenderClipOval extends _RenderCustomClip { @override bool hitTest(HitTestResult result, { Point position }) { - Rect clipBounds = _clip; - Point center = clipBounds.center; + assert(_clip != null); + Point center = _clip.center; // convert the position to an offset from the center of the unit circle - Offset offset = new Offset((position.x - center.x) / clipBounds.width, - (position.y - center.y) / clipBounds.height); + Offset offset = new Offset((position.x - center.x) / _clip.width, + (position.y - center.y) / _clip.height); // check if the point is outside the unit circle if (offset.distanceSquared > 0.25) // x^2 + y^2 > r^2 return false; @@ -1030,8 +1039,8 @@ class RenderClipOval extends _RenderCustomClip { @override void paint(PaintingContext context, Offset offset) { if (child != null) { - Rect clipBounds = _clip; - context.pushClipPath(needsCompositing, offset, clipBounds, _getClipPath(clipBounds), super.paint); + _updateClip(); + context.pushClipPath(needsCompositing, offset, _clip, _getClipPath(_clip), super.paint); } } } @@ -1064,15 +1073,20 @@ class RenderClipPath extends _RenderCustomClip { @override bool hitTest(HitTestResult result, { Point position }) { - if (_clip == null || !_clip.contains(position)) - return false; + if (_clipper != null) { + assert(_clip != null); + if (!_clip.contains(position)) + return false; + } return super.hitTest(result, position: position); } @override void paint(PaintingContext context, Offset offset) { - if (child != null) + if (child != null) { + _updateClip(); context.pushClipPath(needsCompositing, offset, Point.origin & size, _clip, super.paint); + } } } diff --git a/packages/flutter/test/widget/clip_test.dart b/packages/flutter/test/widget/clip_test.dart index 96a13f0f90..d6d37f5d52 100644 --- a/packages/flutter/test/widget/clip_test.dart +++ b/packages/flutter/test/widget/clip_test.dart @@ -15,7 +15,25 @@ class PathClipper extends CustomClipper { ..addRect(new Rect.fromLTWH(50.0, 50.0, 100.0, 100.0)); } @override - bool shouldRepaint(PathClipper oldWidget) => false; + bool shouldRepaint(PathClipper oldClipper) => false; +} + +class ValueClipper extends CustomClipper { + ValueClipper(this.message, this.value); + + final String message; + final T value; + + @override + T getClip(Size size) { + log.add(message); + return value; + } + + @override + bool shouldRepaint(ValueClipper oldClipper) { + return oldClipper.message != message || oldClipper.value != value; + } } void main() { @@ -26,7 +44,6 @@ void main() { child: new GestureDetector( behavior: HitTestBehavior.opaque, onTap: () { log.add('tap'); }, - child: new Container() ) ) ); @@ -47,7 +64,6 @@ void main() { child: new GestureDetector( behavior: HitTestBehavior.opaque, onTap: () { log.add('tap'); }, - child: new Container() ) ) ); @@ -61,4 +77,127 @@ void main() { expect(log, equals(['tap'])); log.clear(); }); + + testWidgets('ClipRect', (WidgetTester tester) async { + await tester.pumpWidget( + new Align( + alignment: FractionalOffset.topLeft, + child: new SizedBox( + width: 100.0, + height: 100.0, + child: new ClipRect( + clipper: new ValueClipper('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)), + child: new GestureDetector( + behavior: HitTestBehavior.opaque, + onTap: () { log.add('tap'); }, + ) + ) + ) + ) + ); + expect(log, equals(['a'])); + + await tester.tapAt(new Point(10.0, 10.0)); + expect(log, equals(['a', 'tap'])); + + await tester.tapAt(new Point(100.0, 100.0)); + expect(log, equals(['a', 'tap'])); + + await tester.pumpWidget( + new Align( + alignment: FractionalOffset.topLeft, + child: new SizedBox( + width: 100.0, + height: 100.0, + child: new ClipRect( + clipper: new ValueClipper('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)), + child: new GestureDetector( + behavior: HitTestBehavior.opaque, + onTap: () { log.add('tap'); }, + ) + ) + ) + ) + ); + expect(log, equals(['a', 'tap'])); + + await tester.pumpWidget( + new Align( + alignment: FractionalOffset.topLeft, + child: new SizedBox( + width: 200.0, + height: 200.0, + child: new ClipRect( + clipper: new ValueClipper('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)), + child: new GestureDetector( + behavior: HitTestBehavior.opaque, + onTap: () { log.add('tap'); }, + ) + ) + ) + ) + ); + expect(log, equals(['a', 'tap', 'a'])); + + await tester.pumpWidget( + new Align( + alignment: FractionalOffset.topLeft, + child: new SizedBox( + width: 200.0, + height: 200.0, + child: new ClipRect( + clipper: new ValueClipper('a', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)), + child: new GestureDetector( + behavior: HitTestBehavior.opaque, + onTap: () { log.add('tap'); }, + ) + ) + ) + ) + ); + expect(log, equals(['a', 'tap', 'a'])); + + await tester.pumpWidget( + new Align( + alignment: FractionalOffset.topLeft, + child: new SizedBox( + width: 200.0, + height: 200.0, + child: new ClipRect( + clipper: new ValueClipper('b', new Rect.fromLTWH(5.0, 5.0, 10.0, 10.0)), + child: new GestureDetector( + behavior: HitTestBehavior.opaque, + onTap: () { log.add('tap'); }, + ) + ) + ) + ) + ); + expect(log, equals(['a', 'tap', 'a', 'b'])); + + await tester.pumpWidget( + new Align( + alignment: FractionalOffset.topLeft, + child: new SizedBox( + width: 200.0, + height: 200.0, + child: new ClipRect( + clipper: new ValueClipper('c', new Rect.fromLTWH(25.0, 25.0, 10.0, 10.0)), + child: new GestureDetector( + behavior: HitTestBehavior.opaque, + onTap: () { log.add('tap'); }, + ) + ) + ) + ) + ); + expect(log, equals(['a', 'tap', 'a', 'b', 'c'])); + + await tester.tapAt(new Point(30.0, 30.0)); + expect(log, equals(['a', 'tap', 'a', 'b', 'c', 'tap'])); + + await tester.tapAt(new Point(100.0, 100.0)); + expect(log, equals(['a', 'tap', 'a', 'b', 'c', 'tap'])); + }); + }