Improve initial selection in the call tree - try 2

src/components/calltree/CallTree.js

@@ -20,6 +20,7 @@ import {
   getScrollToSelectionGeneration,
   getFocusCallTreeGeneration,
   getPreviewSelection,
+  getCategories,
 } from 'firefox-profiler/selectors/profile';
 import { selectedThreadSelectors } from 'firefox-profiler/selectors/per-thread';
 import {
@@ -37,6 +38,7 @@ import type {
   ImplementationFilter,
   ThreadsKey,
   CallNodeInfo,
+  CategoryList,
   IndexIntoCallNodeTable,
   CallNodeDisplayData,
   WeightType,
@@ -54,6 +56,7 @@ type StateProps = {|
   +focusCallTreeGeneration: number,
   +tree: CallTreeType,
   +callNodeInfo: CallNodeInfo,
+  +categories: CategoryList,
   +selectedCallNodeIndex: IndexIntoCallNodeTable | null,
   +rightClickedCallNodeIndex: IndexIntoCallNodeTable | null,
   +expandedCallNodeIndexes: Array<IndexIntoCallNodeTable | null>,
@@ -144,27 +147,29 @@ class CallTreeImpl extends PureComponent<Props> {
 
   componentDidMount() {
     this.focus();
-    if (this.props.selectedCallNodeIndex === null) {
-      this.procureInterestingInitialSelection();
-    } else if (this._treeView) {
+    this.maybeProcureInterestingInitialSelection();
+
+    if (this.props.selectedCallNodeIndex === null && this._treeView) {
       this._treeView.scrollSelectionIntoView();
     }
   }
 
   componentDidUpdate(prevProps) {
     if (
-      this.props.scrollToSelectionGeneration >
-      prevProps.scrollToSelectionGeneration
+      this.props.focusCallTreeGeneration > prevProps.focusCallTreeGeneration
     ) {
-      if (this._treeView) {
-        this._treeView.scrollSelectionIntoView();
-      }
+      this.focus();
     }
 
+    this.maybeProcureInterestingInitialSelection();
+
     if (
-      this.props.focusCallTreeGeneration > prevProps.focusCallTreeGeneration
+      this.props.selectedCallNodeIndex !== null &&
+      this.props.scrollToSelectionGeneration >
+        prevProps.scrollToSelectionGeneration &&
+      this._treeView
     ) {
-      this.focus();
+      this._treeView.scrollSelectionIntoView();
     }
   }
 
@@ -228,35 +233,77 @@ class CallTreeImpl extends PureComponent<Props> {
     openSourceView(file, 'calltree');
   };
 
-  procureInterestingInitialSelection() {
+  maybeProcureInterestingInitialSelection() {
     // Expand the heaviest callstack up to a certain depth and select the frame
     // at that depth.
-    const { tree, expandedCallNodeIndexes } = this.props;
-    const newExpandedCallNodeIndexes = expandedCallNodeIndexes.slice();
-    const maxInterestingDepth = 17; // scientifically determined
-    let currentCallNodeIndex = tree.getRoots()[0];
-    if (currentCallNodeIndex === undefined) {
-      // This tree is empty.
+    const {
+      tree,
+      expandedCallNodeIndexes,
+      selectedCallNodeIndex,
+      callNodeInfo: { callNodeTable },
+      categories,
+    } = this.props;
+
+    if (selectedCallNodeIndex !== null || expandedCallNodeIndexes.length > 0) {
+      // Let's not change some existing state.
       return;
     }
-    newExpandedCallNodeIndexes.push(currentCallNodeIndex);
-    for (let i = 0; i < maxInterestingDepth; i++) {
-      const children = tree.getChildren(currentCallNodeIndex);
-      if (children.length === 0) {
+
+    const newExpandedCallNodeIndexes = [];
+    // This value is completely arbitrary and looked good on Julien's machine
+    // when this was implemented. In the future we may want to look at the
+    // available space instead.
+    const maxVisibleLines = 70;
+    const minimumDepth = 10;
+
+    const idleCategoryIndex = categories.findIndex(
+      (category) => category.name === 'Idle'
+    );
+
+    let children = tree.getRoots();
+    let nodeToSelect = null;
+    let visibleLinesCount = children.length;
+
+    while (true) {
+      const firstNonIdleNode = children.find(
+        (nodeIndex) => callNodeTable.category[nodeIndex] !== idleCategoryIndex
+      );
+
+      if (firstNonIdleNode === undefined) {
+        break;
+      }
+
+      nodeToSelect = firstNonIdleNode;
+
+      const nodeData = tree.getNodeData(firstNonIdleNode);
+      if (nodeData.self > nodeData.total * 0.95) {
+        // This node doesn't have interesting children, let's stop here.
         break;
       }
-      currentCallNodeIndex = children[0];
-      newExpandedCallNodeIndexes.push(currentCallNodeIndex);
+
+      const depth = tree.getDepth(firstNonIdleNode);
+      children = tree.getChildren(firstNonIdleNode);
+
+      if (
+        depth > minimumDepth &&
+        visibleLinesCount + children.length > maxVisibleLines
+      ) {
+        // Expanding this node would exceed our budget.
+        break;
+      }
+
+      newExpandedCallNodeIndexes.push(firstNonIdleNode);
+      visibleLinesCount += children.length;
+    }
+
+    if (newExpandedCallNodeIndexes.length > 0) {
+      // Take care to not trigger a state change if there's nothing to change,
+      // to avoid infinite render loop.
+      this._onExpandedCallNodesChange(newExpandedCallNodeIndexes);
     }
-    this._onExpandedCallNodesChange(newExpandedCallNodeIndexes);
-
-    const category = tree.getDisplayData(currentCallNodeIndex).categoryName;
-    if (category !== 'Idle') {
-      // If we selected the call node with a "idle" category, we'd have a
-      // completely dimmed activity graph because idle stacks are not drawn in
-      // this graph. Because this isn't probably what the average user wants we
-      // do it only when the category is something different.
-      this._onSelectedCallNodeChange(currentCallNodeIndex);
+
+    if (nodeToSelect !== null) {
+      this._onSelectedCallNodeChange(nodeToSelect);
     }
   }
 
@@ -308,6 +355,7 @@ export const CallTree = explicitConnect<{||}, StateProps, DispatchProps>({
     focusCallTreeGeneration: getFocusCallTreeGeneration(state),
     tree: selectedThreadSelectors.getCallTree(state),
     callNodeInfo: selectedThreadSelectors.getCallNodeInfo(state),
+    categories: getCategories(state),
     selectedCallNodeIndex:
       selectedThreadSelectors.getSelectedCallNodeIndex(state),
     rightClickedCallNodeIndex:

src/test/components/ProfileCallTreeView.test.js

@@ -283,14 +283,14 @@ describe('calltree/ProfileCallTreeView', function () {
     expect(getRowElement('C')).toHaveClass('isSelected');
   });
 
-  it('does not select the heaviest stack if it is idle', () => {
+  it('does select a non-idle stack if the heaviest stack is idle', () => {
     const { profile } = getProfileFromTextSamples(`
-      A  A            A            A            A
-      B  C[cat:Idle]  C[cat:Idle]  C[cat:Idle]  D
-      E                                         E
+      A  A            A            A            A  A
+      B  C[cat:Idle]  C[cat:Idle]  C[cat:Idle]  B  D
+      E                                         E  F
     `);
-    const { container } = setup(profile);
-    expect(container.querySelector('.treeViewRow.isSelected')).toBeFalsy();
+    const { getRowElement } = setup(profile);
+    expect(getRowElement('E')).toHaveClass('isSelected');
   });
 });