From fcc0c594cb0d0418d7c8752a9fa141f521c2c153 Mon Sep 17 00:00:00 2001 From: khanak0509 Date: Wed, 29 Jul 2026 01:58:31 +0530 Subject: [PATCH] Move Inspector error tracking out of ErrorBadgeManager --- packages/devtools_app/lib/devtools_app.dart | 1 + .../inspector/inspector_controller.dart | 34 ++-- .../screens/inspector/inspector_errors.dart | 15 ++ .../inspector/inspector_screen_body.dart | 7 +- .../inspector_screen_controller.dart | 154 +++++++++++++++- .../inspector/inspector_tree_controller.dart | 1 + .../shared/framework/screen_controllers.dart | 8 + .../shared/managers/error_badge_manager.dart | 168 +++--------------- .../inspector_screen_controller_test.dart | 75 ++++++++ .../managers/error_badge_manager_test.dart | 74 ++++---- .../lib/src/mocks/fake_service_manager.dart | 4 - 11 files changed, 349 insertions(+), 192 deletions(-) create mode 100644 packages/devtools_app/lib/src/screens/inspector/inspector_errors.dart create mode 100644 packages/devtools_app/test/screens/inspector/inspector_screen_controller_test.dart diff --git a/packages/devtools_app/lib/devtools_app.dart b/packages/devtools_app/lib/devtools_app.dart index fb2637e7665..80880547987 100644 --- a/packages/devtools_app/lib/devtools_app.dart +++ b/packages/devtools_app/lib/devtools_app.dart @@ -27,6 +27,7 @@ export 'src/screens/deep_link_validation/deep_links_screen.dart'; export 'src/screens/dtd/dtd_tools_controller.dart'; export 'src/screens/dtd/dtd_tools_screen.dart'; export 'src/screens/inspector/inspector_controller.dart'; +export 'src/screens/inspector/inspector_errors.dart'; export 'src/screens/inspector/inspector_screen.dart'; export 'src/screens/inspector/inspector_screen_body.dart'; export 'src/screens/inspector/inspector_screen_controller.dart'; diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart index 9484ecb422e..16ca250e83c 100644 --- a/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_controller.dart @@ -31,6 +31,7 @@ import '../../shared/console/primitives/simple_items.dart'; import '../../shared/diagnostics/diagnostics_node.dart'; import '../../shared/diagnostics/inspector_service.dart'; import '../../shared/diagnostics/primitives/instance_ref.dart'; +import '../../shared/framework/screen_controllers.dart'; import '../../shared/globals.dart'; import '../../shared/managers/notifications.dart'; import '../../shared/primitives/query_parameters.dart'; @@ -38,6 +39,7 @@ import '../../shared/primitives/utils.dart'; import '../../shared/utils/utils.dart'; import 'inspector_data_models.dart'; import 'inspector_screen.dart'; +import 'inspector_screen_controller.dart'; import 'inspector_tree_controller.dart'; final _log = Logger('inspector_controller'); @@ -149,6 +151,21 @@ class InspectorController extends DisposableController } } + /// Returns the [InspectorScreenController] when it is registered. + /// + /// [InspectorController] is sometimes constructed in unit tests without an + /// [InspectorScreenController] registered, so callers that only need to clear + /// errors on connect/reload should use this nullable accessor. + InspectorScreenController? get _inspectorScreenControllerOrNull { + final controllers = globals[ScreenControllers] as ScreenControllers?; + if (controllers == null) return null; + if (!controllers.isRegistered()) return null; + return controllers.lookup(); + } + + InspectorScreenController get _inspectorScreenController => + screenControllers.lookup(); + void _handleConnectionStart() { // Clear any existing badge/errors for older errors that were collected. // Do this in a post frame callback so that we are not trying to clear the @@ -157,7 +174,7 @@ class InspectorController extends DisposableController // TODO(kenz): When this method is called outside createState(), this post // frame callback can be removed. WidgetsBinding.instance.addPostFrameCallback((_) { - serviceConnection.errorBadgeManager.clearErrors(InspectorScreen.id); + _inspectorScreenControllerOrNull?.clearErrors(); }); } @@ -467,7 +484,7 @@ class InspectorController extends DisposableController } if (event.kind == EventKind.kIsolateReload) { - serviceConnection.errorBadgeManager.clearErrors(InspectorScreen.id); + _inspectorScreenControllerOrNull?.clearErrors(); _receivedIsolateReloadEvent = true; } } @@ -952,9 +969,7 @@ class InspectorController extends DisposableController void _updateSelectedErrorFromNode(InspectorTreeNode? node) { final inspectorRef = node?.diagnostic?.valueRef.id; - final errors = serviceConnection.errorBadgeManager - .erroredItemsForPage(InspectorScreen.id) - .value; + final errors = _inspectorScreenController.inspectorErrors.value; // Check whether the node that was just selected has any errors associated // with it. @@ -970,10 +985,7 @@ class InspectorController extends DisposableController if (errorIndex != null) { // Marking an error as read will automatically update the badge count to // reflect the remaining unread errors. - serviceConnection.errorBadgeManager.markErrorAsRead( - InspectorScreen.id, - errors[inspectorRef!]!, - ); + _inspectorScreenController.markErrorAsRead(errors[inspectorRef!]!); } } @@ -981,9 +993,7 @@ class InspectorController extends DisposableController void selectErrorByIndex(int index) { _selectedErrorIndex.value = index; - final errors = serviceConnection.errorBadgeManager - .erroredItemsForPage(InspectorScreen.id) - .value; + final errors = _inspectorScreenController.inspectorErrors.value; unawaited( updateSelectionFromService(inspectorRef: errors.keys.elementAt(index)), diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_errors.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_errors.dart new file mode 100644 index 00000000000..eb12e8e4475 --- /dev/null +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_errors.dart @@ -0,0 +1,15 @@ +// Copyright 2024 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. + +import '../../shared/managers/error_badge_manager.dart'; + +/// An error associated with a specific widget that can be inspected in the +/// Inspector screen. +class InspectableWidgetError extends DevToolsError { + InspectableWidgetError(super.errorMessage, super.id, {super.read}); + + @override + InspectableWidgetError asRead() => + InspectableWidgetError(errorMessage, id, read: true); +} diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_screen_body.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_screen_body.dart index f61c22f8106..67403c57e13 100644 --- a/packages/devtools_app/lib/src/screens/inspector/inspector_screen_body.dart +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_screen_body.dart @@ -21,7 +21,9 @@ import '../../shared/ui/search.dart'; import '../../shared/utils/utils.dart'; import 'inspector_controller.dart'; import 'inspector_controls.dart'; +import 'inspector_errors.dart'; import 'inspector_screen.dart'; +import 'inspector_screen_controller.dart'; import 'inspector_tree_controller.dart'; import 'widget_details.dart'; @@ -149,8 +151,9 @@ class InspectorScreenBodyState extends State ), Expanded( child: ValueListenableBuilder( - valueListenable: serviceConnection.errorBadgeManager - .erroredItemsForPage(InspectorScreen.id), + valueListenable: screenControllers + .lookup() + .inspectorErrors, builder: (_, LinkedHashMap errors, _) { final inspectableErrors = errors.map( diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_screen_controller.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_screen_controller.dart index 5a403dbad44..4d33da8b801 100644 --- a/packages/devtools_app/lib/src/screens/inspector/inspector_screen_controller.dart +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_screen_controller.dart @@ -2,11 +2,26 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. +import 'dart:collection'; + +import 'package:collection/collection.dart' show IterableExtension; +import 'package:devtools_app_shared/service.dart'; +import 'package:devtools_app_shared/utils.dart'; +import 'package:flutter/foundation.dart'; +import 'package:vm_service/vm_service.dart'; + +import '../../service/vm_service_wrapper.dart'; import '../../shared/analytics/metrics.dart'; import '../../shared/console/primitives/simple_items.dart'; +import '../../shared/diagnostics/diagnostics_node.dart'; import '../../shared/framework/screen.dart'; import '../../shared/framework/screen_controllers.dart'; +import '../../shared/globals.dart'; +import '../../shared/managers/error_badge_manager.dart'; +import '../../shared/primitives/query_parameters.dart'; import 'inspector_controller.dart'; +import 'inspector_errors.dart'; +import 'inspector_screen.dart'; import 'inspector_tree_controller.dart'; /// Screen controller for the Inspector screen. @@ -19,16 +34,36 @@ import 'inspector_tree_controller.dart'; /// `init` method is called lazily upon the first controller access from /// `screenControllers`. The `dispose` method is called by `screenControllers` /// when DevTools is destroying a set of DevTools screen controllers. -class InspectorScreenController extends DevToolsScreenController { +class InspectorScreenController extends DevToolsScreenController + with AutoDisposeControllerMixin { @override final screenId = ScreenMetaData.inspector.id; late InspectorController inspectorController; late InspectorTreeController inspectorTreeController; + /// Stores the inspector-specific errors keyed by inspector reference ID. + final _activeInspectorErrors = + ValueNotifier>( + LinkedHashMap(), + ); + + /// The errors currently tracked for the inspector screen. + ValueListenable> get inspectorErrors => + _activeInspectorErrors; + + /// The count of unread inspector errors (used for the badge). + ValueListenable get inspectorErrorCount => serviceConnection + .errorBadgeManager + .errorCountNotifier(InspectorScreen.id); + @override void init() { super.init(); + // Inspector owns unread state for its badge; scaffold tab switches must not + // clear it. See https://github.com/flutter/devtools/pull/9805. + serviceConnection.errorBadgeManager.manageErrorCount(InspectorScreen.id); + inspectorTreeController = InspectorTreeController( gaId: InspectorScreenMetrics.summaryTreeGaId, ); @@ -36,10 +71,127 @@ class InspectorScreenController extends DevToolsScreenController { inspectorTree: inspectorTreeController, treeType: FlutterTreeType.widget, ); + + // Listen for Flutter extension events to extract inspector-specific errors. + // Match other screen controllers: attach now if connected, and on connect. + addAutoDisposeListener(serviceConnection.serviceManager.connectedState, () { + if (serviceConnection.serviceManager.connectedState.value.connected) { + _handleConnectionStart(serviceConnection.serviceManager.service!); + } + }); + if (serviceConnection.serviceManager.connectedAppInitialized) { + _handleConnectionStart(serviceConnection.serviceManager.service!); + } + } + + void _handleConnectionStart(VmServiceWrapper service) { + autoDisposeStreamSubscription( + service.onExtensionEventWithHistorySafe.listen(_handleExtensionEvent), + ); + } + + void _handleExtensionEvent(Event e) { + if (e.extensionKind == FlutterEvent.error) { + final inspectableError = _extractInspectableError(e); + if (inspectableError != null) { + appendError(inspectableError); + } + } + } + + InspectableWidgetError? _extractInspectableError(Event error) { + final extensionData = error.extensionData; + if (extensionData == null) return null; + + final node = RemoteDiagnosticsNode(extensionData.data, null, false, null); + + final errorSummaryNode = node.inlineProperties.firstWhereOrNull( + (p) => p.type == 'ErrorSummary', + ); + final errorMessage = errorSummaryNode?.description; + if (errorMessage == null) { + return null; + } + + final devToolsUrlNode = node.inlineProperties.firstWhereOrNull( + (p) => + p.type == 'DevToolsDeepLinkProperty' && + p.getStringMember('value') != null, + ); + if (devToolsUrlNode == null) { + return null; + } + + final queryParams = DevToolsQueryParams.fromUrl( + devToolsUrlNode.getStringMember('value')!, + ); + final inspectorRef = queryParams.inspectorRef ?? ''; + + return InspectableWidgetError(errorMessage, inspectorRef); + } + + /// Appends an error to the inspector's active errors and updates the badge + /// count. + void appendError(DevToolsError error) { + final errors = _activeInspectorErrors; + final previousError = errors.value[error.id]; + + // Build a new map with the new error. Adding to the existing map + // won't cause the ValueNotifier to fire (and it's not permitted to call + // notifyListeners() directly). + final newValue = LinkedHashMap.of(errors.value); + newValue[error.id] = error; + errors.value = newValue; + + if (previousError == null) { + if (!error.read) { + _incrementUnreadCount(); + } + return; + } + + if (previousError.read && !error.read) { + _incrementUnreadCount(); + } else if (!previousError.read && error.read) { + _decrementUnreadCount(); + } + } + + /// Clears all inspector errors and resets the badge count. + void clearErrors() { + _activeInspectorErrors.value = LinkedHashMap(); + serviceConnection.errorBadgeManager.resetErrorCount(InspectorScreen.id); + } + + /// Marks an error as read and decrements the unread count. + void markErrorAsRead(DevToolsError error) { + final errors = _activeInspectorErrors; + + // If this error doesn't exist anymore or is already read, nothing to do. + final currentError = errors.value[error.id]; + if (currentError == null || currentError.read) { + return; + } + + // Otherwise, replace the map with a new one that has the error marked + // as read. + final newValue = LinkedHashMap.of(errors.value); + newValue[error.id] = currentError.asRead(); + errors.value = newValue; + _decrementUnreadCount(); + } + + void _incrementUnreadCount() { + serviceConnection.errorBadgeManager.incrementBadgeCount(InspectorScreen.id); + } + + void _decrementUnreadCount() { + serviceConnection.errorBadgeManager.decrementBadgeCount(InspectorScreen.id); } @override void dispose() { + _activeInspectorErrors.dispose(); inspectorTreeController.dispose(); inspectorController.dispose(); super.dispose(); diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart index 130bec6a5a2..1d62eac7062 100644 --- a/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart @@ -30,6 +30,7 @@ import '../../shared/ui/search.dart'; import '../../shared/ui/utils.dart'; import '../../shared/utils/utils.dart'; import 'inspector_controller.dart'; +import 'inspector_errors.dart'; final _log = Logger('inspector_tree_controller'); diff --git a/packages/devtools_app/lib/src/shared/framework/screen_controllers.dart b/packages/devtools_app/lib/src/shared/framework/screen_controllers.dart index 427254c8bd5..7baade8416e 100644 --- a/packages/devtools_app/lib/src/shared/framework/screen_controllers.dart +++ b/packages/devtools_app/lib/src/shared/framework/screen_controllers.dart @@ -51,6 +51,14 @@ class ScreenControllers { controllers[T] = _LazyController(creator: controllerCreator); } + /// Whether a controller of type [T] has been registered for the active mode. + bool isRegistered() { + final controllers = offlineDataController.showingOfflineData.value + ? offlineControllers + : this.controllers; + return controllers.containsKey(T); + } + /// Returns the active screen controller of type [T]. /// /// When DevTools is showing offline data, the offline screen controller will diff --git a/packages/devtools_app/lib/src/shared/managers/error_badge_manager.dart b/packages/devtools_app/lib/src/shared/managers/error_badge_manager.dart index 10a12fb2d54..2689cd48c81 100644 --- a/packages/devtools_app/lib/src/shared/managers/error_badge_manager.dart +++ b/packages/devtools_app/lib/src/shared/managers/error_badge_manager.dart @@ -3,9 +3,7 @@ // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. import 'dart:async'; -import 'dart:collection'; -import 'package:collection/collection.dart' show IterableExtension; import 'package:devtools_app_shared/service.dart'; import 'package:devtools_app_shared/utils.dart'; import 'package:flutter/foundation.dart'; @@ -17,26 +15,29 @@ import '../../screens/network/network_screen.dart'; import '../../screens/performance/performance_screen.dart'; import '../../service/service_extensions.dart' as extensions; import '../../service/vm_service_wrapper.dart'; -import '../diagnostics/diagnostics_node.dart'; import '../globals.dart'; import '../primitives/listenable.dart'; -import '../primitives/query_parameters.dart'; +/// Manages error badge counts for DevTools screen tabs. +/// +/// This is a generic counter that tracks unread error counts per screen. +/// Screen-specific error tracking logic (e.g., detailed error objects for the +/// Inspector screen) should live in the respective screen controllers. class ErrorBadgeManager extends DisposableController with AutoDisposeControllerMixin { - // TODO(https://github.com/flutter/devtools/issues/9105): Separate out - // Inspector-specific logic from this file. final _activeErrorCounts = >{ InspectorScreen.id: ValueNotifier(0), PerformanceScreen.id: ValueNotifier(0), NetworkScreen.id: ValueNotifier(0), }; - final _activeErrors = - >>{ - InspectorScreen.id: ValueNotifier>( - LinkedHashMap(), - ), - }; + + /// Screen ids whose unread badge count is owned by the screen controller. + /// + /// For these screens, [clearErrorCount] is a no-op so navigating to the tab + /// (see scaffold) does not drop the unread badge. Controllers should update + /// the count via [incrementBadgeCount], [decrementBadgeCount], and + /// [resetErrorCount] instead. + final _managedErrorCounts = {}; void vmServiceOpened(VmServiceWrapper service) { // Ensure structured errors are enabled. @@ -63,100 +64,29 @@ class ErrorBadgeManager extends DisposableController void _handleExtensionEvent(Event e) { if (e.extensionKind == FlutterEvent.error) { incrementBadgeCount(LoggingScreen.id); - - final inspectableError = _extractInspectableError(e); - if (inspectableError != null) { - appendError(InspectorScreen.id, inspectableError); - } - } - } - - InspectableWidgetError? _extractInspectableError(Event error) { - // TODO(https://github.com/flutter/devtools/issues/9105): Switch to using - // the inspectorService from the serviceManager once Jacob's change to add - // it lands. - final node = RemoteDiagnosticsNode( - error.extensionData!.data, - null, - false, - null, - ); - - final errorSummaryNode = node.inlineProperties.firstWhereOrNull( - (p) => p.type == 'ErrorSummary', - ); - final errorMessage = errorSummaryNode?.description; - if (errorMessage == null) { - return null; - } - - final devToolsUrlNode = node.inlineProperties.firstWhereOrNull( - (p) => - p.type == 'DevToolsDeepLinkProperty' && - p.getStringMember('value') != null, - ); - if (devToolsUrlNode == null) { - return null; } - - final queryParams = DevToolsQueryParams.fromUrl( - devToolsUrlNode.getStringMember('value')!, - ); - final inspectorRef = queryParams.inspectorRef ?? ''; - - return InspectableWidgetError(errorMessage, inspectorRef); } void _handleStdErr(Event _) { incrementBadgeCount(LoggingScreen.id); } - void incrementBadgeCount(String screenId) { - if (_activeErrors.containsKey(screenId)) { - return; - } - - final notifier = _errorCountNotifier(screenId); - if (notifier == null) return; - - final currentCount = notifier.value; - notifier.value = currentCount + 1; - } - - void appendError(String screenId, DevToolsError error) { - final errors = _activeErrors[screenId]; - if (errors == null) return; - - final previousError = errors.value[error.id]; - - // Build a new map with the new error. Adding to the existing map - // won't cause the ValueNotifier to fire (and it's not permitted to call - // notifyListeners() directly). - final newValue = LinkedHashMap.of(errors.value); - newValue[error.id] = error; - errors.value = newValue; - - if (previousError == null) { - if (!error.read) { - _incrementUnreadCount(screenId); - } - return; - } - - if (previousError.read && !error.read) { - _incrementUnreadCount(screenId); - } else if (!previousError.read && error.read) { - _decrementUnreadCount(screenId); - } + /// Marks [screenId] as managing its own unread badge count. + /// + /// After this is called, [clearErrorCount] will not reset the badge for + /// [screenId] (preserving unread state across tab switches). + void manageErrorCount(String screenId) { + _managedErrorCounts.add(screenId); } - void _incrementUnreadCount(String screenId) { + void incrementBadgeCount(String screenId) { final notifier = _errorCountNotifier(screenId); if (notifier == null) return; + notifier.value = notifier.value + 1; } - void _decrementUnreadCount(String screenId) { + void decrementBadgeCount(String screenId) { final notifier = _errorCountNotifier(screenId); if (notifier == null) return; if (notifier.value == 0) return; @@ -167,55 +97,23 @@ class ErrorBadgeManager extends DisposableController return _errorCountNotifier(screenId) ?? const FixedValueListenable(0); } - ValueListenable> erroredItemsForPage( - String screenId, - ) { - return _activeErrors[screenId] ?? - FixedValueListenable>( - LinkedHashMap(), - ); - } - ValueNotifier? _errorCountNotifier(String screenId) { return _activeErrorCounts[screenId]; } + /// Clears the badge count for [screenId], unless it is [manageErrorCount]d. void clearErrorCount(String screenId) { - if (_activeErrors.containsKey(screenId)) { - return; - } + if (_managedErrorCounts.contains(screenId)) return; _activeErrorCounts[screenId]?.value = 0; } - void clearErrors(String screenId) { - if (!_activeErrors.containsKey(screenId)) { - clearErrorCount(screenId); - return; - } - - _activeErrors[screenId]?.value = LinkedHashMap(); + /// Unconditionally resets the badge count for [screenId]. + /// + /// Use from screen controllers that call [manageErrorCount] when they need + /// to clear their own unread state (e.g. on hot restart). + void resetErrorCount(String screenId) { _activeErrorCounts[screenId]?.value = 0; } - - void markErrorAsRead(String screenId, DevToolsError error) { - final errors = _activeErrors[screenId]; - if (errors == null) return; - - // If this error doesn't exist anymore or is already read, nothing to do. - if (errors.value[error.id]?.read ?? true) { - return; - } - - // Otherwise, replace the map with a new one that has the error marked - // as read. - errors.value = LinkedHashMap.fromEntries( - errors.value.entries.map((e) { - if (e.value != error) return e; - return MapEntry(e.key, e.value.asRead()); - }), - ); - _decrementUnreadCount(screenId); - } } class DevToolsError { @@ -227,11 +125,3 @@ class DevToolsError { DevToolsError asRead() => DevToolsError(errorMessage, id, read: true); } - -class InspectableWidgetError extends DevToolsError { - InspectableWidgetError(super.errorMessage, super.id, {super.read}); - - @override - InspectableWidgetError asRead() => - InspectableWidgetError(errorMessage, id, read: true); -} diff --git a/packages/devtools_app/test/screens/inspector/inspector_screen_controller_test.dart b/packages/devtools_app/test/screens/inspector/inspector_screen_controller_test.dart new file mode 100644 index 00000000000..02fb69dda21 --- /dev/null +++ b/packages/devtools_app/test/screens/inspector/inspector_screen_controller_test.dart @@ -0,0 +1,75 @@ +// Copyright 2025 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. + +import 'package:devtools_app/devtools_app.dart'; +import 'package:devtools_app_shared/utils.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + late ServiceConnectionManager serviceConnectionManager; + late InspectorScreenController controller; + + setUp(() { + serviceConnectionManager = ServiceConnectionManager(); + setGlobal(ServiceConnectionManager, serviceConnectionManager); + controller = InspectorScreenController(); + // Mirror the badge-ownership registration performed in [init] without + // constructing the full Inspector tree controllers. + serviceConnection.errorBadgeManager.manageErrorCount(InspectorScreen.id); + }); + + int unreadBadgeCount() => serviceConnection.errorBadgeManager + .errorCountNotifier(InspectorScreen.id) + .value; + + test('appendError stores the error and increments the badge', () { + controller.appendError(InspectableWidgetError('Overflow', 'ref-1')); + + expect(controller.inspectorErrors.value.length, equals(1)); + expect(controller.inspectorErrors.value['ref-1']!.errorMessage, 'Overflow'); + expect(unreadBadgeCount(), equals(1)); + }); + + test('appendError does not double-count the same error id', () { + controller.appendError(InspectableWidgetError('Overflow', 'ref-1')); + controller.appendError(InspectableWidgetError('Overflow again', 'ref-1')); + + expect(controller.inspectorErrors.value.length, equals(1)); + expect(unreadBadgeCount(), equals(1)); + }); + + test('markErrorAsRead decrements the badge', () { + final error = InspectableWidgetError('Overflow', 'ref-1'); + controller.appendError(error); + controller.markErrorAsRead(error); + + expect(controller.inspectorErrors.value['ref-1']!.read, isTrue); + expect(unreadBadgeCount(), equals(0)); + }); + + test('clearErrors resets errors and badge', () { + controller.appendError(InspectableWidgetError('Overflow', 'ref-1')); + controller.appendError(InspectableWidgetError('Null check', 'ref-2')); + expect(unreadBadgeCount(), equals(2)); + + controller.clearErrors(); + + expect(controller.inspectorErrors.value, isEmpty); + expect(unreadBadgeCount(), equals(0)); + }); + + test( + 'scaffold-style clearErrorCount does not drop inspector unread badge', + () { + controller.appendError(InspectableWidgetError('Overflow', 'ref-1')); + expect(unreadBadgeCount(), equals(1)); + + // Simulates DevToolsScaffold clearing the badge on tab navigation. + serviceConnection.errorBadgeManager.clearErrorCount(InspectorScreen.id); + + expect(unreadBadgeCount(), equals(1)); + expect(controller.inspectorErrors.value.length, equals(1)); + }, + ); +} diff --git a/packages/devtools_app/test/shared/managers/error_badge_manager_test.dart b/packages/devtools_app/test/shared/managers/error_badge_manager_test.dart index e3c12b6a3b8..bbd3d15cd83 100644 --- a/packages/devtools_app/test/shared/managers/error_badge_manager_test.dart +++ b/packages/devtools_app/test/shared/managers/error_badge_manager_test.dart @@ -13,7 +13,11 @@ import 'package:devtools_app/src/screens/profiler/profiler_screen.dart'; import 'package:devtools_app/src/shared/managers/error_badge_manager.dart'; import 'package:flutter_test/flutter_test.dart'; -final screensWithCountOnly = [PerformanceScreen.id, NetworkScreen.id]; +final screensWithBadgeCounts = [ + InspectorScreen.id, + PerformanceScreen.id, + NetworkScreen.id, +]; final allScreenIds = [ InspectorScreen.id, @@ -30,9 +34,6 @@ void main() { late ErrorBadgeManager errorBadgeManager; group('ErrorBadgeManager', () { - int getActiveErrorCount(String screenId) => - errorBadgeManager.erroredItemsForPage(screenId).value.entries.length; - setUp(() { errorBadgeManager = ErrorBadgeManager(); }); @@ -51,7 +52,7 @@ void main() { allScreenIds.forEach(errorBadgeManager.incrementBadgeCount); for (final id in allScreenIds) { - if (screensWithCountOnly.contains(id)) { + if (screensWithBadgeCounts.contains(id)) { expect(errorBadgeManager.errorCountNotifier(id).value, equals(1)); } else { expect(errorBadgeManager.errorCountNotifier(id).value, equals(0)); @@ -59,11 +60,36 @@ void main() { } }); + test('decrementBadgeCount decrements supported tabs', () { + // First increment + allScreenIds.forEach(errorBadgeManager.incrementBadgeCount); + + for (final id in screensWithBadgeCounts) { + expect(errorBadgeManager.errorCountNotifier(id).value, equals(1)); + } + + // Then decrement + allScreenIds.forEach(errorBadgeManager.decrementBadgeCount); + + for (final id in screensWithBadgeCounts) { + expect(errorBadgeManager.errorCountNotifier(id).value, equals(0)); + } + }); + + test('decrementBadgeCount does not go below zero', () { + // Decrement without any prior increment + errorBadgeManager.decrementBadgeCount(InspectorScreen.id); + expect( + errorBadgeManager.errorCountNotifier(InspectorScreen.id).value, + equals(0), + ); + }); + test('clearErrorCount resets counts', () { allScreenIds.forEach(errorBadgeManager.incrementBadgeCount); for (final id in allScreenIds) { - if (screensWithCountOnly.contains(id)) { + if (screensWithBadgeCounts.contains(id)) { expect(errorBadgeManager.errorCountNotifier(id).value, equals(1)); } else { expect(errorBadgeManager.errorCountNotifier(id).value, equals(0)); @@ -77,43 +103,23 @@ void main() { } }); - // TODO(https://github.com/flutter/devtools/issues/9105): This logic should - // be moved to the inspector. - test('appendError works for inspector screen only', () { - for (final id in allScreenIds) { - errorBadgeManager.appendError(id, DevToolsError('An error', id)); - } - - for (final id in allScreenIds) { - if (id == InspectorScreen.id) { - expect(getActiveErrorCount(id), equals(1)); - } else { - expect(getActiveErrorCount(id), equals(0)); - } - } - }); - - test('clearErrors resets counts and removes errors', () { - expect(getActiveErrorCount(InspectorScreen.id), equals(0)); + test('clearErrorCount is a no-op for manageErrorCount screens', () { + errorBadgeManager.manageErrorCount(InspectorScreen.id); + errorBadgeManager.incrementBadgeCount(InspectorScreen.id); expect( errorBadgeManager.errorCountNotifier(InspectorScreen.id).value, - equals(0), - ); - - errorBadgeManager.appendError( - InspectorScreen.id, - DevToolsError('An error', InspectorScreen.id), + equals(1), ); - expect(getActiveErrorCount(InspectorScreen.id), equals(1)); + // Simulates scaffold clearing the badge on tab navigation. + errorBadgeManager.clearErrorCount(InspectorScreen.id); expect( errorBadgeManager.errorCountNotifier(InspectorScreen.id).value, equals(1), ); - errorBadgeManager.clearErrors(InspectorScreen.id); - - expect(getActiveErrorCount(InspectorScreen.id), equals(0)); + // Controllers that own unread state can still reset explicitly. + errorBadgeManager.resetErrorCount(InspectorScreen.id); expect( errorBadgeManager.errorCountNotifier(InspectorScreen.id).value, equals(0), diff --git a/packages/devtools_test/lib/src/mocks/fake_service_manager.dart b/packages/devtools_test/lib/src/mocks/fake_service_manager.dart index 1b4027fedea..5b948b568dd 100644 --- a/packages/devtools_test/lib/src/mocks/fake_service_manager.dart +++ b/packages/devtools_test/lib/src/mocks/fake_service_manager.dart @@ -3,7 +3,6 @@ // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. import 'dart:async'; -import 'dart:collection'; import 'package:devtools_app/devtools_app.dart'; import 'package:devtools_app_shared/service.dart'; @@ -40,9 +39,6 @@ class FakeServiceConnectionManager extends Fake ); for (final screen in ScreenMetaData.values) { final screenId = screen.id; - when(errorBadgeManager.erroredItemsForPage(screenId)).thenReturn( - FixedValueListenable(LinkedHashMap()), - ); when( errorBadgeManager.errorCountNotifier(screenId), ).thenReturn(ValueNotifier(0));