From a11da307bfb0df00f474de12cb2d5afe07ac291c Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Thu, 4 May 2023 17:34:04 -0700 Subject: [PATCH] Minor fixes found while working on blankcanvas (#125751) This PR contains a series of minor changes to address issues that I happened to run into: - Pretty-print errors triggered when handling events that are pending because of having locked event handling. Previously these were just dumped to the console. - Add more documentation for `debugPaintPadding`. - Add documentation to `Tween` saying how to implement it. - Slight formatting changes in the scrollbar code to align some expressions. - Since we convert ScrollMetricsNotifications to ScrollNotifications in various places, provide an explicit API to do this. This will make the behaviour consistent throughout, and makes the code easier to understand. Added a test for this. - Clarifications to some of the BindingBase and SchedulerBinding documentation. - Clarified some documentation in `flutter_test`'s `Finder` class. --- packages/flutter/lib/src/animation/tween.dart | 6 +++++ .../flutter/lib/src/foundation/binding.dart | 18 ++++++++++++- packages/flutter/lib/src/rendering/debug.dart | 13 ++++++++-- .../flutter/lib/src/scheduler/binding.dart | 15 ++++++----- .../widgets/scroll_notification_observer.dart | 20 +++----------- .../lib/src/widgets/scroll_position.dart | 26 ++++++++++++++----- .../flutter/lib/src/widgets/scrollbar.dart | 20 ++++++-------- .../test/widgets/notification_test.dart | 18 +++++++++++++ packages/flutter_test/lib/src/finders.dart | 3 +++ 9 files changed, 94 insertions(+), 45 deletions(-) diff --git a/packages/flutter/lib/src/animation/tween.dart b/packages/flutter/lib/src/animation/tween.dart index 119288974a7..1f0bb5d387a 100644 --- a/packages/flutter/lib/src/animation/tween.dart +++ b/packages/flutter/lib/src/animation/tween.dart @@ -248,6 +248,12 @@ class _ChainedEvaluation extends Animatable { /// If `T` is not nullable, then [begin] and [end] must both be set to /// non-null values before using [lerp] or [transform], otherwise they /// will throw. +/// +/// ## Implementing a Tween +/// +/// To specialize this class for a new type, the subclass should implement +/// the [lerp] method (and a constructor). The other methods of this class +/// are all defined in terms of [lerp]. class Tween extends Animatable { /// Creates a tween. /// diff --git a/packages/flutter/lib/src/foundation/binding.dart b/packages/flutter/lib/src/foundation/binding.dart index ae98c7ae14f..3b19fc2cc8e 100644 --- a/packages/flutter/lib/src/foundation/binding.dart +++ b/packages/flutter/lib/src/foundation/binding.dart @@ -644,6 +644,11 @@ abstract class BindingBase { /// (which it partially does asynchronously). /// /// The [Future] returned by the `callback` argument is returned by [lockEvents]. + /// + /// The [gestures] binding wraps [PlatformDispatcher.onPointerDataPacket] in + /// logic that honors this event locking mechanism. Similarly, tasks queued + /// using [SchedulerBinding.scheduleTask] will only start when events are not + /// [locked]. @protected Future lockEvents(Future Function() callback) { final developer.TimelineTask timelineTask = developer.TimelineTask()..start('Lock events'); @@ -654,7 +659,16 @@ abstract class BindingBase { _lockCount -= 1; if (!locked) { timelineTask.finish(); - unlocked(); + try { + unlocked(); + } catch (error, stack) { + FlutterError.reportError(FlutterErrorDetails( + exception: error, + stack: stack, + library: 'foundation', + context: ErrorDescription('while handling pending events'), + )); + } } }); return future; @@ -816,6 +830,8 @@ abstract class BindingBase { /// All events dispatched by a [BindingBase] use this method instead of /// calling [developer.postEvent] directly so that tests for [BindingBase] /// can track which events were dispatched by overriding this method. + /// + /// This is unrelated to the events managed by [lockEvents]. @protected void postEvent(String eventKind, Map eventData) { developer.postEvent(eventKind, eventData); diff --git a/packages/flutter/lib/src/rendering/debug.dart b/packages/flutter/lib/src/rendering/debug.dart index 3b8250fd36c..f07293415a4 100644 --- a/packages/flutter/lib/src/rendering/debug.dart +++ b/packages/flutter/lib/src/rendering/debug.dart @@ -260,8 +260,17 @@ void _debugDrawDoubleRect(Canvas canvas, Rect outerRect, Rect innerRect, Color c /// Paint a diagram showing the given area as padding. /// -/// Called by [RenderPadding.debugPaintSize] when [debugPaintSizeEnabled] is -/// true. +/// The `innerRect` argument represents the position of the child, if any. +/// +/// When `innerRect` is null, the method draws the entire `outerRect` in a +/// grayish color representing _spacing_. +/// +/// When `innerRect` is non-null, the method draws the padding region around the +/// `innerRect` in a tealish color, with a solid outline around the inner +/// region. +/// +/// This method is used by [RenderPadding.debugPaintSize] when +/// [debugPaintSizeEnabled] is true. void debugPaintPadding(Canvas canvas, Rect outerRect, Rect? innerRect, { double outlineWidth = 2.0 }) { assert(() { if (innerRect != null && !innerRect.isEmpty) { diff --git a/packages/flutter/lib/src/scheduler/binding.dart b/packages/flutter/lib/src/scheduler/binding.dart index c6a959e3f1c..9d58a91106e 100644 --- a/packages/flutter/lib/src/scheduler/binding.dart +++ b/packages/flutter/lib/src/scheduler/binding.dart @@ -727,14 +727,15 @@ mixin SchedulerBinding on BindingBase { /// Schedule a callback for the end of this frame. /// - /// Does *not* request a new frame. + /// The provided callback is run immediately after a frame, just after the + /// persistent frame callbacks (which is when the main rendering pipeline has + /// been flushed). /// - /// This callback is run during a frame, just after the persistent - /// frame callbacks (which is when the main rendering pipeline has - /// been flushed). If a frame is in progress and post-frame - /// callbacks haven't been executed yet, then the registered - /// callback is still executed during the frame. Otherwise, the - /// registered callback is executed during the next frame. + /// This method does *not* request a new frame. If a frame is already in + /// progress and the execution of post-frame callbacks has not yet begun, then + /// the registered callback is executed at the end of the current frame. + /// Otherwise, the registered callback is executed after the next frame + /// (whenever that may be, if ever). /// /// The callbacks are executed in the order in which they have been /// added. diff --git a/packages/flutter/lib/src/widgets/scroll_notification_observer.dart b/packages/flutter/lib/src/widgets/scroll_notification_observer.dart index bdeca667e35..51fcf07ac11 100644 --- a/packages/flutter/lib/src/widgets/scroll_notification_observer.dart +++ b/packages/flutter/lib/src/widgets/scroll_notification_observer.dart @@ -208,16 +208,12 @@ class ScrollNotificationObserverState extends State @override Widget build(BuildContext context) { - // A ScrollMetricsNotification allows listeners to be notified for an - // initial state, as well as if the content dimensions change without - // scrolling. return NotificationListener( onNotification: (ScrollMetricsNotification notification) { - _notifyListeners(_ConvertedScrollMetricsNotification( - metrics: notification.metrics, - context: notification.context, - depth: notification.depth, - )); + // A ScrollMetricsNotification allows listeners to be notified for an + // initial state, as well as if the content dimensions change without + // scrolling. + _notifyListeners(notification.asScrollUpdate()); return false; }, child: NotificationListener( @@ -240,11 +236,3 @@ class ScrollNotificationObserverState extends State super.dispose(); } } - -class _ConvertedScrollMetricsNotification extends ScrollUpdateNotification { - _ConvertedScrollMetricsNotification({ - required super.metrics, - required super.context, - required super.depth, - }); -} diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index 2098364e778..ceedeff7378 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -521,10 +521,10 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { final ScrollMetrics currentMetrics = copyWith(); return _lastMetrics == null || - !(currentMetrics.extentBefore == _lastMetrics!.extentBefore - && currentMetrics.extentInside == _lastMetrics!.extentInside - && currentMetrics.extentAfter == _lastMetrics!.extentAfter - && currentMetrics.axisDirection == _lastMetrics!.axisDirection); + !(currentMetrics.extentBefore == _lastMetrics!.extentBefore && + currentMetrics.extentInside == _lastMetrics!.extentInside && + currentMetrics.extentAfter == _lastMetrics!.extentAfter && + currentMetrics.axisDirection == _lastMetrics!.axisDirection); } @override @@ -554,9 +554,10 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { assert(!_didChangeViewportDimensionOrReceiveCorrection, 'Use correctForNewDimensions() (and return true) to change the scroll offset during applyContentDimensions().'); if (_isMetricsChanged()) { - // It isn't safe to trigger the ScrollMetricsNotification if we are in - // the middle of rendering the frame, the developer is likely to schedule - // a new frame(build scheduled during frame is illegal). + // It is too late to send useful notifications, because the potential + // listeners have, by definition, already been built this frame. To make + // sure the notification is sent at all, we delay it until after the frame + // is complete. if (!_haveScheduledUpdateNotification) { scheduleMicrotask(didUpdateScrollMetrics); _haveScheduledUpdateNotification = true; @@ -1011,6 +1012,17 @@ class ScrollMetricsNotification extends Notification with ViewportNotificationMi /// determine the size of the viewport, for instance. final BuildContext context; + /// Convert this notification to a [ScrollNotification]. + /// + /// This allows it to be used with [ScrollNotificationPredicate]s. + ScrollUpdateNotification asScrollUpdate() { + return ScrollUpdateNotification( + metrics: metrics, + context: context, + depth: depth, + ); + } + @override void debugFillDescription(List description) { super.debugFillDescription(description); diff --git a/packages/flutter/lib/src/widgets/scrollbar.dart b/packages/flutter/lib/src/widgets/scrollbar.dart index e98ad613aeb..24ae147815b 100644 --- a/packages/flutter/lib/src/widgets/scrollbar.dart +++ b/packages/flutter/lib/src/widgets/scrollbar.dart @@ -1886,17 +1886,13 @@ class RawScrollbarState extends State with TickerProv } bool _handleScrollMetricsNotification(ScrollMetricsNotification notification) { - if (!widget.notificationPredicate(ScrollUpdateNotification( - metrics: notification.metrics, - context: notification.context, - depth: notification.depth, - ))) { + if (!widget.notificationPredicate(notification.asScrollUpdate())) { return false; } if (showScrollbar) { - if (_fadeoutAnimationController.status != AnimationStatus.forward - && _fadeoutAnimationController.status != AnimationStatus.completed) { + if (_fadeoutAnimationController.status != AnimationStatus.forward && + _fadeoutAnimationController.status != AnimationStatus.completed) { _fadeoutAnimationController.forward(); } } @@ -1916,8 +1912,8 @@ class RawScrollbarState extends State with TickerProv final ScrollMetrics metrics = notification.metrics; if (metrics.maxScrollExtent <= metrics.minScrollExtent) { // Hide the bar when the Scrollable widget has no space to scroll. - if (_fadeoutAnimationController.status != AnimationStatus.dismissed - && _fadeoutAnimationController.status != AnimationStatus.reverse) { + if (_fadeoutAnimationController.status != AnimationStatus.dismissed && + _fadeoutAnimationController.status != AnimationStatus.reverse) { _fadeoutAnimationController.reverse(); } @@ -1930,8 +1926,8 @@ class RawScrollbarState extends State with TickerProv if (notification is ScrollUpdateNotification || notification is OverscrollNotification) { // Any movements always makes the scrollbar start showing up. - if (_fadeoutAnimationController.status != AnimationStatus.forward - && _fadeoutAnimationController.status != AnimationStatus.completed) { + if (_fadeoutAnimationController.status != AnimationStatus.forward && + _fadeoutAnimationController.status != AnimationStatus.completed) { _fadeoutAnimationController.forward(); } @@ -1993,7 +1989,7 @@ class RawScrollbarState extends State with TickerProv } final Offset localOffset = _getLocalOffset(_scrollbarPainterKey, position); return scrollbarPainter.hitTestInteractive(localOffset, kind) - && !scrollbarPainter.hitTestOnlyThumbInteractive(localOffset, kind); + && !scrollbarPainter.hitTestOnlyThumbInteractive(localOffset, kind); } /// Returns true if the provided [Offset] is located over the thumb of the /// [RawScrollbar]. diff --git a/packages/flutter/test/widgets/notification_test.dart b/packages/flutter/test/widgets/notification_test.dart index f1b20d55ddd..e58d86f2f06 100644 --- a/packages/flutter/test/widgets/notification_test.dart +++ b/packages/flutter/test/widgets/notification_test.dart @@ -76,4 +76,22 @@ void main() { expect(() { MyNotification().dispatch(key.currentContext); }, isNot(throwsException)); expect(log, [MyNotification]); }); + + testWidgets('Notification basics - listener null return value', (WidgetTester tester) async { + await tester.pumpWidget(const Placeholder()); + final ScrollMetricsNotification n1 = ScrollMetricsNotification( + metrics: FixedScrollMetrics( + minScrollExtent: 1.0, + maxScrollExtent: 2.0, + pixels: 3.0, + viewportDimension: 4.0, + axisDirection: AxisDirection.down, + devicePixelRatio: 5.0, + ), + context: tester.allElements.first, + ); + expect(n1.metrics.pixels, 3.0); + final ScrollUpdateNotification n2 = n1.asScrollUpdate(); + expect(n2.metrics.pixels, 3.0); + }); } diff --git a/packages/flutter_test/lib/src/finders.dart b/packages/flutter_test/lib/src/finders.dart index 649df646f0a..6a706318524 100644 --- a/packages/flutter_test/lib/src/finders.dart +++ b/packages/flutter_test/lib/src/finders.dart @@ -489,6 +489,9 @@ abstract class Finder { /// Returns all the [Element]s that will be considered by this finder. /// + /// This is the internal API for the [Finder]. To obtain the elements from + /// a [Finder] in a test, consider [WidgetTester.elementList]. + /// /// See [collectAllElementsFrom]. @protected Iterable get allCandidates {