Unverified Commit a11da307 authored by Ian Hickson's avatar Ian Hickson Committed by GitHub

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.
parent f22dd30c
......@@ -248,6 +248,12 @@ class _ChainedEvaluation<T> extends Animatable<T> {
/// 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<T extends Object?> extends Animatable<T> {
/// Creates a tween.
///
......
......@@ -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<void> lockEvents(Future<void> 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<String, dynamic> eventData) {
developer.postEvent(eventKind, eventData);
......
......@@ -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) {
......
......@@ -727,14 +727,15 @@ mixin SchedulerBinding on BindingBase {
/// Schedule a callback for the end of this frame.
///
/// Does *not* request a new frame.
///
/// 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.
/// 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 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.
......
......@@ -208,16 +208,12 @@ class ScrollNotificationObserverState extends State<ScrollNotificationObserver>
@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<ScrollMetricsNotification>(
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<ScrollNotification>(
......@@ -240,11 +236,3 @@ class ScrollNotificationObserverState extends State<ScrollNotificationObserver>
super.dispose();
}
}
class _ConvertedScrollMetricsNotification extends ScrollUpdateNotification {
_ConvertedScrollMetricsNotification({
required super.metrics,
required super.context,
required super.depth,
});
}
......@@ -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<String> description) {
super.debugFillDescription(description);
......
......@@ -1886,17 +1886,13 @@ class RawScrollbarState<T extends RawScrollbar> extends State<T> 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<T extends RawScrollbar> extends State<T> 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<T extends RawScrollbar> extends State<T> 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<T extends RawScrollbar> extends State<T> 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].
......
......@@ -76,4 +76,22 @@ void main() {
expect(() { MyNotification().dispatch(key.currentContext); }, isNot(throwsException));
expect(log, <Type>[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);
});
}
......@@ -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<Element> get allCandidates {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment