Inspector widget selection improvements (#3489) #3525

aednlaxer · 2021-12-07T10:28:43Z

Original ticket: #3489

Changes in this pull request:

add widget search with text selection
rename "Details Tree" to "Widget Details Tree"
add breadcrumbs navigator to Widget Details Tree

…#3489) * Add a method for finding a path from currently selected row item to the tree root (flutter#3489) * Add InspectorBreadcrumbNavigator (flutter#3489) * Set text style explicitly (flutter#3489) * Notify controller about selected widget on tap (flutter#3489) * Make currently selected item not clickable (flutter#3489) * Move breadcrumbs out of scrollable tree container (flutter#3489) * Tweak padding size and appearance parameters (flutter#3489) * Scroll to the end of the list so the last breadcrumb is always visible (flutter#3489) * Simplify constructor (flutter#3489) * Provide inspector tree controller directly to the tree widget where needed (flutter#3489) * Provide dummy inspectorTreeController to tests in inspector_tree_test (flutter#3489) * Ensure inspectorTreeController is provided when InspectorTree is showing a widget detail tree (flutter#3489) * Test if breadcrumbs widget is displayed (flutter#3489) * Remove empty constructor (flutter#3489) * Use row for showing breadcrumbs (flutter#3489)

Disable Details tree search & Highlight search matches with orange color & breadcrumbs

…instance_viewer_test.dart (flutter#3489)

kenzieschmoll

Done with first round of comments. Generally, we should be trying to use the search functionality in SearchControllerMixin where possible instead of trying to manage matches and match indexes manually. Happy to discuss further or chat offline for help with hooking the inspector up to the existing search functionality in DevTools.

packages/devtools_app/lib/src/common_widgets.dart

kenzieschmoll · 2021-12-07T16:51:49Z

packages/devtools_app/lib/src/inspector/diagnostics.dart

@@ -35,18 +35,24 @@ class DiagnosticsNodeDescription extends StatelessWidget {
  const DiagnosticsNodeDescription(


cc @matt-ragonese in case you are using this file for your custom screen

packages/devtools_app/lib/src/inspector/diagnostics.dart

kenzieschmoll · 2021-12-07T19:28:54Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

+    // Reset search matches
+    for (final row in cachedRows) {
+      _statsWidgets++;
+      setSearchMatch(row.node, false);


we shouldn't need to do this. SearchControllerMixin already resets the isSearchMatch field in _updateMatches

setSearchMatch does two things: it expands current node and it marks a node (not row) as search match.
InspectorTree uses getRow() to get row data which is different from what's returned from getCachedRow(). _updateMatches() marks a row as search match but this row is a cached row so isSearchMatch is not reflecting the actual status for rows returned by getRow().

I have the following questions regarding this:

can cached rows be used in InspectorTree?

what would be a good way to expand a node if setSearchMatch() is removed?

A couple questsions:

Why do we need to mark both the row and the node as having a search match? as far as I can tell, everywhere hasSearchMatch is used, we have access to the row, so we could just write row.isSearchMatch instead.

Why are we expanding nodes at all for search?

If I understand correctly, the issue with cached rows vs non-cached rows is that we are setting isSearchMatch on cached rows and not on rows returned by getRow() - is that correct? If so, can we do a comparison where ever we need to call getRow() and need to know the value of isSearchMatch to check if there is a matching cached row with isSearchMatch set to true?

Why do we need to mark both the row and the node as having a search match?

It was made as a work around because isSearchMatch is set on cached rows but different instances of rows are used when displaying the tree.

Why are we expanding nodes at all for search?

Search is looking for matches in cachedRows. In order to have all rows in cachedRows list all nodes need to be expanded.

If I understand correctly, the issue with cached rows vs non-cached rows is that we are setting isSearchMatch on cached rows and not on rows returned by getRow() - is that correct?

Yes, that's correct.

can we do a comparison where ever we need to call getRow() and need to know the value of isSearchMatch

Yes. We need to know the value of isSearchMatch in _InspectorTreeState but getRows() is used there.

I've made some changes to the code:

setSearchMatch() and hasSearchMatch have been removed

InspectorTree now uses getCachedRow(). It should be okay to use it since it may still call root?.getRow() under the hood.

There's still need to expand the tree to get all rows cached. I did this by expanding all nodes of the tree in the overridden set search setter (so nodes are expanded before submitting search query). I don't think it's ideal, maybe it's better to find a solution that keeps current state of collapsed and expanded nodes.

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

kenzieschmoll · 2021-12-07T19:30:57Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

+        _statsSearchOps++;
+        if (searchValue.toLowerCase().contains(caseInsensitiveSearch)) {
+          matches.add(row);
+          setSearchMatch(row.node, true);


same comment as above. SearchControllerMixin._updateMatches should already handle this

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

…matches (flutter#3489)

…3489)

… padding (flutter#3489)

…utter#3489)

packages/devtools_app/lib/src/inspector/inspector_breadcrumbs.dart

packages/devtools_app/lib/src/ui/search.dart

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

kenzieschmoll · 2021-12-14T18:16:49Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

+    final pathToRoot = <InspectorTreeRow>[selectedItem];
+    InspectorTreeNode nextParentNode = selectedItem.node.parent;
+    while (nextParentNode != null) {
+      final rowItem = cachedRows.firstWhere(


right, but then wouldn't the path to root be incomplete, meaning the breadcrumb navigator would be incomplete and not show the root node?

kenzieschmoll · 2021-12-14T18:43:46Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

+    // Reset search matches
+    for (final row in cachedRows) {
+      _statsWidgets++;
+      setSearchMatch(row.node, false);


A couple questsions:

Why do we need to mark both the row and the node as having a search match? as far as I can tell, everywhere hasSearchMatch is used, we have access to the row, so we could just write row.isSearchMatch instead.

Why are we expanding nodes at all for search?

If I understand correctly, the issue with cached rows vs non-cached rows is that we are setting isSearchMatch on cached rows and not on rows returned by getRow() - is that correct? If so, can we do a comparison where ever we need to call getRow() and need to know the value of isSearchMatch to check if there is a matching cached row with isSearchMatch set to true?

kenzieschmoll · 2021-12-15T18:22:58Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

+  @override
+  set search(String value) {
+    // Expand tree so all rows are available for search
+    _expandAll(root);


instead of overriding search to expand the tree. Why don't we create a copy of the tree at the time of tree creation. This copy can be fully expanded from the start, which has the advantages that A) we won't have to modify the expand/collapse state of the original tree, and B) we won't have to pay the cost of expanding the tree each time we perform a search.

Having a copy leads to a previous issue when actual\cached rows and rows from the copy are not the same instances so isSearchMatch doesn't reflect the actual value. Anyway I tried using a copy of the tree for search and finding a path to the root for breadcrumbs. isSearchMatch was made to show the actual value because getCachedRow() sets the value to the cached row before returning it.

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

kenzieschmoll · 2021-12-16T16:44:54Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

-      final rowItem = cachedRows.firstWhere(
-        (element) => element.node == nextParentNode,
-        orElse: null,
+      final rowItem = _searchableCachedRows.firstWhere(


have you tried this with a very large app? The worst case scenario of this is that you are trying to get the path from the deepest leaf node to the root. Where N is the depth of the leaf and M is the total number of nodes in the tree, this could be NxM, which could be very slow for a large app.

seems like it would be more performant to either A) make rows aware of their parent rows so that row.parent would yield the next parent, or B) to give Node's access to their associated row, so that row.node.parent.row would yield the next parent.

Turns out it's enough to have an instance of InspectorTreeNode for the selected row to find a way to the root. Such instance is available from an instance of InspectorTreeRow and _cachedSelectedRow in particular. This method now uses parent of the selected row.node to find a path to the root. Should be more performant now as it doesn't iterate through the whole tree to find the right item on every iteration of the while loop.

kenzieschmoll · 2021-12-16T16:47:43Z

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

@@ -184,6 +185,10 @@ class InspectorTreeController extends Object
  final List<InspectorTreeRow> cachedRows = [];
  InspectorTreeRow _cachedSelectedRow;

+  /// Cached rows of the tree. Similar to [cachedRows] but items are populated


dart-doc nit: there should be a blank new line between the first sentence and the rest.

Also just to clarify, _searchableCachedRows contains every row in the tree, is that correct? not just those that are visible (nodes with expanded parents)?

Yes, it contains every row in the tree (including collapsed nodes, children of collapsed nodes), it's now reflected in the doc

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

…ify path to the root calculation (flutter#3489)

# Conflicts: # packages/devtools_app/lib/src/common_widgets.dart # packages/devtools_app/lib/src/network/network_screen.dart # packages/devtools_app/test/goldens/timeline_flame_chart_with_selected_frame.png

packages/devtools_app/lib/src/inspector/inspector_breadcrumbs.dart

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart

aednlaxer · 2021-12-21T11:42:05Z

I'm having difficulties figuring out why goldens tests fail. I'm unable to reproduce it or update golden files locally when using Flutter master or stable. As far as I understand, devtools uses specific version of SDK in CI/CD (2.8.0-3.1.pre currently) but using same version locally didn't help. @kenzieschmoll do you have any idea what could be causing these tests to fail?

…ms (flutter#3489)

…3489)

aednlaxer · 2021-12-23T10:11:49Z

The goldens and other tests seem to be okay now

kenzieschmoll · 2022-01-04T21:54:02Z

I just checked this out locally and played with it against a large application to verify the performance is acceptable. It looks fantastic - great work!

Here is one polish item that should be done as a follow up: #3561

aednlaxer · 2022-01-05T07:38:17Z

Thanks for thoroughly reviewing this PR! I'll have a look at the issue

msavela and others added 19 commits November 18, 2021 15:45

Improve widget selection in the inspector (Related ticket: flutter#3489)

5009433

Removed details tree search (flutter#3489)

0607595

Highlight search matches with orange color (flutter#3489)

4203f88

Changed the orange color of search highlight (flutter#3489)

c5e15bd

Highlight preview only instead of the whole item (flutter#3489)

75b3046

Use different text background colors depending on whether the node is…

8808813

… selected (flutter#3489)

Highlight searchValue in text preview (flutter#3489)

f6bf52e

Reformat code (flutter#3489)

3b72790

Use colors from ui/colors.dart for search matches (flutter#3489)

a8dd2d9

Find widget by tooltip instead of text in inspector_screen_test (flut…

2ee4c21

…ter#3489)

Merge pull request #26 from CodemateLtd/widget-search-alex

b752bca

Disable Details tree search & Highlight search matches with orange color & breadcrumbs

Fix inspector_integration_test.dart (flutter#3489)

580ea85

Update goldens (flutter#3489)

ec063b1

Remove unnecessary import (flutter#3489)

17e117d

Revert goldens update (flutter#3489)

3b79477

Put FakeServiceManager to the global storage before running tests in …

87c6fae

…instance_viewer_test.dart (flutter#3489)

Mock searchInProgressNotifier (flutter#3489)

8df337a

Update goldens (flutter#3489)

cc61b2d

kenzieschmoll reviewed Dec 7, 2021

View reviewed changes

aednlaxer added 10 commits December 8, 2021 15:22

Fix several nits (flutter#3489)

ff60690

Use regular text style when searchValue is null, empty or there's no …

7719e8f

…matches (flutter#3489)

Add extra null checks to isChevron and isEllipsis (flutter#3489)

9ca2161

Use Container's alignment instead of Align (flutter#3489)

7207269

Remove SizedBox (flutter#3489)

774a89d

Add constant for the max number of breadcrumbs (flutter#3489)

6903577

Move search controls to InspectorSummaryTreeControls (flutter#3489)

ae76768

Add TODO comment regarding other values of SearchTargetType (flutter#…

1e3f6c9

…3489)

Add named constructor to PaddedDivider for setting symmetric vertical…

162dce7

… padding (flutter#3489)

Add SmallCircularProgressIndicator and reuse it (flutter#3489)

b14178f

aednlaxer added 2 commits December 13, 2021 13:45

Search previous matches in widget search when asked to (flutter#3489)

d72570a

Perform a global search when search inside previous matches fails (fl…

337bab1

…utter#3489)

kenzieschmoll reviewed Dec 14, 2021

View reviewed changes

aednlaxer added 3 commits December 15, 2021 10:01

Fix nits (flutter#3489)

2c42d9f

Remove setSearchMatch function (flutter#3489)

5374d37

Expand every node of the tree before search (flutter#3489)

6ab7258

kenzieschmoll reviewed Dec 15, 2021

View reviewed changes

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart Outdated Show resolved Hide resolved

Add a list of all cached tree rows for search (flutter#3489)

a9ac6b6

kenzieschmoll reviewed Dec 16, 2021

View reviewed changes

aednlaxer added 3 commits December 17, 2021 11:44

Use tree nodes instead of rows in InspectorBreadcrumbNavigator. Simpl…

6347a52

…ify path to the root calculation (flutter#3489)

Fix nits (flutter#3489)

eafc8bf

Merge branch 'master' into widget-selection-search

20b2d60

# Conflicts: # packages/devtools_app/lib/src/common_widgets.dart # packages/devtools_app/lib/src/network/network_screen.dart # packages/devtools_app/test/goldens/timeline_flame_chart_with_selected_frame.png

kenzieschmoll reviewed Dec 20, 2021

View reviewed changes

packages/devtools_app/lib/src/inspector/inspector_breadcrumbs.dart Outdated Show resolved Hide resolved

packages/devtools_app/lib/src/inspector/inspector_tree_controller.dart Outdated Show resolved Hide resolved

aednlaxer added 3 commits December 20, 2021 19:14

Optimize usage of variables (flutter#3489)

74a32a3

Update goldens (flutter#3489)

7af5098

Merge branch 'master' into widget-selection-search

72c9b5e

aednlaxer added 2 commits December 21, 2021 16:00

Update matchIndex when activeMatchIndex is greater than number of ite…

1937d24

…ms (flutter#3489)

Set searchInProgress to false before calling _updateMatches (flutter#…

90922d0

…3489)

aednlaxer mentioned this pull request Dec 22, 2021

Experimental: widget detail search (#3489) #3552

Closed

aednlaxer added 3 commits December 23, 2021 10:41

Update memory_* goldens (flutter#3489)

1506ea9

Update memory_* goldens (flutter#3489)

001cdd4

Update memory_* goldens (flutter#3489)

035082f

kenzieschmoll approved these changes Jan 4, 2022

View reviewed changes

kenzieschmoll merged commit 8724666 into flutter:master Jan 4, 2022

kenzieschmoll mentioned this pull request Feb 15, 2022

No way to Find a tree element in the inspector page #1841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inspector widget selection improvements (#3489) #3525

Inspector widget selection improvements (#3489) #3525

aednlaxer commented Dec 7, 2021 •

edited

kenzieschmoll left a comment

kenzieschmoll Dec 7, 2021

kenzieschmoll Dec 7, 2021

aednlaxer Dec 13, 2021

kenzieschmoll Dec 14, 2021

aednlaxer Dec 15, 2021

kenzieschmoll Dec 7, 2021

kenzieschmoll Dec 14, 2021

kenzieschmoll Dec 14, 2021

kenzieschmoll Dec 15, 2021

aednlaxer Dec 16, 2021

kenzieschmoll Dec 16, 2021

aednlaxer Dec 17, 2021

kenzieschmoll Dec 16, 2021

aednlaxer Dec 17, 2021

aednlaxer commented Dec 21, 2021 •

edited

aednlaxer commented Dec 23, 2021

kenzieschmoll commented Jan 4, 2022

aednlaxer commented Jan 5, 2022

		@@ -35,18 +35,24 @@ class DiagnosticsNodeDescription extends StatelessWidget {
		const DiagnosticsNodeDescription(

Inspector widget selection improvements (#3489) #3525

Inspector widget selection improvements (#3489) #3525

Conversation

aednlaxer commented Dec 7, 2021 • edited

kenzieschmoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aednlaxer commented Dec 21, 2021 • edited

aednlaxer commented Dec 23, 2021

kenzieschmoll commented Jan 4, 2022

aednlaxer commented Jan 5, 2022

aednlaxer commented Dec 7, 2021 •

edited

aednlaxer commented Dec 21, 2021 •

edited