From 3f8b72030b9117946746b82c403789d825e2b37d Mon Sep 17 00:00:00 2001 From: Kenzie Davisson <43759233+kenzieschmoll@users.noreply.github.com> Date: Fri, 14 Jul 2023 12:40:08 -0700 Subject: [PATCH] Fix routing issue when refreshing DevTools (#6028) --- packages/devtools_app/lib/src/app.dart | 30 ++--------- .../screens/inspector/inspector_screen.dart | 3 +- .../lib/src/service/service_manager.dart | 19 ++++++- .../devtools_app/lib/src/shared/routing.dart | 6 --- .../devtools_app/lib/src/shared/screen.dart | 11 ++++ .../test/shared/initializer_test.dart | 51 +++++++++---------- 6 files changed, 57 insertions(+), 63 deletions(-) diff --git a/packages/devtools_app/lib/src/app.dart b/packages/devtools_app/lib/src/app.dart index c67ad5545a6..2dd1abcd019 100644 --- a/packages/devtools_app/lib/src/app.dart +++ b/packages/devtools_app/lib/src/app.dart @@ -92,9 +92,6 @@ class DevToolsAppState extends State with AutoDisposeMixin { bool get isDarkThemeEnabled => _isDarkThemeEnabled; bool _isDarkThemeEnabled = true; - bool get vmDeveloperModeEnabled => _vmDeveloperModeEnabled; - bool _vmDeveloperModeEnabled = false; - bool get denseModeEnabled => _denseModeEnabled; bool _denseModeEnabled = false; @@ -131,13 +128,6 @@ class DevToolsAppState extends State with AutoDisposeMixin { }); }); - _vmDeveloperModeEnabled = preferences.vmDeveloperModeEnabled.value; - addAutoDisposeListener(preferences.vmDeveloperModeEnabled, () { - setState(() { - _vmDeveloperModeEnabled = preferences.vmDeveloperModeEnabled.value; - }); - }); - _denseModeEnabled = preferences.denseModeEnabled.value; addAutoDisposeListener(preferences.denseModeEnabled, () { setState(() { @@ -220,11 +210,6 @@ class DevToolsAppState extends State with AutoDisposeMixin { page = params['page']; } - final screens = _visibleScreens() - .where((p) => embed && page != null ? p.screenId == page : true) - .where((p) => !hide.contains(p.screenId)) - .toList(); - final connectedToVmService = vmServiceUri != null && vmServiceUri.isNotEmpty; @@ -234,6 +219,10 @@ class DevToolsAppState extends State with AutoDisposeMixin { return ValueListenableBuilder( valueListenable: preferences.vmDeveloperModeEnabled, builder: (_, __, child) { + final screens = _visibleScreens() + .where((p) => embed && page != null ? p.screenId == page : true) + .where((p) => !hide.contains(p.screenId)) + .toList(); return MultiProvider( providers: _providedControllers(), child: DevToolsScaffold( @@ -282,17 +271,6 @@ class DevToolsAppState extends State with AutoDisposeMixin { ), ); }, - appSizeScreenId: (_, __, args, ____) { - final embed = isEmbedded(args); - return DevToolsScaffold.withChild( - key: const Key('appsize'), - embed: embed, - child: MultiProvider( - providers: _providedControllers(), - child: const AppSizeBody(), - ), - ); - }, if (FeatureFlags.memoryAnalysis) memoryAnalysisScreenId: (_, __, args, ____) { final embed = isEmbedded(args); diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_screen.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_screen.dart index 766347b6609..bd7b339cf4b 100644 --- a/packages/devtools_app/lib/src/screens/inspector/inspector_screen.dart +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_screen.dart @@ -13,7 +13,6 @@ import '../../service/service_extensions.dart' as extensions; import '../../shared/analytics/analytics.dart' as ga; import '../../shared/analytics/constants.dart' as gac; import '../../shared/common_widgets.dart'; -import '../../shared/connected_app.dart'; import '../../shared/console/eval/inspector_tree.dart'; import '../../shared/dialogs.dart'; import '../../shared/editable_list.dart'; @@ -34,7 +33,7 @@ class InspectorScreen extends Screen { InspectorScreen() : super.conditional( id: id, - requiresLibrary: flutterLibraryUri, + requiresFlutter: true, requiresDebugBuild: true, title: ScreenMetaData.inspector.title, icon: ScreenMetaData.inspector.icon, diff --git a/packages/devtools_app/lib/src/service/service_manager.dart b/packages/devtools_app/lib/src/service/service_manager.dart index 372aaca6cbe..a17ca9f7bfd 100644 --- a/packages/devtools_app/lib/src/service/service_manager.dart +++ b/packages/devtools_app/lib/src/service/service_manager.dart @@ -134,6 +134,8 @@ class ServiceConnectionManager { final ValueNotifier _connectedState = ValueNotifier(const ConnectedState(false)); + // TODO(kenz): try to replace all uses of this stream with a listener to the + // [connectedState] ValueListenable. Stream get onConnectionAvailable => _connectionAvailableController.stream; @@ -305,8 +307,6 @@ class ServiceConnectionManager { return; } - _connectedState.value = const ConnectedState(true); - final isolates = vm?.isolatesForDevToolsMode() ?? []; await isolateManager.init(isolates); if (service != this.service) { @@ -332,6 +332,7 @@ class ServiceConnectionManager { } _connectionAvailableController.add(service); + _connectedState.value = const ConnectedState(true); } void manuallyDisconnect() { @@ -566,4 +567,18 @@ class ConnectedState { /// Whether this [ConnectedState] was manually initiated by the user. final bool userInitiatedConnectionState; + + @override + bool operator ==(Object? other) { + return other is ConnectedState && + other.connected == connected && + other.userInitiatedConnectionState == userInitiatedConnectionState; + } + + @override + int get hashCode => Object.hash(connected, userInitiatedConnectionState); + + @override + String toString() => + 'ConnectedState(connected: $connected, userInitiated: $userInitiatedConnectionState)'; } diff --git a/packages/devtools_app/lib/src/shared/routing.dart b/packages/devtools_app/lib/src/shared/routing.dart index 5ed1fa12a32..c1a7f1a4f54 100644 --- a/packages/devtools_app/lib/src/shared/routing.dart +++ b/packages/devtools_app/lib/src/shared/routing.dart @@ -12,12 +12,6 @@ import 'globals.dart'; import 'primitives/auto_dispose.dart'; import 'primitives/utils.dart'; -/// The page ID (used in routing) for the standalone app-size page. -/// -/// This must be different to the AppSizeScreen ID which is also used in routing when -/// cnnected to a VM to ensure they have unique URLs. -const appSizeScreenId = 'appsize'; - const memoryAnalysisScreenId = 'memoryanalysis'; const homeScreenId = ''; diff --git a/packages/devtools_app/lib/src/shared/screen.dart b/packages/devtools_app/lib/src/shared/screen.dart index 06abca51c0a..8bda151666f 100644 --- a/packages/devtools_app/lib/src/shared/screen.dart +++ b/packages/devtools_app/lib/src/shared/screen.dart @@ -56,6 +56,7 @@ abstract class Screen { this.requiresLibrary, this.requiresConnection = true, this.requiresDartVm = false, + this.requiresFlutter = false, this.requiresDebugBuild = false, this.requiresVmDeveloperMode = false, this.worksOffline = false, @@ -68,6 +69,7 @@ abstract class Screen { String? requiresLibrary, bool requiresConnection = true, bool requiresDartVm = false, + bool requiresFlutter = false, bool requiresDebugBuild = false, bool requiresVmDeveloperMode = false, bool worksOffline = false, @@ -82,6 +84,7 @@ abstract class Screen { requiresLibrary: requiresLibrary, requiresConnection: requiresConnection, requiresDartVm: requiresDartVm, + requiresFlutter: requiresFlutter, requiresDebugBuild: requiresDebugBuild, requiresVmDeveloperMode: requiresVmDeveloperMode, worksOffline: worksOffline, @@ -142,6 +145,9 @@ abstract class Screen { /// Whether this screen should only be included when the app is running on the Dart VM. final bool requiresDartVm; + /// Whether this screen should only be included when the app is a Flutter app. + final bool requiresFlutter; + /// Whether this screen should only be included when the app is debuggable. final bool requiresDebugBuild; @@ -296,6 +302,11 @@ bool shouldShowScreen(Screen screen) { return false; } } + if (screen.requiresFlutter && + serviceManager.connectedApp!.isFlutterAppNow == false) { + _log.finest('screen requires Flutter: returning false'); + return false; + } if (screen.requiresDebugBuild) { if (serviceManager.connectedApp!.isProfileBuildNow == true) { _log.finest('screen requires debug build: returning false'); diff --git a/packages/devtools_app/test/shared/initializer_test.dart b/packages/devtools_app/test/shared/initializer_test.dart index 8fb6d297631..464071fc741 100644 --- a/packages/devtools_app/test/shared/initializer_test.dart +++ b/packages/devtools_app/test/shared/initializer_test.dart @@ -13,7 +13,6 @@ import 'package:mockito/mockito.dart'; void main() { group('Initializer', () { - late MaterialApp app; const Key initializedKey = Key('initialized'); setUp(() { final serviceManager = FakeServiceManager(); @@ -22,17 +21,20 @@ void main() { setGlobal(ServiceConnectionManager, serviceManager); setGlobal(FrameworkController, FrameworkController()); setGlobal(OfflineModeController, OfflineModeController()); + setGlobal(IdeTheme, IdeTheme()); + }); - app = MaterialApp( - initialRoute: '/init', - routes: { - '/init': (_) => Initializer( - url: null, - builder: (_) => const SizedBox(key: initializedKey), - ), - }, + Future pumpInitializer(WidgetTester tester) async { + await tester.pumpWidget( + wrap( + Initializer( + url: null, + builder: (_) => const SizedBox(key: initializedKey), + ), + ), ); - }); + await tester.pumpAndSettle(); + } testWidgets( 'shows disconnected overlay if not connected', @@ -43,8 +45,7 @@ void main() { hasConnection: false, ), ); - - await tester.pumpFrames(app, const Duration(milliseconds: 100)); + await pumpInitializer(tester); expect(find.text('Disconnected'), findsOneWidget); }, ); @@ -56,15 +57,16 @@ void main() { setGlobal(ServiceConnectionManager, serviceManager); // Expect standard connected state. - await tester.pumpFrames(app, const Duration(milliseconds: 100)); + serviceManager.changeState(true); + await pumpInitializer(tester); expect(find.byKey(initializedKey), findsOneWidget); expect(find.text('Disconnected'), findsNothing); // Trigger a disconnect. serviceManager.changeState(false); + await tester.pumpAndSettle(const Duration(microseconds: 1000)); // Expect Disconnected overlay. - await tester.pumpFrames(app, const Duration(milliseconds: 100)); expect(find.text('Disconnected'), findsOneWidget); }, ); @@ -75,28 +77,23 @@ void main() { final serviceManager = FakeServiceManager(); setGlobal(ServiceConnectionManager, serviceManager); + // Expect standard connected state. + serviceManager.changeState(true); + await pumpInitializer(tester); + expect(find.byKey(initializedKey), findsOneWidget); + expect(find.text('Disconnected'), findsNothing); + // Trigger a disconnect and ensure the overlay appears. - await tester.pumpFrames(app, const Duration(milliseconds: 100)); serviceManager.changeState(false); - await tester.pumpFrames(app, const Duration(milliseconds: 100)); + await tester.pumpAndSettle(); expect(find.text('Disconnected'), findsOneWidget); // Trigger a reconnect serviceManager.changeState(true); + await tester.pumpAndSettle(); // Expect no overlay. - await tester.pumpFrames(app, const Duration(milliseconds: 100)); - expect(find.text('Disconnected'), findsNothing); - }, - ); - - testWidgets( - 'builds contents when initialized', - (WidgetTester tester) async { - await tester.pumpWidget(app); - await tester.pumpAndSettle(); expect(find.text('Disconnected'), findsNothing); - expect(find.byKey(initializedKey), findsOneWidget); }, ); });