Extract evalService from debuggerController that serves console. #4992

polina-c · 2022-12-25T00:20:48Z

RELEASE_NOTE_EXCEPTION=[no functional changes]

…alService

jacob314 · 2022-12-27T17:37:54Z

packages/devtools_app/lib/src/screens/debugger/codeview.dart

@@ -1154,7 +1154,7 @@ class _LineItemState extends State<LineItem>

    if (word != '') {
      try {
-        final response = await controller.evalAtCurrentFrame(word);
+        final response = await controller.evalService.evalAtCurrentFrame(word);


This case is tricky as you wouldn't want any injected variables from the console to show up when evaluating while hovering over source code. Maybe this case should take an extra named parameter like includeConsoleVariables: false

Discussed and agreed evalAtCurrentFrame will never be invoked for global scope.

jacob314 · 2022-12-27T17:41:27Z

packages/devtools_app/lib/src/shared/console/eval/eval_service.dart

+    required this.service,
+  });
+
+  final IsolateRef? Function() isolateRef;


It would seem more consistent to use ValueListenable for all of these rather than callbacks.

ValueListenable is too powerful. I want it to be clear that EvalService does not need to listen for the changes. It just needs up-to-date value.

I don't quite buy this. The autocomplete cache is bogus if the isolate changed, for example. ValueListenable does not force you to always listen for changes. As appropriate you can always just use .value to get the current value. On the other end of the spectrum, VmServiceWrapper needs to just be a fixed instance because there is no reason anyone writing this code should worry it can changed.,

My goal is clarity of contract for the method.
I want the constructor parameters to be smallest needed set of what the object needs to function. Of course, not at cost of convenience.

About ValueListenable vs function. I remember a story: when I passed ValueListenable, I got comments in code review like 'why this object need to listen to every change? it is performance overhead'. And then I needed to explain that the object just needs values.

So, it affects readability of code too.

Does it change something for you?

About VmServiceWrapper: as I understand when reconnect happens, DebuggerController is not recreated, but it resets some variables.
So, EvalService will not recreate, but just reset.

Yes, it may be not the best implementation, but it is what we currently have. And it will take more refactoring to change.

And, I did not understand you point about consistency. In Flutter framework there is ton of methods and constructors that take functions, not listeners.

Each time an app connects you get a new DebuggerController. How it happens flows back to how the provider for the DebuggerController is initialized.
The private getter for the service in debuggerController is really just calling
serviceManager.service!
Would be clearer to just call that global from EvalService rather than making it look like it is injected in.

In general, I think ValueListenable is almost always the right choice if something can actually change. If it can't change then pass in the actual value when initializing.

Converted service to variable, isPaused and variables to listenable.

jacob314 · 2022-12-27T17:41:59Z

packages/devtools_app/lib/src/shared/console/eval/eval_service.dart

+
+  String get _isolateRefId {
+    final id = isolateRef()?.id;
+    if (id == null) return '';


why is returning '' for the isolateRefId ever ok?

I do not know. I copied it from previous code. Added 'TODO'.

jacob314 · 2022-12-27T18:38:31Z

packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart

-  /// The return value can be one of [Obj] or [Sentinel].
-  Future<Obj> getObject(ObjRef objRef) {
-    return _service.getObject(_isolateRefId, objRef.id!);
+    evalService.cache.clear();


this is badly coupled. Instead EvalService should listen for isolate changes itself instead of requiring the debuggerControl to call this method at the appropriate time.

thanks. fixed.

jacob314 · 2022-12-27T22:43:58Z

packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart

+  late final EvalService evalService = EvalService(
+    isolateRef: isolateRef,
+    variables: variables,
+    frameForEval: () => _frameForEval,


_frameForEval should also be made a ValueListenable for consistency. That would probably cleanup some of the debugger UI that should be listening for changes to the frameForEval.

It does not seem to make sense for me to recalculate and update frameForEval every time _selectedStackFrame or _stackFramesWithLocation change, from both code readability/simplicity and performance perspective.

Deleted getter and moved the logic to the function, as the getter is not used in other places..

jacob314 · 2022-12-27T22:44:19Z

packages/devtools_app/lib/src/screens/debugger/debugger_controller.dart

    }
  }

  final codeViewController = CodeViewController();

+  late final EvalService evalService = EvalService(


make this final instead of late final.

Initializer cannot use getters:

Yeah. Move the initializer to the constructor to avoid this issue. In general, I would avoid initializing fields inline except for very trivial cases. Otherwise the initialization order semantics can trip some people up when reviewing.

Even after the move it is still late, because it cannot be initialized in constructor initializers, but only in the body. Did i miss something? Let's follow up then.

jacob314

I'll be out tomorrow. Would be nice to remove the late final moving it to a constructor.

polina-c added 30 commits December 23, 2022 15:22

-

400c40a

-

61bd1a2

-

1247f0b

-

b41aa0a

Update expandable_variable.dart

309db19

-

e2f387c

-

3fc9bed

-

15e9aee

-

ae40b63

-

158c633

-

103d4d0

-

6d36473

-

ee83265

-

92bfafa

-

c4a9cd0

-

72f84fc

-

2b33256

Update console.dart

46ea97a

Update console.dart

e16dbbb

-

9cbdb0f

-

4a9f2b8

-

6e0a4a5

-

9253c67

Update eval_service.dart

398e6ca

-

a259c16

-

65a25b6

Update debugger_evaluation_test.dart

85cd166

-

c1ca478

-

c09536b

-

86dd969

polina-c added 4 commits December 25, 2022 11:11

-

593d027

-

3f2e729

Update generated_mocks_factories.dart

27eb226

-

a1048fe

polina-c changed the title ~~Console2 - eval service~~ Extract eval service that serves console. Dec 27, 2022

Merge branch 'master' of github.com:flutter/devtools into console2-ev…

c431c23

…alService

polina-c marked this pull request as ready for review December 27, 2022 17:35

polina-c requested review from elliette and a team as code owners December 27, 2022 17:35

polina-c requested review from bkonyi and removed request for a team December 27, 2022 17:35

polina-c changed the title ~~Extract eval service that serves console.~~ Extract evalService from debuggerController that serves console. Dec 27, 2022

jacob314 reviewed Dec 27, 2022

View reviewed changes

Update eval_service.dart

6d3e9d8

jacob314 reviewed Dec 27, 2022

View reviewed changes

polina-c added 3 commits December 27, 2022 12:06

-

277c29a

-

f815d9b

Update generated_mocks_factories.dart

0b41ed1

jacob314 reviewed Dec 27, 2022

View reviewed changes

Update debugger_controller.dart

7bc6cb4

jacob314 approved these changes Dec 28, 2022

View reviewed changes

polina-c added 2 commits December 28, 2022 11:03

Update debugger_controller.dart

a577748

-

7175896

polina-c merged commit fad158b into flutter:master Dec 28, 2022

polina-c deleted the console2-evalService branch December 28, 2022 20:34

CoderDake mentioned this pull request Jan 24, 2023

test bot pr #5099

Closed

7 tasks

DartDevtoolWorkflowBot mentioned this pull request Jan 24, 2023

test bot pr #5103

Closed

7 tasks

Extract evalService from debuggerController that serves console. #4992

Extract evalService from debuggerController that serves console. #4992

Uh oh!

Conversation

polina-c commented Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

polina-c commented Dec 25, 2022 •

edited

Loading

polina-c Dec 27, 2022 •

edited

Loading

polina-c Dec 27, 2022 •

edited

Loading