From 3ed4989d9c414179e79c2f4162a179c8d1e45b3a Mon Sep 17 00:00:00 2001 From: Kenzie Davisson <43759233+kenzieschmoll@users.noreply.github.com> Date: Tue, 21 May 2024 13:54:58 -0700 Subject: [PATCH] Store available and visible DevTools extensions in a single pattern (#7783) --- .../devtools_extensions_test.dart | 36 ++++----- packages/devtools_app/lib/src/app.dart | 14 ++-- .../lib/src/extensions/extension_service.dart | 81 ++++++++++++++----- .../src/extensions/extension_settings.dart | 3 +- .../standalone_ui/vs_code/debug_sessions.dart | 8 +- .../src/standalone_ui/vs_code/devtools.dart | 6 +- .../extensions/extension_screen_test.dart | 2 +- .../extensions/extension_service_test.dart | 10 +-- .../extensions/extension_settings_test.dart | 4 +- 9 files changed, 98 insertions(+), 66 deletions(-) diff --git a/packages/devtools_app/integration_test/test/live_connection/devtools_extensions_test.dart b/packages/devtools_app/integration_test/test/live_connection/devtools_extensions_test.dart index 3734f9b0bfa..dbba651a15d 100644 --- a/packages/devtools_app/integration_test/test/live_connection/devtools_extensions_test.dart +++ b/packages/devtools_app/integration_test/test/live_connection/devtools_extensions_test.dart @@ -43,8 +43,8 @@ void main() { logStatus( 'verify static extensions are available before connecting to an app', ); - expect(extensionService.availableExtensions.value.length, 1); - expect(extensionService.visibleExtensions.value.length, 1); + expect(extensionService.availableExtensions.length, 1); + expect(extensionService.visibleExtensions.length, 1); await _verifyExtensionsSettingsMenu( tester, [ @@ -54,8 +54,8 @@ void main() { await connectToTestApp(tester, testApp); - expect(extensionService.availableExtensions.value.length, 5); - expect(extensionService.visibleExtensions.value.length, 5); + expect(extensionService.availableExtensions.length, 5); + expect(extensionService.visibleExtensions.length, 5); await _verifyExtensionsSettingsMenu( tester, [ @@ -91,8 +91,8 @@ void main() { await _verifyContextMenuActionsAndDisable(tester); - expect(extensionService.availableExtensions.value.length, 5); - expect(extensionService.visibleExtensions.value.length, 4); + expect(extensionService.availableExtensions.length, 5); + expect(extensionService.visibleExtensions.length, 4); await _verifyExtensionTabVisibility( tester, extensionIndex: 0, @@ -117,8 +117,8 @@ void main() { ); await _answerEnableExtensionPrompt(tester, enable: false); - expect(extensionService.availableExtensions.value.length, 5); - expect(extensionService.visibleExtensions.value.length, 3); + expect(extensionService.availableExtensions.length, 5); + expect(extensionService.visibleExtensions.length, 3); await _verifyExtensionTabVisibility( tester, extensionIndex: 1, @@ -139,8 +139,8 @@ void main() { logStatus('verify we can re-enable an extension from the settings menu'); await _changeExtensionSetting(tester, extensionIndex: 1, enable: true); - expect(extensionService.availableExtensions.value.length, 5); - expect(extensionService.visibleExtensions.value.length, 4); + expect(extensionService.availableExtensions.length, 5); + expect(extensionService.visibleExtensions.length, 4); await _switchToExtensionScreen(tester, extensionIndex: 1); expect(find.byType(EnableExtensionPrompt), findsNothing); expect(find.byType(EmbeddedExtensionView), findsOneWidget); @@ -168,8 +168,8 @@ void main() { logStatus('disable the extension from the settings menu'); await _changeExtensionSetting(tester, extensionIndex: 2, enable: false); - expect(extensionService.availableExtensions.value.length, 5); - expect(extensionService.visibleExtensions.value.length, 3); + expect(extensionService.availableExtensions.length, 5); + expect(extensionService.visibleExtensions.length, 3); await _verifyExtensionTabVisibility( tester, extensionIndex: 2, @@ -193,8 +193,7 @@ Future _switchToExtensionScreen( required int extensionIndex, bool initialLoad = false, }) async { - final extensionConfig = - extensionService.availableExtensions.value[extensionIndex]; + final extensionConfig = extensionService.availableExtensions[extensionIndex]; await switchToScreen( tester, tabIcon: extensionConfig.icon, @@ -222,8 +221,7 @@ Future _verifyExtensionTabVisibility( 'verify the extension at index $extensionIndex is ' '${!visible ? 'not' : ''} visible', ); - final extensionConfig = - extensionService.availableExtensions.value[extensionIndex]; + final extensionConfig = extensionService.availableExtensions[extensionIndex]; final tabFinder = await findTab(tester, extensionConfig.icon); expect(tabFinder.evaluate(), visible ? isNotEmpty : isEmpty); } @@ -352,14 +350,14 @@ Future _verifyExtensionVisibilitySetting(WidgetTester tester) async { preferences.devToolsExtensions.showOnlyEnabledExtensions.value, isFalse, ); - expect(extensionService.visibleExtensions.value.length, 5); + expect(extensionService.visibleExtensions.length, 5); // No need to open the settings menu as it should already be open. await _toggleShowOnlyEnabledExtensions(tester); expect( preferences.devToolsExtensions.showOnlyEnabledExtensions.value, isTrue, ); - expect(extensionService.visibleExtensions.value.length, 0); + expect(extensionService.visibleExtensions.length, 0); // Return the setting to its original state. await _toggleShowOnlyEnabledExtensions(tester); @@ -367,7 +365,7 @@ Future _verifyExtensionVisibilitySetting(WidgetTester tester) async { preferences.devToolsExtensions.showOnlyEnabledExtensions.value, isFalse, ); - expect(extensionService.visibleExtensions.value.length, 5); + expect(extensionService.visibleExtensions.length, 5); await _closeExtensionSettingsMenu(tester); } diff --git a/packages/devtools_app/lib/src/app.dart b/packages/devtools_app/lib/src/app.dart index e3c2fe79558..31b9101ae41 100644 --- a/packages/devtools_app/lib/src/app.dart +++ b/packages/devtools_app/lib/src/app.dart @@ -93,7 +93,8 @@ class DevToolsAppState extends State with AutoDisposeMixin { if (FeatureFlags.devToolsExtensions) { // TODO(https://github.com/flutter/devtools/issues/6273): stop special // casing the package:provider extension. - final containsProviderExtension = extensionService.visibleExtensions.value + final containsProviderExtension = extensionService + .currentExtensions.value.visibleExtensions .where((e) => e.name == 'provider') .isNotEmpty; final devToolsScreens = containsProviderExtension @@ -110,7 +111,7 @@ class DevToolsAppState extends State with AutoDisposeMixin { widget.originalScreens.map((s) => s.screen).toList(); Iterable get _extensionScreens => - extensionService.visibleExtensions.value.map( + extensionService.visibleExtensions.map( (e) => DevToolsScreen(ExtensionScreen(e)).screen, ); @@ -152,11 +153,7 @@ class DevToolsAppState extends State with AutoDisposeMixin { if (FeatureFlags.devToolsExtensions) { addAutoDisposeListener( - extensionService.availableExtensions, - clearRoutesAndSetState, - ); - addAutoDisposeListener( - extensionService.visibleExtensions, + extensionService.currentExtensions, clearRoutesAndSetState, ); } @@ -263,8 +260,7 @@ class DevToolsAppState extends State with AutoDisposeMixin { return MultiValueListenableBuilder( listenables: [ preferences.vmDeveloperModeEnabled, - extensionService.availableExtensions, - extensionService.visibleExtensions, + extensionService.currentExtensions, ], builder: (_, __, child) { final screens = _visibleScreens() diff --git a/packages/devtools_app/lib/src/extensions/extension_service.dart b/packages/devtools_app/lib/src/extensions/extension_service.dart index 2b5811d11b4..abf56c62ff7 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service.dart @@ -17,6 +17,23 @@ final _log = Logger('ExtensionService'); // TODO(https://github.com/flutter/devtools/issues/7594): detect extensions from // globally activated pub packages. +/// Data pattern containing a [List] of available extensions and a [List] of +/// visible extensions. +typedef DevToolsExtensionsGroup = ({ + /// All the DevTools extensions, runtime and static, that are available for + /// the connected application, regardless of whether they have been enabled or + /// disabled by the user. + /// + /// This set of extensions will include one version of a DevTools extension + /// per package and will exclude any duplicates that have been marked as + /// ignored in [_maybeIgnoreExtensions]. + List availableExtensions, + + /// DevTools extensions that are visible in their own DevTools screen (i.e. + /// extensions that have not been manually disabled by the user). + List visibleExtensions, +}); + class ExtensionService extends DisposableController with AutoDisposeControllerMixin { ExtensionService({this.fixedAppRoot, this.ignoreServiceConnection = false}); @@ -36,6 +53,21 @@ class ExtensionService extends DisposableController /// [ExtensionService] will manage DevTools extensions for. Uri? _appRoot; + /// A listenable for the current set of DevTools extensions. + /// + /// The [DevToolsExtensionsGroup] contains both the List of available + /// extensions and the List of visible extensions. These values are updated + /// in tandem in the common case, so storing them as a group saves listeners + /// from having to listen to two separate notifiers. + ValueListenable get currentExtensions => + _currentExtensions; + final _currentExtensions = ValueNotifier( + ( + availableExtensions: [], + visibleExtensions: [], + ), + ); + /// All the DevTools extensions, runtime and static, that are available for /// the connected application, regardless of whether they have been enabled or /// disabled by the user. @@ -43,15 +75,13 @@ class ExtensionService extends DisposableController /// This set of extensions will include one version of a DevTools extension /// per package and will exclude any duplicates that have been marked as /// ignored in [_maybeIgnoreExtensions]. - ValueListenable> get availableExtensions => - _availableExtensions; - final _availableExtensions = ValueNotifier>([]); + List get availableExtensions => + _currentExtensions.value.availableExtensions; /// DevTools extensions that are visible in their own DevTools screen (i.e. /// extensions that have not been manually disabled by the user). - ValueListenable> get visibleExtensions => - _visibleExtensions; - final _visibleExtensions = ValueNotifier>([]); + List get visibleExtensions => + _currentExtensions.value.visibleExtensions; /// DevTools extensions available in the user's project that do not require a /// running application. @@ -132,7 +162,9 @@ class ExtensionService extends DisposableController addAutoDisposeListener( preferences.devToolsExtensions.showOnlyEnabledExtensions, () async { - await _refreshExtensionEnabledStates(); + await _refreshExtensionEnabledStates( + availableExtensions: _currentExtensions.value.availableExtensions, + ); }, ); @@ -166,11 +198,11 @@ class ExtensionService extends DisposableController staticExtensions = allExtensions.where((e) => e.detectedFromStaticContext).toList(); _maybeIgnoreExtensions(connectedToApp: _appRoot != null); - _availableExtensions.value = [ + final available = [ ...runtimeExtensions, ...staticExtensions.where((ext) => !isExtensionIgnored(ext)), ]..sort(); - await _refreshExtensionEnabledStates(); + await _refreshExtensionEnabledStates(availableExtensions: available); _refreshInProgress.value = false; } @@ -254,12 +286,14 @@ class ExtensionService extends DisposableController } } - Future _refreshExtensionEnabledStates() async { + Future _refreshExtensionEnabledStates({ + required List availableExtensions, + }) async { final onlyIncludeEnabled = preferences.devToolsExtensions.showOnlyEnabledExtensions.value; final visible = []; - for (final extension in _availableExtensions.value) { + for (final extension in availableExtensions) { final stateFromOptionsFile = await server.extensionEnabledState( devtoolsOptionsFileUri: extension.devtoolsOptionsUri, extensionName: extension.name, @@ -282,11 +316,12 @@ class ExtensionService extends DisposableController 'visible extensions after refreshing - ${visible.map((e) => e.name).toList()}', ); - // [_visibleExtensions] should be set last so that all extension states in - // [_extensionEnabledStates] are updated by the time we notify listeners of - // [visibleExtensions]. It is not necessary to sort [visible] because - // [_availableExtensions] is already sorted. - _visibleExtensions.value = visible; + // It is not necessary to sort [visible] because [availableExtensions] is + // already sorted. + _currentExtensions.value = ( + availableExtensions: availableExtensions, + visibleExtensions: visible, + ); } /// Sets the enabled state for [extension] and any currently ignored @@ -310,7 +345,9 @@ class ExtensionService extends DisposableController enable: enable, ), ]); - await _refreshExtensionEnabledStates(); + await _refreshExtensionEnabledStates( + availableExtensions: _currentExtensions.value.availableExtensions, + ); } /// Marks this extension configuration as ignored or unignored based on the @@ -339,9 +376,10 @@ class ExtensionService extends DisposableController runtimeExtensions.clear(); staticExtensions.clear(); _ignoredStaticExtensionsByHashCode.clear(); - - _availableExtensions.value = []; - _visibleExtensions.value = []; + _currentExtensions.value = ( + availableExtensions: [], + visibleExtensions: [], + ); _extensionEnabledStates.clear(); _refreshInProgress.value = false; } @@ -351,8 +389,7 @@ class ExtensionService extends DisposableController for (final notifier in _extensionEnabledStates.values) { notifier.dispose(); } - _availableExtensions.dispose(); - _visibleExtensions.dispose(); + _currentExtensions.dispose(); _refreshInProgress.dispose(); super.dispose(); } diff --git a/packages/devtools_app/lib/src/extensions/extension_settings.dart b/packages/devtools_app/lib/src/extensions/extension_settings.dart index 4e9e6b1c8fd..cd35d253c59 100644 --- a/packages/devtools_app/lib/src/extensions/extension_settings.dart +++ b/packages/devtools_app/lib/src/extensions/extension_settings.dart @@ -27,7 +27,8 @@ class ExtensionSettingsAction extends ScaffoldAction { showDialog( context: context, builder: (context) => ExtensionSettingsDialog( - extensions: extensionService.availableExtensions.value, + extensions: extensionService + .currentExtensions.value.availableExtensions, ), ), ); diff --git a/packages/devtools_app/lib/src/standalone_ui/vs_code/debug_sessions.dart b/packages/devtools_app/lib/src/standalone_ui/vs_code/debug_sessions.dart index 36977df017d..a51260cae6f 100644 --- a/packages/devtools_app/lib/src/standalone_ui/vs_code/debug_sessions.dart +++ b/packages/devtools_app/lib/src/standalone_ui/vs_code/debug_sessions.dart @@ -248,12 +248,12 @@ class _DevToolsMenuState extends State<_DevToolsMenu> { ), if (_extensionServiceForSession != null) ValueListenableBuilder( - valueListenable: _extensionServiceForSession!.visibleExtensions, - builder: (context, extensions, _) { - return extensions.isEmpty + valueListenable: _extensionServiceForSession!.currentExtensions, + builder: (context, currentExtensions, _) { + return currentExtensions.visibleExtensions.isEmpty ? const SizedBox.shrink() : ExtensionScreenMenuItem( - extensions: extensions, + extensions: currentExtensions.visibleExtensions, onPressed: (e) { ga.select( gac.VsCodeFlutterSidebar.id, diff --git a/packages/devtools_app/lib/src/standalone_ui/vs_code/devtools.dart b/packages/devtools_app/lib/src/standalone_ui/vs_code/devtools.dart index 56167f88da7..0aa4aa6978a 100644 --- a/packages/devtools_app/lib/src/standalone_ui/vs_code/devtools.dart +++ b/packages/devtools_app/lib/src/standalone_ui/vs_code/devtools.dart @@ -124,10 +124,10 @@ class _DevToolsExtensionsState extends State<_DevToolsExtensions> _extensionService = ExtensionService(ignoreServiceConnection: true); cancelListeners(); - extensions = _extensionService!.visibleExtensions.value; - addAutoDisposeListener(_extensionService!.visibleExtensions, () { + extensions = _extensionService!.visibleExtensions; + addAutoDisposeListener(_extensionService!.currentExtensions, () { setState(() { - extensions = _extensionService!.visibleExtensions.value; + extensions = _extensionService!.visibleExtensions; }); }); diff --git a/packages/devtools_app/test/extensions/extension_screen_test.dart b/packages/devtools_app/test/extensions/extension_screen_test.dart index 875d00ab2d8..ba97d843e76 100644 --- a/packages/devtools_app/test/extensions/extension_screen_test.dart +++ b/packages/devtools_app/test/extensions/extension_screen_test.dart @@ -41,7 +41,7 @@ void main() { await extensionService.initialize(); expect(extensionService.staticExtensions.length, 4); expect(extensionService.runtimeExtensions.length, 3); - expect(extensionService.availableExtensions.value.length, 5); + expect(extensionService.availableExtensions.length, 5); }); tearDown(() { diff --git a/packages/devtools_app/test/extensions/extension_service_test.dart b/packages/devtools_app/test/extensions/extension_service_test.dart index 0311e3aeb08..abddea53d87 100644 --- a/packages/devtools_app/test/extensions/extension_service_test.dart +++ b/packages/devtools_app/test/extensions/extension_service_test.dart @@ -37,14 +37,14 @@ void main() { final service = ExtensionService( fixedAppRoot: Uri.parse('file:///Users/me/package_root_1'), ); - expect(service.availableExtensions.value, isEmpty); + expect(service.availableExtensions, isEmpty); expect(service.staticExtensions, isEmpty); expect(service.runtimeExtensions, isEmpty); await service.initialize(); expect(service.staticExtensions.length, 4); expect(service.runtimeExtensions.length, 3); - expect(service.availableExtensions.value.length, 5); + expect(service.availableExtensions.length, 5); final ignoredStaticExtensions = service.staticExtensions.where(service.isExtensionIgnored); @@ -65,12 +65,12 @@ void main() { final service = ExtensionService(ignoreServiceConnection: true); expect(service.staticExtensions, isEmpty); expect(service.runtimeExtensions, isEmpty); - expect(service.availableExtensions.value, isEmpty); + expect(service.availableExtensions, isEmpty); await service.initialize(); expect(service.staticExtensions.length, 4); expect(service.runtimeExtensions, isEmpty); - expect(service.availableExtensions.value.length, 1); + expect(service.availableExtensions.length, 1); final ignoredStaticExtensions = service.staticExtensions.where(service.isExtensionIgnored); @@ -89,7 +89,7 @@ void main() { await service.initialize(); expect(service.staticExtensions.length, 4); expect(service.runtimeExtensions.length, 3); - expect(service.availableExtensions.value.length, 5); + expect(service.availableExtensions.length, 5); Future enabledOnDisk( DevToolsExtensionConfig ext, diff --git a/packages/devtools_app/test/extensions/extension_settings_test.dart b/packages/devtools_app/test/extensions/extension_settings_test.dart index 804ed9b4594..bbc2ab7bba0 100644 --- a/packages/devtools_app/test/extensions/extension_settings_test.dart +++ b/packages/devtools_app/test/extensions/extension_settings_test.dart @@ -42,10 +42,10 @@ void main() { await extensionService.initialize(); expect(extensionService.staticExtensions.length, 4); expect(extensionService.runtimeExtensions.length, 3); - expect(extensionService.availableExtensions.value.length, 5); + expect(extensionService.availableExtensions.length, 5); dialog = ExtensionSettingsDialog( - extensions: extensionService.availableExtensions.value, + extensions: extensionService.availableExtensions, ); });