Fixes broken program explorer on hot restart #3527
Conversation
elliette
changed the title
| _initialized.value = true; | ||
| } | ||
|
|
||
| void initListeners() { | ||
| addAutoDisposeListener( |
There was a problem hiding this comment.
This was removed because it was redundant - the debugger_controller was also listening for serviceManager.isolateManager.selectedIsolate code link, and was re-populating the scripts when the isolate changed. Therefore, what was happening was:
- isolate changes -> refresh
- sorted scripts changes -> refresh
Since we only want to refresh once we have the scripts, this first listener could be removed.
| @@ -63,10 +63,8 @@ class _TreeViewState<T extends TreeNode<T>> extends State<TreeView<T>> | |||
| @override | |||
| void didUpdateWidget(TreeView<T> oldWidget) { | |||
| super.didUpdateWidget(oldWidget); | |||
| if (widget.dataRoots != oldWidget.dataRoots) { | |||
There was a problem hiding this comment.
I don't think this check was ever true. It seems like the oldWidget.dataRoots were always identical to the widget.dataRoots, regardless of whether the dataRoots had changed or not. This meant the tree view wasn't being re-drawn on refresh.
There was a problem hiding this comment.
I looked into this for the uses of TreeView throughout devtools. I think we should change TreeView to accept dataRootsListenable instead of dataRoots. Then all the ValueListenableBuilders around TreeView could be removed, and TreeView would be responsible for listening to changes in dataRootsListenable and calling setState. The one place where we aren't using a List<T> listenable for the tree view is ExpandableVariable (where the listenable has type T not a List<T>). For this case you can just pass in the raw variable to ExpandableVariable and use a FixedValueListenable with this value for the TreeView
There was a problem hiding this comment.
Also as a cleanup as part of this change, looks like the VariablesList class can be deleted
There was a problem hiding this comment.
Done, thanks! Also deleted VariablesList
| @@ -130,6 +126,7 @@ class ProgramExplorerController extends DisposableController | |||
|
|
|||
| /// Marks [node] as the currently selected node, clearing the selection state | |||
| /// of any currently selected node. | |||
| /// TODO(elliette): Scroll to node when selected. | |||
There was a problem hiding this comment.
That looks like it implements scrolling within the file, this comment is for scrolling within the program explorer tree view - eg: user opens some_file.dart at the bottom of the tree view, and then does a hot-restart. On restart, main.dart is selected and opened, but the tree is still scrolled to some_file.dart. I've added more details to the comment to make it clearer.
There was a problem hiding this comment.
got it. On another note, after a hot restart, should the selected and opened file be some_file.dart if it is still available instead of main.dart?
There was a problem hiding this comment.
Hmm good point - added a TODO to see if that's possible
| /// to calling .clear() then .addAll(), because it only notifies listeners | ||
| /// once. | ||
| void replaceAll(Iterable<T> elements) { | ||
| _rawList.clear(); |
There was a problem hiding this comment.
let's use _rawList = <T>[]; here to be consistent with how the clear method clears out the list
| // TODO(kenz): preserve expanded state of tree on switching frames and | ||
| // on stepping. | ||
| return TreeView<DartObjectNode>( | ||
| dataRootsListenable: ValueNotifier([variable]), |
There was a problem hiding this comment.
Use FixedValueListenable here instead
| @@ -56,21 +57,30 @@ class _TreeViewState<T extends TreeNode<T>> extends State<TreeView<T>> | |||
| @override | |||
| void initState() { | |||
| super.initState(); | |||
| _initData(); | |||
| _updateItems(); | |||
| widget.dataRootsListenable.addListener(_updateTreeView); | |||
There was a problem hiding this comment.
can you use addAutoDisposeListener instead?
| } | ||
|
|
||
| // @override |
| void _initData() { | ||
| dataRoots = List.from(widget.dataRoots); | ||
| dataRoots = List.from(widget.dataRootsListenable.value); |
There was a problem hiding this comment.
just put this directly in _updateTreeView since _initData is not used anywhere else
| return ValueListenableBuilder<List<DartObjectNode>>( | ||
| valueListenable: controller.variables, | ||
| builder: (context, variables, _) { | ||
| if (variables.isEmpty) return const SizedBox(); |
There was a problem hiding this comment.
we can actually remove the ValueListenableBuilder and the special case for variables.isEmpty. TreeView returns a SizedBox in its build method when there is an empty data set
| builder: (context, nodes, _) { | ||
| if (nodes == null || nodes.isEmpty) { | ||
| return const Center( | ||
| child: Text('Nothing to inspect'), |
There was a problem hiding this comment.
Let's add an optional parameter to TreeView, emptyTreeDisplayBuilder or some better name if you can think of one that defaults to building const SizedBox(). Then use this value in the TreeView build method when the data is empty. I just took a look at the code, and I don't think controller.outlineNodes.value will ever be null, so we shouldn't have to worry about the null case.
Then you can remove the ValueListenableBuilder here as well. As is, there are two listeners for controller.outlineNodes when there only needs to be one (in TreeView)
There was a problem hiding this comment.
Done, added optional parameter
Fixes #3521