diff --git a/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart b/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart index 679bdccd399..f35ab0e690e 100644 --- a/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart +++ b/packages/devtools_app/lib/src/screens/accessibility/accessibility_controller.dart @@ -50,7 +50,6 @@ class AccessibilityController extends DevToolsScreenController } void _initListeners() { - addAutoDisposeListener(brightness, _onBrightnessChanged); addAutoDisposeListener(textScale, _onTextScaleChanged); addAutoDisposeListener(boldText, _onBoldTextChanged); addAutoDisposeListener(screenReader, _onScreenReaderChanged); @@ -94,35 +93,49 @@ class AccessibilityController extends DevToolsScreenController } void _initServiceExtensionStates() { - final state = serviceConnection.serviceManager.serviceExtensionManager - .getServiceExtensionState(extensions.brightnessMode.extension); + final serviceManager = serviceConnection.serviceManager; + late ValueListenable state; - void updateFromDeviceState(ServiceExtensionState state) { - final newBrightness = !state.enabled || state.value == null + void updateFromDeviceState() { + final newBrightness = !state.value.enabled || state.value.value == null ? BrightnessOverride.system : BrightnessOverride.values.firstWhere( - (b) => b.value == state.value, + (b) => b.value == state.value.value, orElse: () => BrightnessOverride.system, ); - brightness.value = newBrightness; + _brightness.value = newBrightness; } - updateFromDeviceState(state.value); - addAutoDisposeListener(state, () => updateFromDeviceState(state.value)); - } + // ServiceExtensionManager stops updating the state that it returned once + // the main isolate exits, such as during a hot restart, so look the state + // up again whenever the main isolate changes. + void listenToDeviceState() { + cancelListener(updateFromDeviceState); + state = serviceManager.serviceExtensionManager.getServiceExtensionState( + extensions.brightnessMode.extension, + ); + addAutoDisposeListener(state, updateFromDeviceState); + updateFromDeviceState(); + } - void _onBrightnessChanged() { - final value = brightness.value; - unawaited( - serviceConnection.serviceManager.serviceExtensionManager - .setServiceExtensionState( - extensions.brightnessMode.extension, - enabled: value != BrightnessOverride.system, - value: value.value, - ), + listenToDeviceState(); + addAutoDisposeListener( + serviceManager.isolateManager.mainIsolate, + listenToDeviceState, ); } + /// Overrides the brightness of the app with [value], or stops overriding it + /// if [value] is [BrightnessOverride.system]. + Future setBrightness(BrightnessOverride value) { + return serviceConnection.serviceManager.serviceExtensionManager + .setServiceExtensionState( + extensions.brightnessMode.extension, + enabled: value != BrightnessOverride.system, + value: value.value, + ); + } + void _onTextScaleChanged() { // TODO(hannah-hyj): Implement VM service extension call for text scale override. } @@ -144,7 +157,10 @@ class AccessibilityController extends DevToolsScreenController final screenId = ScreenMetaData.accessibility.id; // --- Accessibility Overrides State --- - final brightness = ValueNotifier( + + /// The brightness that DevTools last set, or that the app last reported. + ValueListenable get brightness => _brightness; + final _brightness = ValueNotifier( BrightnessOverride.system, ); final textScale = ValueNotifier(1.0); @@ -321,7 +337,7 @@ class AccessibilityController extends DevToolsScreenController @override void dispose() { unawaited(_disposeSemanticsOnApp()); - brightness.dispose(); + _brightness.dispose(); textScale.dispose(); boldText.dispose(); screenReader.dispose(); diff --git a/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart b/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart index 9e2555ac122..b16c5332155 100644 --- a/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart +++ b/packages/devtools_app/lib/src/screens/accessibility/overrides_pane.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. +import 'dart:async'; + import 'package:devtools_app_shared/ui.dart'; import 'package:material_ui/material_ui.dart'; @@ -117,7 +119,7 @@ class _BrightnessOverride extends StatelessWidget { }).toList(), onChanged: (newValue) { if (newValue != null) { - controller.brightness.value = newValue; + unawaited(controller.setBrightness(newValue)); } }, ); diff --git a/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart b/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart index 2575cb09fd3..2848fab5a4b 100644 --- a/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart +++ b/packages/devtools_app/test/screens/accessibility/accessibility_controller_test.dart @@ -100,6 +100,41 @@ void main() { }, ); + test( + 'does not send the brightness that the app reports back to the app', + () async { + final extensionManager = + serviceConnection.serviceManager.serviceExtensionManager + as FakeServiceExtensionManager; + final brightnessExtension = brightnessMode.extension; + extensionManager.extensionValueOnDevice[brightnessExtension] = + 'Brightness.dark'; + await extensionManager.fakeAddServiceExtension(brightnessExtension); + await extensionManager.fakeFrame(); + // Forget the brightness on the device, so that a call to set it shows. + extensionManager.extensionValueOnDevice.remove(brightnessExtension); + + // The page opens after DevTools read the brightness of the app. + final pageController = AccessibilityController()..init(); + addTearDown(pageController.dispose); + expect(pageController.brightness.value, BrightnessOverride.dark); + // Let any call that the page makes in response finish. + await pumpEventQueue(); + + extensionManager.fakeServiceExtensionStateChanged( + brightnessExtension, + 'Brightness.light', + ); + expect(pageController.brightness.value, BrightnessOverride.light); + + await pumpEventQueue(); + expect( + extensionManager.extensionValueOnDevice, + isNot(contains(brightnessExtension)), + ); + }, + ); + test( 'setting controller brightness updates service extension state', () async { @@ -111,20 +146,17 @@ void main() { expect(controller.brightness.value, BrightnessOverride.system); // Set to dark mode - controller.brightness.value = BrightnessOverride.dark; - - // Wait for async operations to complete - await Future.delayed(Duration.zero); + await controller.setBrightness(BrightnessOverride.dark); final darkState = fakeServiceExtensionManager .getServiceExtensionState(brightnessMode.extension) .value; expect(darkState.value, equals('Brightness.dark')); expect(darkState.enabled, isTrue); + expect(controller.brightness.value, BrightnessOverride.dark); // Set to light mode - controller.brightness.value = BrightnessOverride.light; - await Future.delayed(Duration.zero); + await controller.setBrightness(BrightnessOverride.light); final lightState = fakeServiceExtensionManager .getServiceExtensionState(brightnessMode.extension) @@ -133,8 +165,7 @@ void main() { expect(lightState.enabled, isTrue); // Set to system - controller.brightness.value = BrightnessOverride.system; - await Future.delayed(Duration.zero); + await controller.setBrightness(BrightnessOverride.system); final systemState = fakeServiceExtensionManager .getServiceExtensionState(brightnessMode.extension) @@ -144,6 +175,43 @@ void main() { }, ); + test('restores the brightness override after a hot restart', () async { + final extensionManager = + serviceConnection.serviceManager.serviceExtensionManager + as FakeServiceExtensionManager; + final isolateManager = + serviceConnection.serviceManager.isolateManager as FakeIsolateManager; + final brightnessExtension = brightnessMode.extension; + await extensionManager.fakeAddServiceExtension(brightnessExtension); + await extensionManager.fakeFrame(); + await controller.setBrightness(BrightnessOverride.dark); + // Let any call that the page makes in response finish. + await pumpEventQueue(); + + // A hot restart replaces the main isolate, and the new one starts + // without the override. + extensionManager.fakeMainIsolateClosed(); + isolateManager.setMainIsolate(null); + extensionManager.extensionValueOnDevice.remove(brightnessExtension); + isolateManager.setMainIsolate( + IsolateRef.parse({'id': 'new_main_isolate_id'}), + ); + await extensionManager.fakeAddServiceExtension(brightnessExtension); + await extensionManager.fakeFrame(); + expect( + extensionManager.extensionValueOnDevice[brightnessExtension], + 'Brightness.dark', + ); + expect(controller.brightness.value, BrightnessOverride.dark); + + // The page follows the brightness of the new main isolate. + extensionManager.fakeServiceExtensionStateChanged( + brightnessExtension, + 'Brightness.light', + ); + expect(controller.brightness.value, BrightnessOverride.light); + }); + test( 'SemanticsNodeModel properties, parseRect, rectDisplay, and shallowCopy', () { diff --git a/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart b/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart index 5e9823fdd07..09c5688a78a 100644 --- a/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart +++ b/packages/devtools_test/lib/src/mocks/fake_isolate_manager.dart @@ -46,6 +46,12 @@ base class FakeIsolateManager extends Fake with TestIsolateManager { _isolateCreatedController.add(isolate); } + /// Changes the main isolate, as [IsolateManager] does during a hot restart. + @visibleForTesting + void setMainIsolate(IsolateRef? isolate) { + _mainIsolate.value = isolate; + } + @override ValueNotifier> get isolates { return _isolates ??= ValueNotifier([?_selectedIsolate.value]); diff --git a/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart b/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart index fc47c3adacd..7511d697519 100644 --- a/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart +++ b/packages/devtools_test/lib/src/mocks/fake_service_extension_manager.dart @@ -194,6 +194,21 @@ base class FakeServiceExtensionManager extends Fake } } + /// Hook for tests to simulate the main isolate exiting, as it does during a + /// hot restart. + /// + /// Like [ServiceExtensionManager], this resets the listenables that + /// [hasServiceExtension] and [getServiceExtensionState] returned, and then + /// stops updating them. + void fakeMainIsolateClosed() { + vmServiceClosed(); + _serviceExtensionAvailable.clear(); + for (final state in _serviceExtensionStateController.values) { + state.value = ServiceExtensionState(enabled: false, value: null); + } + _serviceExtensionStateController.clear(); + } + /// Sets the state for a service extension and makes the call to the VMService. @override Future setServiceExtensionState(