Fix a bug with the review history on disconnect experience (#8985)

Author: kenzieschmoll

packages/devtools_app/lib/src/app.dart

@@ -141,12 +141,23 @@ class DevToolsAppState extends State<DevToolsApp> with AutoDisposeMixin {
       }
     });
     addAutoDisposeListener(serviceConnection.serviceManager.connectedState, () {
-      final connected =
-          serviceConnection.serviceManager.connectedState.value.connected;
+      final connectionState =
+          serviceConnection.serviceManager.connectedState.value;
+
+      // When disconnecting from an app, prepare the offline state for reviewing
+      // history.
+      screenControllers.forEachInitialized((screenController) {
+        if (screenController is OfflineScreenControllerMixin &&
+            !connectionState.connected &&
+            !connectionState.userInitiatedConnectionState) {
+          screenController.maybePrepareDataForReviewingHistory();
+        }
+      });
+
       // Dispose the current connected controllers, if any, for any change to
       // the connected state.
       screenControllers.disposeConnectedControllers();
-      _initScreenControllers(connected: connected);
+      _initScreenControllers(connected: connectionState.connected);
     });
 
     // TODO(https://github.com/flutter/devtools/issues/6018): Once
@@ -582,14 +593,10 @@ class DevToolsScreen<C extends DevToolsScreenController> {
     DevToolsRouterDelegate routerDelegate, {
     required bool offline,
   }) {
-    screenControllers.register<C>(() {
-      final controller =
-          createController!(routerDelegate) as DisposableController;
-      if (controller is OfflineScreenControllerMixin) {
-        controller.initReviewHistoryOnDisconnectListener();
-      }
-      return controller as C;
-    }, offline: offline);
+    screenControllers.register<C>(
+      () => createController!(routerDelegate),
+      offline: offline,
+    );
   }
 }
 

packages/devtools_app/lib/src/shared/framework/screen_controllers.dart

@@ -82,6 +82,25 @@ class ScreenControllers {
     }
     offlineControllers.clear();
   }
+
+  /// Calls the [callback] function on each initialized screen controller.
+  ///
+  /// Optionally, calls the [callback] on the offline screen controllers when
+  /// [includeOfflineControllers] is true.
+  void forEachInitialized(
+    void Function(DevToolsScreenController screenController) callback, {
+    bool includeOfflineControllers = false,
+  }) {
+    final controllers =
+        includeOfflineControllers
+            ? [...this.controllers.values, ...offlineControllers.values]
+            : this.controllers.values;
+    for (final lazyController in controllers) {
+      if (lazyController.initialized) {
+        callback(lazyController.controller);
+      }
+    }
+  }
 }
 
 /// Helper class that performs lazy initialization for
@@ -96,6 +115,9 @@ class _LazyController<T extends DevToolsScreenController> {
   T get controller => _controller ??= creator()..init();
   T? _controller;
 
+  /// Whether the controller has been initialized.
+  bool get initialized => _controller != null;
+
   void dispose() {
     _controller?.dispose();
     _controller = null;

packages/devtools_app/lib/src/shared/offline/offline_data.dart

@@ -192,37 +192,24 @@ mixin OfflineScreenControllerMixin<T>
     _exportController.downloadFile(encodedData);
   }
 
-  /// Adds a listener that will prepare the screen's current data for offline
-  /// viewing after an app disconnect.
+  /// Prepare the screen's current data for offline viewing after an app
+  /// disconnect.
   ///
   /// This is in preparation for the user clicking the 'Review History' button
   /// from the disconnect screen.
-  ///
-  /// For screens that support the disconnect experience, which is a screen that
-  /// allows you to view historical data from before the app was disconnected
-  /// even after we lose connection to the device, this should be called in the
-  /// controller's initialization.
-  void initReviewHistoryOnDisconnectListener() {
-    addAutoDisposeListener(serviceConnection.serviceManager.connectedState, () {
-      final connectionState =
-          serviceConnection.serviceManager.connectedState.value;
-      if (!connectionState.connected &&
-          !connectionState.userInitiatedConnectionState) {
-        final currentScreenData = prepareOfflineScreenData();
-        // Only store data for the current page. We can change this in the
-        // future if we support offline imports for more than once screen at a
-        // time.
-        if (DevToolsRouterDelegate.currentPage == currentScreenData.screenId) {
-          final previouslyConnectedApp =
-              offlineDataController.previousConnectedApp;
-          final offlineData = _exportController.generateDataForExport(
-            offlineScreenData: currentScreenData.toJson(),
-            connectedApp: previouslyConnectedApp,
-          );
-          offlineDataController.offlineDataJson = offlineData;
-        }
-      }
-    });
+  void maybePrepareDataForReviewingHistory() {
+    final currentScreenData = prepareOfflineScreenData();
+    // Only store data for the current page. We can change this in the
+    // future if we support offline imports for more than once screen at a
+    // time.
+    if (DevToolsRouterDelegate.currentPage == currentScreenData.screenId) {
+      final previouslyConnectedApp = offlineDataController.previousConnectedApp;
+      final offlineData = _exportController.generateDataForExport(
+        offlineScreenData: currentScreenData.toJson(),
+        connectedApp: previouslyConnectedApp,
+      );
+      offlineDataController.offlineDataJson = offlineData;
+    }
   }
 }
 

packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md

@@ -15,7 +15,8 @@ To learn more about DevTools, check out the
 
 ## General updates
 
-TODO: Remove this section if there are not any general updates.
+* Fix a bug with the review history on disconnect experience. -
+[#8985](https://github.com/flutter/devtools/pull/8985)
 
 ## Inspector updates
 

packages/devtools_app/test/shared/framework/screen_controllers_test.dart

@@ -72,6 +72,10 @@ void main() {
 
       verifyNever(controller1.init());
       verifyNever(controller2.init());
+      expect(
+        screenControllers.controllers.values.any((c) => c.initialized),
+        false,
+      );
     });
 
     test('register overwrites existing controller', () {
@@ -170,9 +174,11 @@ void main() {
       );
 
       verifyNever(controller.init()); // Not initialized yet
+      expect(screenControllers.controllers.values.first.initialized, false);
 
       screenControllers.lookup<MockDevToolsScreenController>();
       verify(controller.init()).called(1); // Now it's initialized
+      expect(screenControllers.controllers.values.first.initialized, true);
     });
   });
 }