From d4507c4b9d6f7227a80b4acc9d4a25fe2833a4c0 Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Thu, 15 Apr 2021 11:30:08 -0700 Subject: [PATCH 1/3] Fix issue where first build of the VM Tools status bar would cause a null pointer exception `VMDeveloperToolsScreen` exposes a `ValueListenable` provided by the `VMDeveloperToolsController` in order to control the visibility of the isolate selector. Given that `VMDeveloperToolsScreen.showIsolateSelector` can be invoked before the static controller in `_VMDeveloperToolsScreenState` is initialized, a null pointer exception was being thrown when first building the VM Tools screen. This change updates `DevToolsScreen` to take either a `createController` callback or a pre-instantiated `controller`, allowing for the same instance of `VMDeveloperToolsController` to be used by `VMDeveloperToolsScreen` while also being exposed via `Provider`. --- packages/devtools_app/lib/src/app.dart | 112 +++++++++++------- .../devtools_app/lib/src/status_line.dart | 3 + .../vm_developer_tools_screen.dart | 14 ++- 3 files changed, 79 insertions(+), 50 deletions(-) diff --git a/packages/devtools_app/lib/src/app.dart b/packages/devtools_app/lib/src/app.dart index de2a88b8636..0b9db37a724 100644 --- a/packages/devtools_app/lib/src/app.dart +++ b/packages/devtools_app/lib/src/app.dart @@ -273,7 +273,7 @@ class DevToolsAppState extends State { Widget _providedControllers({@required Widget child, bool offline = false}) { final _providers = widget.screens .where((s) => - s.createController != null && (offline ? s.supportsOffline : true)) + s.providesController && (offline ? s.supportsOffline : true)) .map((s) => s.controllerProvider) .toList(); @@ -303,7 +303,8 @@ class DevToolsAppState extends State { class DevToolsScreen { const DevToolsScreen( this.screen, { - @required this.createController, + this.createController, + this.controller, this.supportsOffline = false, }); @@ -315,17 +316,35 @@ class DevToolsScreen { /// widgets depending on this controller can access it by calling /// `Provider.of(context)`. /// - /// If null, [screen] will be responsible for creating and maintaining its own - /// controller. + /// If [createController] and [controller] are both null, [screen] will be + /// responsible for creating and maintaining its own controller. final C Function() createController; + /// A provided controller for this screen, if non-null. + /// + /// The controller will then be provided via [controllerProvider], and + /// widgets depending on this controller can access it by calling + /// `Provider.of(context)`. + /// + /// If [createController] and [controller] are both null, [screen] will be + /// responsible for creating and maintaining its own controller. + final C controller; + + /// Returns true if a controller was provided for [screen]. If false, + /// [screen] is responsible for creating and maintaining its own controller. + bool get providesController => createController != null || controller != null; + /// Whether this screen has implemented offline support. /// /// Defaults to false. final bool supportsOffline; Provider get controllerProvider { - assert(createController != null); + assert((createController != null && controller == null) || + (createController == null && controller != null)); + if (controller != null) { + return Provider.value(value: controller); + } return Provider(create: (_) => createController()); } } @@ -526,49 +545,54 @@ class SettingsDialog extends StatelessWidget { /// /// Conditional screens can be added to this list, and they will automatically /// be shown or hidden based on the [Screen.conditionalLibrary] provided. -List get defaultScreens => [ - DevToolsScreen( - const InspectorScreen(), - createController: () => InspectorSettingsController(), - ), - DevToolsScreen( - const PerformanceScreen(), - createController: () => PerformanceController(), - supportsOffline: true, - ), - DevToolsScreen( - const ProfilerScreen(), - createController: () => ProfilerScreenController(), - supportsOffline: true, - ), - DevToolsScreen( - const MemoryScreen(), - createController: () => MemoryController(), - ), - DevToolsScreen( - const DebuggerScreen(), - createController: () => DebuggerController(), - ), - DevToolsScreen( - const NetworkScreen(), - createController: () => NetworkController(), - ), - DevToolsScreen( - const LoggingScreen(), - createController: () => LoggingController(), - ), - DevToolsScreen( - const AppSizeScreen(), - createController: () => AppSizeController(), - ), - DevToolsScreen( - const VMDeveloperToolsScreen(), - createController: () => VMDeveloperToolsController(), +List get defaultScreens { + final vmDeveloperToolsController = VMDeveloperToolsController(); + return [ + DevToolsScreen( + const InspectorScreen(), + createController: () => InspectorSettingsController(), + ), + DevToolsScreen( + const PerformanceScreen(), + createController: () => PerformanceController(), + supportsOffline: true, + ), + DevToolsScreen( + const ProfilerScreen(), + createController: () => ProfilerScreenController(), + supportsOffline: true, + ), + DevToolsScreen( + const MemoryScreen(), + createController: () => MemoryController(), + ), + DevToolsScreen( + const DebuggerScreen(), + createController: () => DebuggerController(), + ), + DevToolsScreen( + const NetworkScreen(), + createController: () => NetworkController(), + ), + DevToolsScreen( + const LoggingScreen(), + createController: () => LoggingController(), + ), + DevToolsScreen( + const AppSizeScreen(), + createController: () => AppSizeController(), + ), + DevToolsScreen( + VMDeveloperToolsScreen( + controller: vmDeveloperToolsController ), + controller: vmDeveloperToolsController, + ), // Uncomment to see a sample implementation of a conditional screen. // DevToolsScreen( // const ExampleConditionalScreen(), // createController: () => ExampleController(), // supportsOffline: true, // ), - ]; + ]; +} diff --git a/packages/devtools_app/lib/src/status_line.dart b/packages/devtools_app/lib/src/status_line.dart index 26160474c42..1e5efa6904f 100644 --- a/packages/devtools_app/lib/src/status_line.dart +++ b/packages/devtools_app/lib/src/status_line.dart @@ -47,6 +47,9 @@ class StatusLine extends StatelessWidget { // Optionally display an isolate selector. if (currentScreen != null) { + // Ensure that the current screen's state is initialized. In particular, + // make sure that the screen's controller is available before calling + // `currentScreen.showIsolateSelector`. children.add( ValueListenableBuilder( valueListenable: currentScreen.showIsolateSelector, diff --git a/packages/devtools_app/lib/src/vm_developer/vm_developer_tools_screen.dart b/packages/devtools_app/lib/src/vm_developer/vm_developer_tools_screen.dart index 95b4349abad..bb81aed6721 100644 --- a/packages/devtools_app/lib/src/vm_developer/vm_developer_tools_screen.dart +++ b/packages/devtools_app/lib/src/vm_developer/vm_developer_tools_screen.dart @@ -38,8 +38,9 @@ abstract class VMDeveloperView { } class VMDeveloperToolsScreen extends Screen { - const VMDeveloperToolsScreen() - : super.conditional( + const VMDeveloperToolsScreen({ + @required this.controller, + }) : super.conditional( id: id, title: 'VM Tools', icon: Icons.settings_applications, @@ -48,9 +49,11 @@ class VMDeveloperToolsScreen extends Screen { static const id = 'vm-tools'; + final VMDeveloperToolsController controller; + @override ValueListenable get showIsolateSelector => - _VMDeveloperToolsScreenState.controller.showIsolateSelector; + controller.showIsolateSelector; @override Widget build(BuildContext context) => const VMDeveloperToolsScreenBody(); @@ -70,9 +73,8 @@ class VMDeveloperToolsScreenBody extends StatefulWidget { class _VMDeveloperToolsScreenState extends State with AutoDisposeMixin { - // TODO(bkonyi): do we want this to be static? Currently necessary to provide - // access to the `showIsolateSelector` via `VMDeveloperToolsScreen` - static VMDeveloperToolsController controller; + + VMDeveloperToolsController controller; @override void didChangeDependencies() { From efde1271ea1fee4ee1d2f4c5ddbcd9ad6923e95a Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Thu, 15 Apr 2021 12:58:42 -0700 Subject: [PATCH 2/3] Update status_line.dart --- packages/devtools_app/lib/src/status_line.dart | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/devtools_app/lib/src/status_line.dart b/packages/devtools_app/lib/src/status_line.dart index 1e5efa6904f..26160474c42 100644 --- a/packages/devtools_app/lib/src/status_line.dart +++ b/packages/devtools_app/lib/src/status_line.dart @@ -47,9 +47,6 @@ class StatusLine extends StatelessWidget { // Optionally display an isolate selector. if (currentScreen != null) { - // Ensure that the current screen's state is initialized. In particular, - // make sure that the screen's controller is available before calling - // `currentScreen.showIsolateSelector`. children.add( ValueListenableBuilder( valueListenable: currentScreen.showIsolateSelector, From 38542d7abe5702ae312617ee96136f3cec951616 Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Thu, 15 Apr 2021 13:03:20 -0700 Subject: [PATCH 3/3] Add assertion --- packages/devtools_app/lib/src/app.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/devtools_app/lib/src/app.dart b/packages/devtools_app/lib/src/app.dart index 0b9db37a724..2cbd33f7682 100644 --- a/packages/devtools_app/lib/src/app.dart +++ b/packages/devtools_app/lib/src/app.dart @@ -306,7 +306,7 @@ class DevToolsScreen { this.createController, this.controller, this.supportsOffline = false, - }); + }) : assert(createController == null || controller == null); final Screen screen;