Replace `ScriptPicker` with `ProgramExplorer` on `DebuggerScreen` #3227

bkonyi · 2021-07-29T21:40:57Z

This change replaces the current ScriptPicker implementation with a more fully featured ProgramExplorer.

Functionality:

Navigate the program structure, including libraries, scripts, classes, functions, and fields
Display generic parameter information for generic classes and functions
View function and field signatures in the program explorer
Color-coded icons for each object type to match existing syntax highlighting
Jump to definition of classes, functions, and fields
Lazy population of class nodes
Filtering based on object names

Additional Changes:

Updated CodeView to allow for scrolling to a location aligned to the top of the view
Fixed issue where CodeView background was transparent
TreeView nodes are now highlighted when selected
Added support for different behaviors for TreeView selection vs expansion

Future Work:

Display code objects in VM developer mode
Allow for inspecting object information in an "object inspector" window in the debugger page

…gram_explorer

bkonyi · 2021-07-29T21:42:15Z

@kenzieschmoll @jacob314 I think this is ready for an initial pass. PTAL when you have a chance.

kenzieschmoll · 2021-08-03T22:00:17Z

Can you add a screenshot of the new program explorer to the PR description?

kenzieschmoll · 2021-08-03T22:11:10Z

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

@@ -78,6 +80,13 @@ class _CodeViewState extends State<CodeView>
    gutterController = verticalController.addAndGet();
    textController = verticalController.addAndGet();

+    if (widget.initialPosition != null) {


This likely also needs to be in didUpdateWidget so that this state object can manage the scroll position if the widget was updated with a new value for initialPosition

agree we need to do something in didUpdateWidget but we will need to be very careful. You don't want to update the scrollPosition unless widget.initialPosition != oldWidget.initialPosition.

kenzieschmoll · 2021-08-03T22:11:32Z

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

@@ -78,6 +80,13 @@ class _CodeViewState extends State<CodeView>
    gutterController = verticalController.addAndGet();
    textController = verticalController.addAndGet();

+    if (widget.initialPosition != null) {
+      final location = widget.initialPosition.location;
+      final lineIndex = location.line - 1;


can you add a comment as to why we are subtracting 1

kenzieschmoll · 2021-08-03T22:20:29Z

packages/devtools_app/lib/src/debugger/controls.dart

@@ -124,7 +124,7 @@ class _DebuggingControlsState extends State<DebuggingControls>
          child: Container(
            color: visible ? Theme.of(context).highlightColor : null,
            child: DebuggerButton(
-              title: 'Libraries',
+              title: 'Program Explorer',


I'm not completely sold on this name. Maybe "Source Explorer"? Input here @jacob314 @devoncarew?

Can we leave it as Libraries for now? We are just showing more information about the libraries.

kenzieschmoll · 2021-08-03T22:22:05Z

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

@@ -278,6 +285,9 @@ class DebuggerController extends DisposableController
  /// Return the sorted list of ScriptRefs active in the current isolate.
  ValueListenable<List<ScriptRef>> get sortedScripts => _sortedScripts;

+  bool get centerScrollLocation => _centerScrollLocation;


is it possible that this value could get accessed for a script it is not associated with? Storing this at the controller level vs the per call level seems problematic

kenzieschmoll · 2021-08-03T22:22:43Z

packages/devtools_app/lib/src/debugger/debugger_model.dart

 class SourcePosition {
-  SourcePosition({@required this.line, @required this.column, this.tokenPos});
+  const SourcePosition(
+      {@required this.line, @required this.column, this.file, this.tokenPos});


nit: trailing comma

kenzieschmoll · 2021-08-03T22:22:54Z

packages/devtools_app/lib/src/debugger/debugger_screen.dart

@@ -79,6 +80,7 @@ class DebuggerScreenBodyState extends State<DebuggerScreenBody>
  static const breakpointsTitle = 'Breakpoints';

  DebuggerController controller;
+  //ObjectTreeController objectSelectorController = ObjectTreeController();


delete comment?

kenzieschmoll · 2021-08-03T22:26:09Z

packages/devtools_app/lib/src/theme.dart

@@ -399,6 +399,11 @@ extension ThemeDataExtension on ThemeData {
  TextStyle get subtleFixedFontStyle =>
      fixedFontStyle.copyWith(color: unselectedWidgetColor);

+  TextStyle get toolTipFixedFontStyle => //colorScheme.isLight ? 


remove comment

kenzieschmoll · 2021-08-03T22:27:40Z

packages/devtools_app/lib/src/history_viewport.dart

+                      if (onChange != null) {
+                        onChange(
+                          history.current.value,
+                          current,


why not implement a peekBack method to mirror what is done above with moveBack and peekNext?

kenzieschmoll · 2021-08-03T22:35:37Z

At a high level, is this change something we have discussed in the past? It seems inconsistent with other discussions we've had regarding debugger UI improvements (see #3075). This is probably something that could use some UX input on (@InMatrix). @devoncarew @jacob314 for input on using a program explorer (classes, functions, fields, etc.) vs a file tree explorer in the debugger (described in linked issue #3075).

My concern is making this view more complicated and thus more difficult for users to find what they are looking for. Do users expect to see a file explorer or a program explorer in a debugger? Should we make displaying low level details (classes, functions, etc.) an optional setting?

InMatrix · 2021-08-03T23:20:03Z

@bkonyi Could you post some before-and-after screenshots to help me better understand this change?

bkonyi · 2021-08-03T23:27:11Z

There wasn't an explicit discussion, but I did chat with @jacob314 offline. In general, we need to be able to examine details of fields, functions, and classes to reach feature parity with Observatory (that work isn't included in this PR as the explorer was already a big change) in addition to examining code objects (in VM developer mode). Inspecting code objects will require a code view in addition to an object details view, so the debugger pane seemed like the right place for this rather than creating another top-level tab.

Here's some screenshots:

Before:

After:

bkonyi · 2021-08-03T23:30:15Z

@InMatrix it's not clear from the screenshots but selecting classes, functions, and fields currently jumps to their definition in the script.

kenzieschmoll · 2021-08-04T16:49:15Z

Some things that are confusing to me:

package and file names are repeated (e.g. dart:http & dart:http on the same line item). Perhaps for packages and files, we could densify the line item and just show the package name (as one line, not two), and file names can just have the file name without the whole path (the path could be displayed as a tooltip or something). This would make the UI look more like a tree file explorer at the package / file level.
Icons for packages / paths should be more basic (e.g. a folder icon for packages and a file or page icon for a file). For example:

Icons for underlying classes, functions, etc should be more descriptive - for example I like the sigma icon for functions.

bkonyi · 2021-08-04T17:01:37Z

package and file names are repeated (e.g. dart:http & dart:http on the same line item). Perhaps for packages and files, we could densify the line item and just show the package name (as one line, not two), and file names can just have the file name without the whole path (the path could be displayed as a tooltip or something). This would make the UI look more like a tree file explorer at the package / file level.

Sure, that definitely makes sense. It looked a bit "empty" to me without the second row, but it's not essential.

Icons for packages / paths should be more basic (e.g. a folder icon for packages and a file or page icon for a file).

The icons for libraries and folders are identical to the icons in the original script picker :-)

Icons for underlying classes, functions, etc should be more descriptive - for example I like the sigma icon for functions.

I agree 1000%. I basically scanned through the Flutter material icons and picked ones that looked okay, so we could definitely use some design input for those.

jacob314 · 2021-08-09T22:01:33Z

With regards to #3075, I don't see this in conflict with it. We won't use this view or the script picker for searching but the view will be more useful than the existing ScriptPicker for browsing.

jacob314 · 2021-08-09T22:07:14Z

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

@@ -580,7 +603,7 @@ class _LinesState extends State<Lines> with AutoDisposeMixin {

        if (isOutOfViewTop || isOutOfViewBottom) {
          // Scroll this search token to the middle of the view.
-          final targetOffset = math.max(
+          final targetOffset = math.max<double>(


why was this type added? seems like it should be inferred.

jacob314 · 2021-08-09T22:13:39Z

packages/devtools_app/lib/src/debugger/program_explorer_model.dart

+  final Map<String, VMServiceObjectNode> _childrenAsMap = {};
+
+  @override
+  bool get isExpandable => super.isExpandable || object is ClassRef;


what if a class has zero members?

jacob314 · 2021-08-09T22:19:29Z

I would suggest aligning the icons with what the structure and project views in IntelliJ or VSCode use.
There are existing icons for files, classes, and methods that users should be somewhat familiar with.

We might also need an option to toggle whether to show the contents of a library by default or not. I am a little worried that showing the contents of a library by default may make the tree harder to browse. One option would be to switch to a master-details style view where we show the list of libraries separate from the "structure" view that shows the contents of the selected library but I don't have a strong opinion on that.

kenzieschmoll · 2021-08-10T18:44:34Z

+1 to Jacob's last comment regarding splitting this view up into two views: file explorer and structure.

packages/devtools_app/devtools_app.iml

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

packages/devtools_app/lib/src/debugger/debugger_model.dart

packages/devtools_app/lib/src/framework/framework_core.dart

packages/devtools_app/lib/src/debugger/file_search.dart

packages/devtools_app/lib/src/theme.dart

packages/devtools_app/lib/src/trees.dart

kenzieschmoll · 2021-09-22T17:24:48Z

packages/devtools_app/lib/src/trees.dart

@@ -43,6 +43,8 @@ abstract class TreeNode<T extends TreeNode<T>> {

  bool get isRoot => parent == null;

+  bool get isSelected => false;


Should this getter be on the subclass instead of in the TreeNode base class? seems odd to have a getter return a set bool value when there aren't also methods in this TreeNode class to modify the state of isSelected

I think this needs to be here since _TreeViewItemState uses this to determine whether or not to highlight a given TreeNode item.

if that is the case, then we should move the logic for managing the state of isSelected into this class as well. Subclasses can always override and call super if they need to do additional work on changing selection, in which case we'd want to annotate with @mustCallSuper

packages/devtools_app/test/debugger_screen_test.dart

packages/devtools_app/lib/src/debugger/program_explorer.dart

kenzieschmoll

Done with review

packages/devtools_app/lib/src/debugger/program_explorer_controller.dart

packages/devtools_app/lib/src/tree.dart

packages/devtools_app/lib/src/debugger/program_explorer_model.dart

packages/devtools_app/lib/src/debugger/program_explorer.dart

packages/devtools_app/lib/src/performance/legacy/performance_model.dart

kenzieschmoll · 2021-09-24T15:45:57Z

packages/devtools_app/lib/src/tree.dart

    setExpanded(widget.data.isExpanded);
  }

  void _onSelected() {
-    widget.onItemSelected(widget.data);
+    widget?.onItemSelected(widget.data);
    setExpanded(widget.data.isExpanded);


should we be calling onItemExpanded here as well?

No, since onItemExpanded corresponds to clicking the >, whereas onItemSelected corresponds to clicking on any other part of the list item.

kenzieschmoll

a few more comments then LGTM

…een` (flutter#3227)" and associated follow-on PRs

bkonyi added 6 commits July 23, 2021 08:16

Program explorer rough work

e837049

More rough work

8feef51

Cleanup, ready for initial review

b5b7c90

Merge branch 'master' of https://github.com/flutter/devtools into pro…

d2c2974

…gram_explorer

Fix merge conflict and revert unnecessary changes

14b8fd6

General cleanup

c85e987

bkonyi requested review from kenzieschmoll and jacob314 July 29, 2021 21:41

kenzieschmoll reviewed Aug 3, 2021

View reviewed changes

jacob314 reviewed Aug 9, 2021

View reviewed changes

Program explorer rough work

101c302

Fix inspector_tree_test

652d3b2