Skip to content

Commit

Permalink
Store available and visible DevTools extensions in a single pattern (f…
Browse files Browse the repository at this point in the history
  • Loading branch information
kenzieschmoll authored May 21, 2024
1 parent 699b0ab commit 3ed4989
Show file tree
Hide file tree
Showing 9 changed files with 98 additions and 66 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
[
Expand All @@ -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,
[
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -193,8 +193,7 @@ Future<void> _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,
Expand Down Expand Up @@ -222,8 +221,7 @@ Future<void> _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);
}
Expand Down Expand Up @@ -352,22 +350,22 @@ Future<void> _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);
expect(
preferences.devToolsExtensions.showOnlyEnabledExtensions.value,
isFalse,
);
expect(extensionService.visibleExtensions.value.length, 5);
expect(extensionService.visibleExtensions.length, 5);

await _closeExtensionSettingsMenu(tester);
}
Expand Down
14 changes: 5 additions & 9 deletions packages/devtools_app/lib/src/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ class DevToolsAppState extends State<DevToolsApp> 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
Expand All @@ -110,7 +111,7 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
widget.originalScreens.map((s) => s.screen).toList();

Iterable<Screen> get _extensionScreens =>
extensionService.visibleExtensions.value.map(
extensionService.visibleExtensions.map(
(e) => DevToolsScreen<void>(ExtensionScreen(e)).screen,
);

Expand Down Expand Up @@ -152,11 +153,7 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {

if (FeatureFlags.devToolsExtensions) {
addAutoDisposeListener(
extensionService.availableExtensions,
clearRoutesAndSetState,
);
addAutoDisposeListener(
extensionService.visibleExtensions,
extensionService.currentExtensions,
clearRoutesAndSetState,
);
}
Expand Down Expand Up @@ -263,8 +260,7 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
return MultiValueListenableBuilder(
listenables: [
preferences.vmDeveloperModeEnabled,
extensionService.availableExtensions,
extensionService.visibleExtensions,
extensionService.currentExtensions,
],
builder: (_, __, child) {
final screens = _visibleScreens()
Expand Down
81 changes: 59 additions & 22 deletions packages/devtools_app/lib/src/extensions/extension_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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<DevToolsExtensionConfig> availableExtensions,

/// DevTools extensions that are visible in their own DevTools screen (i.e.
/// extensions that have not been manually disabled by the user).
List<DevToolsExtensionConfig> visibleExtensions,
});

class ExtensionService extends DisposableController
with AutoDisposeControllerMixin {
ExtensionService({this.fixedAppRoot, this.ignoreServiceConnection = false});
Expand All @@ -36,22 +53,35 @@ 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<DevToolsExtensionsGroup> get currentExtensions =>
_currentExtensions;
final _currentExtensions = ValueNotifier<DevToolsExtensionsGroup>(
(
availableExtensions: <DevToolsExtensionConfig>[],
visibleExtensions: <DevToolsExtensionConfig>[],
),
);

/// 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].
ValueListenable<List<DevToolsExtensionConfig>> get availableExtensions =>
_availableExtensions;
final _availableExtensions = ValueNotifier<List<DevToolsExtensionConfig>>([]);
List<DevToolsExtensionConfig> 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<List<DevToolsExtensionConfig>> get visibleExtensions =>
_visibleExtensions;
final _visibleExtensions = ValueNotifier<List<DevToolsExtensionConfig>>([]);
List<DevToolsExtensionConfig> get visibleExtensions =>
_currentExtensions.value.visibleExtensions;

/// DevTools extensions available in the user's project that do not require a
/// running application.
Expand Down Expand Up @@ -132,7 +162,9 @@ class ExtensionService extends DisposableController
addAutoDisposeListener(
preferences.devToolsExtensions.showOnlyEnabledExtensions,
() async {
await _refreshExtensionEnabledStates();
await _refreshExtensionEnabledStates(
availableExtensions: _currentExtensions.value.availableExtensions,
);
},
);

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -254,12 +286,14 @@ class ExtensionService extends DisposableController
}
}

Future<void> _refreshExtensionEnabledStates() async {
Future<void> _refreshExtensionEnabledStates({
required List<DevToolsExtensionConfig> availableExtensions,
}) async {
final onlyIncludeEnabled =
preferences.devToolsExtensions.showOnlyEnabledExtensions.value;

final visible = <DevToolsExtensionConfig>[];
for (final extension in _availableExtensions.value) {
for (final extension in availableExtensions) {
final stateFromOptionsFile = await server.extensionEnabledState(
devtoolsOptionsFileUri: extension.devtoolsOptionsUri,
extensionName: extension.name,
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -339,9 +376,10 @@ class ExtensionService extends DisposableController
runtimeExtensions.clear();
staticExtensions.clear();
_ignoredStaticExtensionsByHashCode.clear();

_availableExtensions.value = [];
_visibleExtensions.value = [];
_currentExtensions.value = (
availableExtensions: <DevToolsExtensionConfig>[],
visibleExtensions: <DevToolsExtensionConfig>[],
);
_extensionEnabledStates.clear();
_refreshInProgress.value = false;
}
Expand All @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ class ExtensionSettingsAction extends ScaffoldAction {
showDialog(
context: context,
builder: (context) => ExtensionSettingsDialog(
extensions: extensionService.availableExtensions.value,
extensions: extensionService
.currentExtensions.value.availableExtensions,
),
),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(() {
Expand Down
Loading

0 comments on commit 3ed4989

Please sign in to comment.