From 5de6684b9c74d5b44b409d2d3c784f66a1bb8dc3 Mon Sep 17 00:00:00 2001 From: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Tue, 8 Aug 2023 17:53:10 -0700 Subject: [PATCH] Add more info to `OverlayState.insert` error messages (#129363) I was debugging an Overlay issue and felt I could have identified the problem faster if the existing assertions provided more information about the current state of the OverlayEntry and Overlay. --- packages/flutter/lib/src/widgets/overlay.dart | 65 ++++++++++++++---- .../flutter/test/widgets/overlay_test.dart | 66 +++++++++++++++++++ 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/packages/flutter/lib/src/widgets/overlay.dart b/packages/flutter/lib/src/widgets/overlay.dart index 04c71fab73..f62b5fddc1 100644 --- a/packages/flutter/lib/src/widgets/overlay.dart +++ b/packages/flutter/lib/src/widgets/overlay.dart @@ -222,7 +222,7 @@ class OverlayEntry implements Listenable { } @override - String toString() => '${describeIdentity(this)}(opaque: $opaque; maintainState: $maintainState)'; + String toString() => '${describeIdentity(this)}(opaque: $opaque; maintainState: $maintainState)${_disposedByOwner ? "(DISPOSED)" : ""}'; } class _OverlayEntryWidget extends StatefulWidget { @@ -296,7 +296,7 @@ class _OverlayEntryWidgetState extends State<_OverlayEntryWidget> { late final Iterable _hitTestOrderIterable = _createChildIterable(reversed: true); // The following uses sync* because hit-testing is lazy, and LinkedList as a - // Iterable doesn't support current modification. + // Iterable doesn't support concurrent modification. Iterable _createChildIterable({ required bool reversed }) sync* { final LinkedList<_OverlayEntryLocation>? children = _sortedTheaterSiblings; if (children == null || children.isEmpty) { @@ -543,6 +543,55 @@ class OverlayState extends State with TickerProviderStateMixin { return _entries.length; } + bool _debugCanInsertEntry(OverlayEntry entry) { + final List operandsInformation = [ + DiagnosticsProperty('The OverlayEntry was', entry, style: DiagnosticsTreeStyle.errorProperty), + DiagnosticsProperty( + 'The Overlay the OverlayEntry was trying to insert to was', this, style: DiagnosticsTreeStyle.errorProperty, + ), + ]; + + if (!mounted) { + throw FlutterError.fromParts([ + ErrorSummary('Attempted to insert an OverlayEntry to an already disposed Overlay.'), + ...operandsInformation, + ]); + } + + final OverlayState? currentOverlay = entry._overlay; + final bool alreadyContainsEntry = _entries.contains(entry); + + if (alreadyContainsEntry) { + final bool inconsistentOverlayState = !identical(currentOverlay, this); + throw FlutterError.fromParts([ + ErrorSummary('The specified entry is already present in the target Overlay.'), + ...operandsInformation, + if (inconsistentOverlayState) ErrorHint('This could be an error in the Flutter framework.') + else ErrorHint( + 'Consider calling remove on the OverlayEntry before inserting it to a different Overlay, ' + 'or switching to the OverlayPortal API to avoid manual OverlayEntry management.' + ), + if (inconsistentOverlayState) DiagnosticsProperty( + "The OverlayEntry's current Overlay was", currentOverlay, style: DiagnosticsTreeStyle.errorProperty, + ), + ]); + } + + if (currentOverlay == null) { + return true; + } + + throw FlutterError.fromParts([ + ErrorSummary('The specified entry is already present in a different Overlay.'), + ...operandsInformation, + DiagnosticsProperty("The OverlayEntry's current Overlay was", currentOverlay, style: DiagnosticsTreeStyle.errorProperty,), + ErrorHint( + 'Consider calling remove on the OverlayEntry before inserting it to a different Overlay, ' + 'or switching to the OverlayPortal API to avoid manual OverlayEntry management.' + ) + ]); + } + /// Insert the given entry into the overlay. /// /// If `below` is non-null, the entry is inserted just below `below`. @@ -552,8 +601,7 @@ class OverlayState extends State with TickerProviderStateMixin { /// It is an error to specify both `above` and `below`. void insert(OverlayEntry entry, { OverlayEntry? below, OverlayEntry? above }) { assert(_debugVerifyInsertPosition(above, below)); - assert(!_entries.contains(entry), 'The specified entry is already present in the Overlay.'); - assert(entry._overlay == null, 'The specified entry is already present in another Overlay.'); + assert(_debugCanInsertEntry(entry)); entry._overlay = this; setState(() { _entries.insert(_insertionIndex(below, above), entry); @@ -569,14 +617,7 @@ class OverlayState extends State with TickerProviderStateMixin { /// It is an error to specify both `above` and `below`. void insertAll(Iterable entries, { OverlayEntry? below, OverlayEntry? above }) { assert(_debugVerifyInsertPosition(above, below)); - assert( - entries.every((OverlayEntry entry) => !_entries.contains(entry)), - 'One or more of the specified entries are already present in the Overlay.', - ); - assert( - entries.every((OverlayEntry entry) => entry._overlay == null), - 'One or more of the specified entries are already present in another Overlay.', - ); + assert(entries.every(_debugCanInsertEntry)); if (entries.isEmpty) { return; } diff --git a/packages/flutter/test/widgets/overlay_test.dart b/packages/flutter/test/widgets/overlay_test.dart index 0914b06dff..66663e061a 100644 --- a/packages/flutter/test/widgets/overlay_test.dart +++ b/packages/flutter/test/widgets/overlay_test.dart @@ -1139,6 +1139,72 @@ void main() { ); }); + testWidgets('OverlayEntry throws if inserted to an invalid Overlay', (WidgetTester tester) async { + await tester.pumpWidget( + const Directionality( + textDirection: TextDirection.ltr, + child: Overlay(), + ), + ); + final OverlayState overlay = tester.state(find.byType(Overlay)); + final OverlayEntry entry = OverlayEntry(builder: (BuildContext context) => const SizedBox()); + expect( + () => overlay.insert(entry), + returnsNormally, + ); + + // Throws when inserted to the same Overlay. + expect( + () => overlay.insert(entry), + throwsA(isA().having( + (FlutterError error) => error.toString(), + 'toString()', + allOf( + contains('The specified entry is already present in the target Overlay.'), + contains('The OverlayEntry was'), + contains('The Overlay the OverlayEntry was trying to insert to was'), + ), + )), + ); + + await tester.pumpWidget( + const Directionality( + textDirection: TextDirection.ltr, + child: SizedBox(child: Overlay()), + ), + ); + + // Throws if inserted to an already disposed Overlay. + expect( + () => overlay.insert(entry), + throwsA(isA().having( + (FlutterError error) => error.toString(), + 'toString()', + allOf( + contains('Attempted to insert an OverlayEntry to an already disposed Overlay.'), + contains('The OverlayEntry was'), + contains('The Overlay the OverlayEntry was trying to insert to was'), + ), + )), + ); + + final OverlayState newOverlay = tester.state(find.byType(Overlay)); + // Throws when inserted to a different Overlay without calling remove. + expect( + () => newOverlay.insert(entry), + throwsA(isA().having( + (FlutterError error) => error.toString(), + 'toString()', + allOf( + contains('The specified entry is already present in a different Overlay.'), + contains('The OverlayEntry was'), + contains('The Overlay the OverlayEntry was trying to insert to was'), + contains("The OverlayEntry's current Overlay was"), + ), + )), + ); + }); + group('OverlayEntry listenable', () { final GlobalKey overlayKey = GlobalKey(); final Widget emptyOverlay = Directionality(