Skip to content

Use isolateManager as source of truth for isPaused. #5018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jan 10, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -1145,7 +1145,7 @@ class _LineItemState extends State<LineItem>
// ignore: avoid-unused-parameters
required bool Function() isHoverStale,
}) async {
if (!serviceManager.appState.isPaused.value) return null;
if (!serviceManager.isMainIsolatePaused) return null;

final word = wordForHover(
event.localPosition.dx,
Expand Down
15 changes: 11 additions & 4 deletions packages/devtools_app/lib/src/screens/debugger/controls.dart
Original file line number Diff line number Diff line change
Expand Up @@ -33,24 +33,31 @@ class _DebuggingControlsState extends State<DebuggingControls>
void didChangeDependencies() {
super.didChangeDependencies();
if (!initController()) return;
addAutoDisposeListener(serviceManager.appState.isPaused);
addAutoDisposeListener(
serviceManager.isolateManager.mainIsolateState!.isPaused,
);
addAutoDisposeListener(controller.resuming);
addAutoDisposeListener(controller.stackFramesWithLocation);
}

@override
Widget build(BuildContext context) {
final isPaused = serviceManager.appState.isPaused.value;
final resuming = controller.resuming.value;
final hasStackFrames = controller.stackFramesWithLocation.value.isNotEmpty;
final isSystemIsolate = controller.isSystemIsolate;
final canStep = isPaused && !resuming && hasStackFrames && !isSystemIsolate;
final canStep = serviceManager.isMainIsolatePaused &&
!resuming &&
hasStackFrames &&
!isSystemIsolate;
final isVmApp = serviceManager.connectedApp?.isRunningOnDartVM ?? false;
return SizedBox(
height: defaultButtonHeight,
child: Row(
children: [
_pauseAndResumeButtons(isPaused: isPaused, resuming: resuming),
_pauseAndResumeButtons(
isPaused: serviceManager.isMainIsolatePaused,
resuming: resuming,
),
const SizedBox(width: denseSpacing),
_stepButtons(canStep: canStep),
const SizedBox(width: denseSpacing),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ class DebuggerController extends DisposableController

final appState = serviceManager.appState;

appState.setPausedOnBreakpoint(false);
_resuming.value = false;
_lastEvent = null;
_stackFramesWithLocation.value = [];
Expand Down Expand Up @@ -168,7 +167,6 @@ class DebuggerController extends DisposableController
// and modify to detect if app is paused from the isolate
// https://github.com/flutter/devtools/pull/4993#discussion_r1060845351

serviceManager.appState.setPausedOnBreakpoint(false);
await _pause(false);

_clearCaches();
Expand Down Expand Up @@ -373,7 +371,6 @@ class DebuggerController extends DisposableController
// serviceManager.isolateManager.selectedIsolateState.isPaused.value;
// listening for changes there instead of having separate logic.
await _getStackOperation?.cancel();
serviceManager.appState.setPausedOnBreakpoint(paused);

_log.log('_pause(running: ${!paused})');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -360,27 +360,29 @@ class DebuggerStatus extends StatefulWidget {
class _DebuggerStatusState extends State<DebuggerStatus> with AutoDisposeMixin {
String _status = '';

bool get _isPaused => serviceManager.isMainIsolatePaused;

@override
void initState() {
super.initState();

addAutoDisposeListener(
serviceManager.appState.isPaused,
() => unawaited(
_updateStatus(),
),
);

_updateStatusOnPause();
unawaited(_updateStatus());
}

@override
void didUpdateWidget(DebuggerStatus oldWidget) {
super.didUpdateWidget(oldWidget);

// todo: should we check that widget.controller != oldWidget.controller?
if (widget.controller == oldWidget.controller) return;

cancelListeners();
_updateStatusOnPause();
}

void _updateStatusOnPause() {
addAutoDisposeListener(
serviceManager.appState.isPaused,
serviceManager.isolateManager.mainIsolateState!.isPaused,
() => unawaited(
_updateStatus(),
),
Expand All @@ -406,9 +408,7 @@ class _DebuggerStatusState extends State<DebuggerStatus> with AutoDisposeMixin {
}

Future<String> _computeStatus() async {
final paused = serviceManager.appState.isPaused.value;

if (!paused) {
if (!_isPaused) {
return 'running';
}

Expand Down Expand Up @@ -447,20 +447,21 @@ class _FloatingDebuggerControlsState extends State<FloatingDebuggerControls>
with
AutoDisposeMixin,
ProvidedControllerMixin<DebuggerController, FloatingDebuggerControls> {
late bool paused;

late double controlHeight;

bool get _isPaused => serviceManager.isMainIsolatePaused;

@override
void didChangeDependencies() {
super.didChangeDependencies();
if (!initController()) return;
paused = serviceManager.appState.isPaused.value;
controlHeight = paused ? defaultButtonHeight : 0.0;
addAutoDisposeListener(serviceManager.appState.isPaused, () {
cancelListeners();

controlHeight = _isPaused ? defaultButtonHeight : 0.0;
addAutoDisposeListener(
serviceManager.isolateManager.mainIsolateState!.isPaused, () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could isMainIsolatePaused just be a notifier too?
I feel like there is a bit of a disconnect by using isMainIsolatePaused as a proxy to state inside the isolateManager, and then needing to know that the two are connected if you ever want to monitor that the state has changed.
also will _isPaused update here, if the paused state changes?

Copy link
Contributor Author

@polina-c polina-c Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isMainIsolatePaused is a helper method. It's value is in shortening the code. If I make it notifier, the code will increase by .value in many places.
Yes, there are pros and cons here. But, whoever would want to listen to the value, will start with reading the method body and will understand how to monitor the state.
Added documentation to decrease possible confusion.

setState(() {
paused = serviceManager.appState.isPaused.value;
if (paused) {
if (_isPaused) {
controlHeight = defaultButtonHeight;
}
});
Expand All @@ -470,10 +471,10 @@ class _FloatingDebuggerControlsState extends State<FloatingDebuggerControls>
@override
Widget build(BuildContext context) {
return AnimatedOpacity(
opacity: paused ? 1.0 : 0.0,
opacity: _isPaused ? 1.0 : 0.0,
duration: longDuration,
onEnd: () {
if (!paused) {
if (!_isPaused) {
setState(() {
controlHeight = 0.0;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ class ProgramExplorerController extends DisposableController
ValueListenable<int> get selectedNodeIndex => _selectedNodeIndex;
final _selectedNodeIndex = ValueNotifier<int>(0);

IsolateRef? _isolate;

/// Notifies that the controller has finished initializing.
ValueListenable<bool> get initialized => _initialized;
final _initialized = ValueNotifier<bool>(false);
Expand All @@ -76,10 +74,10 @@ class ProgramExplorerController extends DisposableController
}
_initializing = true;

_isolate = serviceManager.isolateManager.selectedIsolate.value;
final libraries = _isolate != null
final isolate = serviceManager.isolateManager.selectedIsolate.value;
final libraries = isolate != null
? serviceManager.isolateManager
.isolateDebuggerState(_isolate)!
.isolateState(isolate)
.isolateNow!
.libraries!
: <LibraryRef>[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class VMServiceObjectNode extends TreeNode<VMServiceObjectNode> {
final service = serviceManager.service!;
final isolate = serviceManager.isolateManager.selectedIsolate.value!;
final libRef = serviceManager.isolateManager
.isolateDebuggerState(isolate)!
.isolateState(isolate)
.isolateNow!
.libraries!
.firstWhere(
Expand Down Expand Up @@ -194,7 +194,7 @@ class VMServiceObjectNode extends TreeNode<VMServiceObjectNode> {
// part of a package. Otherwise, it's a file path and its directory should
// appear near the top of the list anyway.
final rootLibUri = serviceManager
.isolateManager.mainIsolateDebuggerState?.isolateNow?.rootLib?.uri;
.isolateManager.mainIsolateState?.isolateNow?.rootLib?.uri;
if (rootLibUri != null) {
if (rootLibUri.startsWith('package:') ||
rootLibUri.startsWith('google3:')) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ class _InspectorTreeState extends State<InspectorTree>
autoDisposeFocusNode(_focusNode);

callOnceWhenReady(
trigger: serviceManager.isolateManager.mainIsolateDebuggerState!.isPaused,
trigger: serviceManager.isolateManager.mainIsolateState!.isPaused,
callback: _bindToController,
readyWhen: (triggerValue) => triggerValue == false,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,12 @@ class IsolateManager extends Disposer {
await _initIsolates(isolates);
}

IsolateState? get mainIsolateDebuggerState {
IsolateState? get mainIsolateState {
return _mainIsolate.value != null
? _isolateStates[_mainIsolate.value!]
: null;
}

IsolateState? isolateDebuggerState(IsolateRef? isolate) {
return isolate != null ? _isolateStates[isolate] : null;
}

/// Return a unique, monotonically increasing number for this Isolate.
int? isolateIndex(IsolateRef isolateRef) {
if (!_isolateIndexMap.containsKey(isolateRef.id)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ class ServiceConnectionManager {

final isolateManager = IsolateManager();

/// Proxy to state inside the isolateManager, for code consizeness.
///
/// Defaults to false if there is no main isolate.
bool get isMainIsolatePaused =>
isolateManager.mainIsolateState?.isPaused.value ?? false;

final consoleService = ConsoleService();

final resolvedUriManager = ResolvedUriManager();
Expand Down Expand Up @@ -547,7 +553,7 @@ class ServiceConnectionManager {
if (uri == null) return false;
assert(_serviceAvailable.isCompleted);
assert(serviceManager.isolateManager.mainIsolate.value != null);
final isolate = isolateManager.mainIsolateDebuggerState!.isolateNow;
final isolate = isolateManager.mainIsolateState!.isolateNow;
return (isolate?.libraries ?? [])
.map((ref) => ref.uri)
.toList()
Expand Down
7 changes: 0 additions & 7 deletions packages/devtools_app/lib/src/shared/connected_app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,6 @@ class AppState extends DisposableController with AutoDisposeControllerMixin {
final _variables = ValueNotifier<List<DartObjectNode>>([]);
void setVariables(List<DartObjectNode> value) => _variables.value = value;

ValueListenable<bool> get isPaused => _isPaused;
final _isPaused = ValueNotifier<bool>(false);

/// This setter should be invoked only by debugger.
void setPausedOnBreakpoint(bool value) => _isPaused.value = value;

ValueListenable<Frame?> get currentFrame => _currentFrame;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok for us to be removing the setPausedOnBreakpoint function in a refactor?
It sounds important

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the value here _isPaused not used any more, so setter does not make sense

final _currentFrame = ValueNotifier<Frame?>(null);
void setCurrentFrame(Frame? value) => _currentFrame.value = value;
Expand All @@ -267,7 +261,6 @@ class AppState extends DisposableController with AutoDisposeControllerMixin {
@override
void dispose() {
_variables.dispose();
_isPaused.dispose();
_currentFrame.dispose();
super.dispose();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class EvalService extends DisposableController with AutoDisposeControllerMixin {
Future<Response> evalAtCurrentFrame(String expression) {
final appState = serviceManager.appState;

if (!appState.isPaused.value) {
if (!serviceManager.isMainIsolatePaused) {
return Future.error(
RPCError.withDetails(
'evaluateInFrame',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,7 @@ class ExpressionEvalFieldState extends State<ExpressionEvalField>
if (expressionText.isEmpty) return;

// Only try to eval if we are paused.
if (!serviceManager
.isolateManager.mainIsolateDebuggerState!.isPaused.value) {
if (!serviceManager.isMainIsolatePaused) {
notificationService
.push('Application must be paused to support expression evaluation.');
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,7 @@ abstract class InspectorServiceBase extends DisposableController
/// The VM Service protocol must be used when paused at a breakpoint as the
/// Daemon API calls won't execute until after the current frame is done
/// rendering.
bool get useDaemonApi {
return !(serviceManager
.isolateManager.mainIsolateDebuggerState?.isPaused.value ??
false);
}
bool get useDaemonApi => !serviceManager.isMainIsolatePaused;

@override
void dispose() {
Expand Down
3 changes: 1 addition & 2 deletions packages/devtools_app/lib/src/shared/preferences.dart
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,7 @@ class InspectorPreferencesController extends DisposableController
() {
if (_mainScriptDir != null &&
serviceManager.isolateManager.mainIsolate.value != null) {
final debuggerState =
serviceManager.isolateManager.mainIsolateDebuggerState;
final debuggerState = serviceManager.isolateManager.mainIsolateState;

if (debuggerState?.isPaused.value == false) {
// the isolate is already unpaused, we can try to load
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ void main() {
setGlobal(NotificationService, NotificationService());
fakeServiceManager.consoleService.ensureServiceInitialized();

fakeServiceManager.appState.setPausedOnBreakpoint(true);
setUp(() {
fakeServiceManager.isMainIsolatePaused = true;
});

Future<void> pumpControls(WidgetTester tester) async {
await tester.pumpWidget(
Expand All @@ -41,6 +43,7 @@ void main() {

testWidgets('display as expected', (WidgetTester tester) async {
await pumpControls(tester);

final animatedOpacityFinder = find.byType(AnimatedOpacity);
expect(animatedOpacityFinder, findsOneWidget);
final animatedOpacity =
Expand Down Expand Up @@ -85,7 +88,9 @@ void main() {
});

testWidgets('are hidden when app is not paused', (WidgetTester tester) async {
fakeServiceManager.appState.setPausedOnBreakpoint(false);
final state =
fakeServiceManager.isolateManager.mainIsolateState! as MockIsolateState;
state.isPaused.value = false;
await pumpControls(tester);
final animatedOpacityFinder = find.byType(AnimatedOpacity);
expect(animatedOpacityFinder, findsOneWidget);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ void main() {
}

setUp(() {
fakeServiceManager.appState.setPausedOnBreakpoint(true);
final state =
fakeServiceManager.isolateManager.mainIsolateState! as MockIsolateState;
state.isPaused.value = true;
});

testWidgetsWithWindowSize(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ void main() {
when(mockServiceManager.errorBadgeManager).thenReturn(mockErrorBadgeManager);
when(mockErrorBadgeManager.errorCountNotifier(any))
.thenReturn(ValueNotifier<int>(0));
when(mockServiceManager.isMainIsolatePaused).thenReturn(false);

setGlobal(ServiceConnectionManager, mockServiceManager);
setGlobal(FrameworkController, FrameworkController());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ void main() {
AppState(mockServiceManager.isolateManager.selectedIsolate),
);
final mockDebuggerController = MockDebuggerController();
mockServiceManager.appState.setPausedOnBreakpoint(true);
final state =
serviceManager.isolateManager.mainIsolateState! as MockIsolateState;
state.isPaused.value = true;
when(mockServiceManager.isMainIsolatePaused).thenReturn(false);

await tester.pumpWidget(
wrapWithControllers(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ void main() {
when(mockServiceManager.connectedApp).thenReturn(mockConnectedApp);
final mockDebuggerController = MockDebuggerController();

mockServiceManager.appState.setPausedOnBreakpoint(true);
final state =
serviceManager.isolateManager.mainIsolateState! as MockIsolateState;
state.isPaused.value = true;

await tester.pumpWidget(
wrapWithControllers(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Future<void> main() async {
.thenReturn(AppState(mockServiceManager.isolateManager.selectedIsolate));
when(mockServiceManager.runDeviceBusyTask(Future<void>.value()))
.thenAnswer((_) => Future<void>.value());
when(mockServiceManager.isMainIsolatePaused).thenReturn(false);
setGlobal(ServiceConnectionManager, mockServiceManager);
setGlobal(NotificationService, NotificationService());

Expand Down
Loading