From fb6f049501a838ecac818c9555d18b43f078471b Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 24 Aug 2022 13:54:44 +0200 Subject: [PATCH 1/8] Implement sort arrows for MarkerTable --- src/components/marker-table/index.js | 43 +++- src/components/shared/TreeView.css | 13 ++ src/components/shared/TreeView.js | 208 +++++++++++++++--- .../__snapshots__/MarkerTable.test.js.snap | 9 +- .../ProfileCallTreeView.test.js.snap | 168 +++++++++----- 5 files changed, 349 insertions(+), 92 deletions(-) diff --git a/src/components/marker-table/index.js b/src/components/marker-table/index.js index 1f85812064..7032a22c5a 100644 --- a/src/components/marker-table/index.js +++ b/src/components/marker-table/index.js @@ -8,7 +8,7 @@ import React, { PureComponent } from 'react'; import memoize from 'memoize-immutable'; import explicitConnect from '../../utils/connect'; -import { TreeView } from '../shared/TreeView'; +import { TreeView, ColumnSortState } from '../shared/TreeView'; import { MarkerTableEmptyReasons } from './MarkerTableEmptyReasons'; import { getZeroAt, @@ -42,7 +42,9 @@ const MAX_DESCRIPTION_CHARACTERS = 500; type MarkerDisplayData = {| start: string, + rawStart: Milliseconds, duration: string | null, + rawDuration: Milliseconds | null, name: string, type: string, |}; @@ -113,10 +115,13 @@ class MarkerTree { } let duration = null; + let rawDuration: number | null = null; if (marker.incomplete) { duration = 'unknown'; } else if (marker.end !== null) { duration = formatTimestamp(marker.end - marker.start); + // $FlowFixMe + rawDuration = marker.end - marker.start; } displayData = { @@ -128,6 +133,8 @@ class MarkerTree { marker.name, marker.data ), + rawDuration: rawDuration, + rawStart: marker.start, }; this._displayDataByIndex.set(markerIndex, displayData); } @@ -169,6 +176,7 @@ class MarkerTableImpl extends PureComponent { _onExpandedNodeIdsChange = () => {}; _treeView: ?TreeView; _takeTreeViewRef = (treeView) => (this._treeView = treeView); + _sortedColumns = new ColumnSortState([]); getMarkerTree = memoize((...args) => new MarkerTree(...args), { limit: 1 }); @@ -204,6 +212,33 @@ class MarkerTableImpl extends PureComponent { changeRightClickedMarker(threadsKey, selectedMarker); }; + _compareColumn = ( + first: MarkerDisplayData, + second: MarkerDisplayData, + column: number + ) => { + switch (column) { + case 1: + return second.rawStart - first.rawStart; + case 2: + if (first.rawDuration === null) { + return -1; + } + if (second.rawDuration === null) { + return 1; + } + return second.rawDuration - first.rawDuration; + case 3: + return first.type.localeCompare(second.type); + default: + throw new Error('Invalid column'); + } + }; + + _onSort = (sortedColumns: ColumnSortState) => { + this._sortedColumns = sortedColumns; + }; + render() { const { getMarker, @@ -214,7 +249,7 @@ class MarkerTableImpl extends PureComponent { markerSchemaByName, getMarkerLabel, } = this.props; - const tree = this.getMarkerTree( + const tree: MarkerTree = this.getMarkerTree( getMarker, markerIndexes, zeroAt, @@ -247,6 +282,10 @@ class MarkerTableImpl extends PureComponent { contextMenuId="MarkerContextMenu" rowHeight={16} indentWidth={10} + initialSortedColumns={this._sortedColumns} + onSort={this._onSort} + compareColumn={this._compareColumn} + sortableColumns={new Set([1, 2, 3])} /> )} diff --git a/src/components/shared/TreeView.css b/src/components/shared/TreeView.css index dda999b6ad..68e900b75b 100644 --- a/src/components/shared/TreeView.css +++ b/src/components/shared/TreeView.css @@ -102,6 +102,19 @@ border-right: 1px solid var(--grey-30); } +.treeViewHeaderColumn.sortInactive::after { + content: " ▲"; + opacity: 0; +} + +.treeViewHeaderColumn.sortDescending::after { + content: ' ▲'; +} + +.treeViewHeaderColumn.sortAscending::after { + content: ' ▼ '; +} + .treeBadge { display: inline-block; overflow: hidden; diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index fa0c0a03ce..7fe1297fa8 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -56,37 +56,98 @@ export type Column = {| type TreeViewHeaderProps = {| +fixedColumns: Column[], +mainColumn: Column, + +onSort: (number) => void, + +columnSortDirections: Array | null, |}; -const TreeViewHeader = ({ - fixedColumns, - mainColumn, -}: TreeViewHeaderProps) => { - if (fixedColumns.length === 0 && !mainColumn.titleL10nId) { - // If there is nothing to display in the header, do not render it. - return null; +// columns start at 1 +export class ColumnSortState { + sortedColumns: number[]; + + // -column: sort descending, +column: sort ascending, start by sorting last column + constructor(sortedColumns: number[]) { + this.sortedColumns = sortedColumns; + } + + _sortColumn(column: number) { + const sortedColumns = this.sortedColumns.filter( + (c) => c !== column && c !== -column + ); + sortedColumns.push(column); + return new ColumnSortState(sortedColumns); + } + + push(column: number) { + return this._sortColumn(-this._sortState(column)); + } + + _sortState(column: number) { + return this.sortedColumns + .filter((c) => c === column || c === -column) + .concat(-column)[0]; } - return ( -
- {fixedColumns.map((col) => ( + + getSortStateArr(sortableColumns: Set): Array { + const arr = new Array(Math.max(...sortableColumns) + 1).fill(null); + for (const column of sortableColumns) { + arr[column] = this._sortState(column) < 0; + } + return arr; + } +} + +class TreeViewHeader extends React.PureComponent< + TreeViewHeaderProps +> { + _onSort = (e: Event) => { + const { onSort } = this.props; + // $FlowExpectError + onSort(Number(e.currentTarget.dataset.column)); + }; + + render() { + const { fixedColumns, mainColumn, columnSortDirections } = this.props; + if (fixedColumns.length === 0 && !mainColumn.titleL10nId) { + // If there is nothing to display in the header, do not render it. + return null; + } + return ( +
+ {fixedColumns.map((col, i) => { + let sortClass = ''; + if (columnSortDirections) { + if (columnSortDirections[i + 1] === true) { + sortClass = 'sortAscending'; + } else if (columnSortDirections[i + 1] === false) { + sortClass = 'sortDescending'; + } + } + return ( + + + + ); + })} - ))} - - - -
- ); -}; +
+ ); + } +} function reactStringWithHighlightedSubstrings( string: string, @@ -393,13 +454,31 @@ type TreeViewProps = {| +rowHeight: CssPixels, +indentWidth: CssPixels, +onKeyDown?: (SyntheticKeyboardEvent<>) => void, + // number: column number, -1: first larger, 0: same, 1: first smaller + +compareColumn?: (DisplayData, DisplayData, number) => number, + +initialSortedColumns?: ColumnSortState, + +onSort?: (ColumnSortState) => void, + +sortableColumns?: Set, +|}; + +type TreeViewState = {| + sortedColumns: ColumnSortState, |}; export class TreeView extends React.PureComponent< - TreeViewProps + TreeViewProps, + TreeViewState > { _list: VirtualList | null = null; _takeListRef = (list: VirtualList | null) => (this._list = list); + state = { sortedColumns: new ColumnSortState([]) }; + + constructor(props: TreeViewProps) { + super(props); + if (props.initialSortedColumns) { + this.state.sortedColumns = props.initialSortedColumns; + } + } // The tuple `specialItems` always contains 2 elements: the first element is // the selected node id (if any), and the second element is the right clicked @@ -422,19 +501,51 @@ export class TreeView extends React.PureComponent< ); _computeAllVisibleRowsMemoized = memoize( - (tree: Tree, expandedNodes: Set) => { + ( + tree: Tree, + expandedNodes: Set, + sortedColumns: ColumnSortState, + compareColumn: ?(DisplayData, DisplayData, number) => number + ) => { + function sortNodes(nodeIds: Array): Array { + if (!compareColumn) { + return nodeIds; + } + let sortedNodeIds = nodeIds; + for (let i = 0; i < sortedColumns.sortedColumns.length; i++) { + const column = sortedColumns.sortedColumns[i]; + const absColumn = Math.abs(column); + const sign = column < 0 ? -1 : 1; + sortedNodeIds = sortedNodeIds.sort((a, b) => { + return ( + sign * + ((compareColumn: any): ( + DisplayData, + DisplayData, + number + ) => number)( + tree.getDisplayData(a), + tree.getDisplayData(b), + absColumn + ) + ); + }); + } + return sortedNodeIds; + } + function _addVisibleRowsFromNode(tree, expandedNodes, arr, nodeId) { arr.push(nodeId); if (!expandedNodes.has(nodeId)) { return; } - const children = tree.getChildren(nodeId); + const children = sortNodes(tree.getChildren(nodeId)); for (let i = 0; i < children.length; i++) { _addVisibleRowsFromNode(tree, expandedNodes, arr, children[i]); } } - const roots = tree.getRoots(); + const roots = sortNodes(tree.getRoots()); const allRows = []; for (let i = 0; i < roots.length; i++) { _addVisibleRowsFromNode(tree, expandedNodes, allRows, roots[i]); @@ -462,7 +573,6 @@ export class TreeView extends React.PureComponent< _renderRow = (nodeId: NodeIndex, index: number, columnIndex: number) => { const { - tree, fixedColumns, mainColumn, appendageColumn, @@ -472,6 +582,7 @@ export class TreeView extends React.PureComponent< rowHeight, indentWidth, } = this.props; + const { tree } = this.props; const displayData = tree.getDisplayData(nodeId); // React converts height into 'px' values, while lineHeight is valid in // non-'px' units. @@ -522,8 +633,13 @@ export class TreeView extends React.PureComponent< } _getAllVisibleRows(): NodeIndex[] { - const { tree } = this.props; - return this._computeAllVisibleRowsMemoized(tree, this._getExpandedNodes()); + const { tree, compareColumn } = this.props; + return this._computeAllVisibleRowsMemoized( + tree, + this._getExpandedNodes(), + this.state.sortedColumns, + compareColumn + ); } _getSpecialItems(): [NodeIndex | void, NodeIndex | void] { @@ -595,7 +711,8 @@ export class TreeView extends React.PureComponent< _onCopy = (event: ClipboardEvent) => { event.preventDefault(); - const { tree, selectedNodeId, mainColumn } = this.props; + const { selectedNodeId, mainColumn } = this.props; + const { tree } = this.props; if (selectedNodeId) { const displayData = tree.getDisplayData(selectedNodeId); const clipboardData: DataTransfer = (event: any).clipboardData; @@ -736,6 +853,24 @@ export class TreeView extends React.PureComponent< } } + _onSort = (column: number) => { + if ( + !this.props.sortableColumns || + !this.props.sortableColumns.has(column) + ) { + return; + } + this.setState((state) => { + const newSortedColumns = state.sortedColumns.push(column); + if (this.props.onSort) { + this.props.onSort(newSortedColumns); + } + return { + sortedColumns: newSortedColumns, + }; + }); + }; + render() { const { fixedColumns, @@ -746,10 +881,21 @@ export class TreeView extends React.PureComponent< maxNodeDepth, rowHeight, selectedNodeId, + sortableColumns, } = this.props; + const { sortedColumns } = this.state; + const columnSortDirections = + sortableColumns && sortedColumns + ? sortedColumns.getSortStateArr(sortableColumns) + : null; return (
- + Start Duration Type diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index 53f3829e06..60d8c9b3ed 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -149,10 +149,12 @@ exports[`ProfileCallTreeView with JS Allocations matches the snapshot for JS all class="treeViewHeader" > Date: Wed, 24 Aug 2022 17:25:28 +0200 Subject: [PATCH 2/8] Fix linter errors --- src/components/shared/TreeView.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index 7fe1297fa8..6a121b7581 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -99,10 +99,17 @@ export class ColumnSortState { class TreeViewHeader extends React.PureComponent< TreeViewHeaderProps > { - _onSort = (e: Event) => { + _onSort = (e: MouseEvent) => { const { onSort } = this.props; +<<<<<<< HEAD // $FlowExpectError onSort(Number(e.currentTarget.dataset.column)); +======= + const target = e.target; + if (target instanceof HTMLElement) { + onSort(Number(target.getAttribute('data-column'))); + } +>>>>>>> 5f297e49 (Fix linter errors) }; render() { @@ -130,7 +137,7 @@ class TreeViewHeader extends React.PureComponent< > From d913736fa6da393932b8973621cea6bccf081d71 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 24 Aug 2022 18:01:19 +0200 Subject: [PATCH 3/8] Add sorting to Call Tree view and tests --- src/components/calltree/CallTree.js | 29 +- src/components/shared/TreeView.css | 4 +- src/components/shared/TreeView.js | 9 +- src/profile-logic/call-tree.js | 2 + src/test/components/MarkerTable.test.js | 11 + .../components/ProfileCallTreeView.test.js | 11 + .../__snapshots__/MarkerTable.test.js.snap | 1290 ++++++++++- .../ProfileCallTreeView.test.js.snap | 2058 ++++++++++++++++- src/test/store/icons.test.js | 2 + src/test/unit/profile-tree.test.js | 6 + src/types/profile-derived.js | 3 + 11 files changed, 3328 insertions(+), 97 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index cdfbda64eb..84c24d229d 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -6,7 +6,10 @@ import React, { PureComponent } from 'react'; import memoize from 'memoize-immutable'; import explicitConnect from 'firefox-profiler/utils/connect'; -import { TreeView } from 'firefox-profiler/components/shared/TreeView'; +import { + TreeView, + ColumnSortState, +} from 'firefox-profiler/components/shared/TreeView'; import { CallTreeEmptyReasons } from './CallTreeEmptyReasons'; import { Icon } from 'firefox-profiler/components/shared/Icon'; import { getCallNodePathFromIndex } from 'firefox-profiler/profile-logic/profile-data'; @@ -87,6 +90,22 @@ class CallTreeImpl extends PureComponent { }; _treeView: TreeView | null = null; _takeTreeViewRef = (treeView) => (this._treeView = treeView); + _sortedColumns = new ColumnSortState([]); + + _compareColumn = ( + first: CallNodeDisplayData, + second: CallNodeDisplayData, + column: number + ) => { + switch (column) { + case 2: + return second.rawTotal - first.rawTotal; + case 3: + return second.rawSelf - first.rawSelf; + default: + throw new Error('Invalid column'); + } + }; /** * Call Trees can have different types of "weights" for the data. Choose the @@ -260,6 +279,10 @@ class CallTreeImpl extends PureComponent { } } + _onSort = (sortedColumns: ColumnSortState) => { + this._sortedColumns = sortedColumns; + }; + render() { const { tree, @@ -296,6 +319,10 @@ class CallTreeImpl extends PureComponent { onKeyDown={this._onKeyDown} onEnterKey={this._onEnterOrDoubleClick} onDoubleClick={this._onEnterOrDoubleClick} + initialSortedColumns={this._sortedColumns} + onSort={this._onSort} + compareColumn={this._compareColumn} + sortableColumns={new Set([2, 3])} /> ); } diff --git a/src/components/shared/TreeView.css b/src/components/shared/TreeView.css index 68e900b75b..5900fec852 100644 --- a/src/components/shared/TreeView.css +++ b/src/components/shared/TreeView.css @@ -103,7 +103,7 @@ } .treeViewHeaderColumn.sortInactive::after { - content: " ▲"; + content: ' ▲'; opacity: 0; } @@ -112,7 +112,7 @@ } .treeViewHeaderColumn.sortAscending::after { - content: ' ▼ '; + content: ' ▼'; } .treeBadge { diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index 6a121b7581..8ce62a1e4c 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -101,15 +101,10 @@ class TreeViewHeader extends React.PureComponent< > { _onSort = (e: MouseEvent) => { const { onSort } = this.props; -<<<<<<< HEAD - // $FlowExpectError - onSort(Number(e.currentTarget.dataset.column)); -======= - const target = e.target; + const target = e.currentTarget; if (target instanceof HTMLElement) { - onSort(Number(target.getAttribute('data-column'))); + onSort(Number(target.dataset.column)); } ->>>>>>> 5f297e49 (Fix linter errors) }; render() { diff --git a/src/profile-logic/call-tree.js b/src/profile-logic/call-tree.js index 89fd02beb1..bdd31091dc 100644 --- a/src/profile-logic/call-tree.js +++ b/src/profile-logic/call-tree.js @@ -353,6 +353,8 @@ export class CallTree { iconSrc, icon, ariaLabel, + rawTotal: total, + rawSelf: self, }; this._displayDataByIndex.set(callNodeIndex, displayData); } diff --git a/src/test/components/MarkerTable.test.js b/src/test/components/MarkerTable.test.js index ba67e6a2d7..4cf67da211 100644 --- a/src/test/components/MarkerTable.test.js +++ b/src/test/components/MarkerTable.test.js @@ -103,6 +103,17 @@ describe('MarkerTable', function () { expect(scrolledRows()).toHaveLength(2); }); + it('sorts when the start header and duration header is clicked', () => { + const { container, getByText } = setup(); + + fireFullClick(getByText('Start')); + expect(container).toMatchSnapshot(); + fireFullClick(getByText('Start')); + expect(container).toMatchSnapshot(); + fireFullClick(getByText('Duration')); + expect(container).toMatchSnapshot(); + }); + it('selects a row when left clicking', () => { const { getByText, getRowElement } = setup(); diff --git a/src/test/components/ProfileCallTreeView.test.js b/src/test/components/ProfileCallTreeView.test.js index 9e9ed6cf3a..7637d2dd45 100644 --- a/src/test/components/ProfileCallTreeView.test.js +++ b/src/test/components/ProfileCallTreeView.test.js @@ -292,6 +292,17 @@ describe('calltree/ProfileCallTreeView', function () { const { container } = setup(profile); expect(container.querySelector('.treeViewRow.isSelected')).toBeFalsy(); }); + + it('sorts when the total column header is clicked', () => { + const { container, getByText } = setup(); + + expect(container.firstChild).toMatchSnapshot(); + + fireFullClick(getByText('Total (samples)')); + expect(container.firstChild).toMatchSnapshot(); + fireFullClick(getByText('Total (samples)')); + expect(container.firstChild).toMatchSnapshot(); + }); }); describe('calltree/ProfileCallTreeView EmptyReasons', function () { diff --git a/src/test/components/__snapshots__/MarkerTable.test.js.snap b/src/test/components/__snapshots__/MarkerTable.test.js.snap index 6ef48c2e40..0f5b43d9f2 100644 --- a/src/test/components/__snapshots__/MarkerTable.test.js.snap +++ b/src/test/components/__snapshots__/MarkerTable.test.js.snap @@ -88,19 +88,19 @@ exports[`MarkerTable renders some basic markers and updates when needed 1`] = ` > Start Duration Type @@ -459,3 +459,1287 @@ exports[`MarkerTable renders some basic markers and updates when needed 1`] = `
`; + +exports[`MarkerTable sorts when the start header and duration header is clicked 1`] = ` +
+
+
+
+
+ +
+
+
+
+ + Start + + + Duration + + + Type + + + Description + +
+
+
+
+
+
+
+
+ + 0.162s + + + 1ms + + + FileIO + +
+
+ + 0.158s + + + + Log + +
+
+ + 0.153s + + + 584.00ns + + + Text + +
+
+ + 0.153s + + + 584.00ns + + + Text + +
+
+ + 0.108s + + + unknown + + + IPC + +
+
+ + 0.002s + + + + Paint + +
+
+ + 0.000s + + + 0s + + + UserTiming + +
+
+
+
+
+
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+
+
+
+
+
+
+
+`; + +exports[`MarkerTable sorts when the start header and duration header is clicked 2`] = ` +
+
+
+
+
+ +
+
+
+
+ + Start + + + Duration + + + Type + + + Description + +
+
+
+
+
+
+
+
+ + 0.000s + + + 0s + + + UserTiming + +
+
+ + 0.002s + + + + Paint + +
+
+ + 0.108s + + + unknown + + + IPC + +
+
+ + 0.153s + + + 584.00ns + + + Text + +
+
+ + 0.153s + + + 584.00ns + + + Text + +
+
+ + 0.158s + + + + Log + +
+
+ + 0.162s + + + 1ms + + + FileIO + +
+
+
+
+
+
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+
+
+
+
+
+
+
+`; + +exports[`MarkerTable sorts when the start header and duration header is clicked 3`] = ` +
+
+
+
+
+ +
+
+
+
+ + Start + + + Duration + + + Type + + + Description + +
+
+
+
+
+
+
+
+ + 0.158s + + + + Log + +
+
+ + 0.108s + + + unknown + + + IPC + +
+
+ + 0.002s + + + + Paint + +
+
+ + 0.162s + + + 1ms + + + FileIO + +
+
+ + 0.153s + + + 584.00ns + + + Text + +
+
+ + 0.153s + + + 584.00ns + + + Text + +
+
+ + 0.000s + + + 0s + + + UserTiming + +
+
+
+
+
+
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+ +
+
+
+
+
+
+
+
+
+`; diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index 60d8c9b3ed..8bc7a49204 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -150,11 +150,11 @@ exports[`ProfileCallTreeView with JS Allocations matches the snapshot for JS all >
`; + +exports[`calltree/ProfileCallTreeView sorts when the total column header is clicked 1`] = ` +
+
+
    +
  • + + + +
    • +
  • + +
    • +
+
+ +
+
+
    +
  1. + + Complete “⁨Empty⁩” + +
    2. +
+
+
+ + + Total (samples) + + + Self + + + +
+
+
+
+
+
+
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 67% + + + 2 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + 1 + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+
+
+
+
+
+ + + + + A + + +
+
+ + + + + B + + +
+
+ + + + + C + + +
+
+ + + + + D + + +
+
+ +
+ + +
+
+
+
+
+
+
+`; + +exports[`calltree/ProfileCallTreeView sorts when the total column header is clicked 2`] = ` +
+
+
    +
  • + + + +
    • +
  • + +
    • +
+
+ +
+
+
    +
  1. + + Complete “⁨Empty⁩” + +
    2. +
+
+
+ + + Total (samples) + + + Self + + + +
+
+
+
+
+
+
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 67% + + + 2 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + 1 + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+
+
+
+
+
+ + + + + A + + +
+
+ + + + + B + + +
+
+ + + + + C + + +
+
+ + + + + D + + +
+
+ +
+ + +
+
+
+
+
+
+
+`; + +exports[`calltree/ProfileCallTreeView sorts when the total column header is clicked 3`] = ` +
+
+
    +
  • + + + +
    • +
  • + +
    • +
+
+ +
+
+
    +
  1. + + Complete “⁨Empty⁩” + +
    2. +
+
+
+ + + Total (samples) + + + Self + + + +
+
+
+
+
+
+
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 67% + + + 2 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + 1 + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+
+
+
+
+
+ + + + + A + + +
+
+ + + + + B + + +
+ +
+ + + + + C + + +
+
+ + + + + D + + +
+
+ +
+ +
+
+
+
+
+
+
+`; diff --git a/src/test/store/icons.test.js b/src/test/store/icons.test.js index ff78a1397e..b3b6549444 100644 --- a/src/test/store/icons.test.js +++ b/src/test/store/icons.test.js @@ -48,6 +48,8 @@ describe('actions/icons', function () { icon, iconSrc: 'https://edition.cnn.com/favicon.ico', ariaLabel: 'fake aria label', + rawTotal: 0, + rawSelf: 0, }; } diff --git a/src/test/unit/profile-tree.test.js b/src/test/unit/profile-tree.test.js index 12a242ff21..a628cc032a 100644 --- a/src/test/unit/profile-tree.test.js +++ b/src/test/unit/profile-tree.test.js @@ -335,6 +335,9 @@ describe('unfiltered call tree', function () { totalPercent: '100%', categoryColor: 'grey', categoryName: 'Other', + rawSelf: 0, + rawTotal: 3, + badge: undefined, }); expect(callTree.getDisplayData(I)).toEqual({ ariaLabel: @@ -351,6 +354,9 @@ describe('unfiltered call tree', function () { totalPercent: '33%', categoryColor: 'grey', categoryName: 'Other', + rawSelf: 1, + rawTotal: 1, + badge: undefined, }); }); }); diff --git a/src/types/profile-derived.js b/src/types/profile-derived.js index d8f413437c..5fcf9dd610 100644 --- a/src/types/profile-derived.js +++ b/src/types/profile-derived.js @@ -244,6 +244,9 @@ export type CallNodeDisplayData = $Exact< badge?: ExtraBadgeInfo, icon: string | null, ariaLabel: string, + // just used for sorting + rawSelf: number, + rawTotal: number, }> >; From 801ff27107871f90dbe6a079e90880d2389f60e2 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 5 Oct 2022 16:59:54 +0200 Subject: [PATCH 4/8] Incorporate the suggested changes --- src/components/calltree/CallTree.js | 11 +- src/components/marker-table/index.js | 21 +- src/components/shared/TreeView.css | 2 +- src/components/shared/TreeView.js | 108 ++--- .../__snapshots__/MarkerTable.test.js.snap | 252 ++++++------ .../ProfileCallTreeView.test.js.snap | 372 +++++++++--------- 6 files changed, 389 insertions(+), 377 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 84c24d229d..c787e8612a 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -90,20 +90,21 @@ class CallTreeImpl extends PureComponent { }; _treeView: TreeView | null = null; _takeTreeViewRef = (treeView) => (this._treeView = treeView); + _sortableColumns = new Set(['self', 'total']); _sortedColumns = new ColumnSortState([]); _compareColumn = ( first: CallNodeDisplayData, second: CallNodeDisplayData, - column: number + column: string ) => { switch (column) { - case 2: + case 'total': return second.rawTotal - first.rawTotal; - case 3: + case 'self': return second.rawSelf - first.rawSelf; default: - throw new Error('Invalid column'); + throw new Error('Invalid column ' + column); } }; @@ -322,7 +323,7 @@ class CallTreeImpl extends PureComponent { initialSortedColumns={this._sortedColumns} onSort={this._onSort} compareColumn={this._compareColumn} - sortableColumns={new Set([2, 3])} + sortableColumns={this._sortableColumns} /> ); } diff --git a/src/components/marker-table/index.js b/src/components/marker-table/index.js index 7032a22c5a..0106c24e1b 100644 --- a/src/components/marker-table/index.js +++ b/src/components/marker-table/index.js @@ -116,12 +116,12 @@ class MarkerTree { let duration = null; let rawDuration: number | null = null; + const markerEnd = marker.end; if (marker.incomplete) { duration = 'unknown'; - } else if (marker.end !== null) { - duration = formatTimestamp(marker.end - marker.start); - // $FlowFixMe - rawDuration = marker.end - marker.start; + } else if (markerEnd !== null) { + duration = formatTimestamp(markerEnd - marker.start); + rawDuration = markerEnd - marker.start; } displayData = { @@ -171,6 +171,7 @@ class MarkerTableImpl extends PureComponent { { propName: 'duration', titleL10nId: 'MarkerTable--duration' }, { propName: 'type', titleL10nId: 'MarkerTable--type' }, ]; + _sortableColumns = new Set(['start', 'duration', 'type', 'name']); _mainColumn = { propName: 'name', titleL10nId: 'MarkerTable--description' }; _expandedNodeIds: Array = []; _onExpandedNodeIdsChange = () => {}; @@ -215,12 +216,12 @@ class MarkerTableImpl extends PureComponent { _compareColumn = ( first: MarkerDisplayData, second: MarkerDisplayData, - column: number + column: string ) => { switch (column) { - case 1: + case 'start': return second.rawStart - first.rawStart; - case 2: + case 'duration': if (first.rawDuration === null) { return -1; } @@ -228,10 +229,10 @@ class MarkerTableImpl extends PureComponent { return 1; } return second.rawDuration - first.rawDuration; - case 3: + case 'type': return first.type.localeCompare(second.type); default: - throw new Error('Invalid column'); + throw new Error('Invalid column ' + column); } }; @@ -285,7 +286,7 @@ class MarkerTableImpl extends PureComponent { initialSortedColumns={this._sortedColumns} onSort={this._onSort} compareColumn={this._compareColumn} - sortableColumns={new Set([1, 2, 3])} + sortableColumns={this._sortableColumns} /> )}
diff --git a/src/components/shared/TreeView.css b/src/components/shared/TreeView.css index 5900fec852..45e9cda9ae 100644 --- a/src/components/shared/TreeView.css +++ b/src/components/shared/TreeView.css @@ -112,7 +112,7 @@ } .treeViewHeaderColumn.sortAscending::after { - content: ' ▼'; + content: ' ▼ '; } .treeBadge { diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index 8ce62a1e4c..e6bdda5e28 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -53,46 +53,57 @@ export type Column = {| |}>, |}; +type SingleColumnSortState = {| + column: string, + ascending: boolean, +|}; + type TreeViewHeaderProps = {| +fixedColumns: Column[], +mainColumn: Column, - +onSort: (number) => void, - +columnSortDirections: Array | null, + +onSort: (string) => void, + +currentSortedColumn: SingleColumnSortState | null, |}; -// columns start at 1 export class ColumnSortState { - sortedColumns: number[]; + sortedColumns: SingleColumnSortState[]; - // -column: sort descending, +column: sort ascending, start by sorting last column - constructor(sortedColumns: number[]) { + // start by sorting last column + constructor(sortedColumns: SingleColumnSortState[]) { this.sortedColumns = sortedColumns; } - _sortColumn(column: number) { - const sortedColumns = this.sortedColumns.filter( - (c) => c !== column && c !== -column - ); - sortedColumns.push(column); + sortColumn(column: string, ascending: boolean | null = null) { + const current = this.getStateForColumn(column); + const sortedColumns = this.sortedColumns.filter((c) => c.column !== column); + if (ascending === true || current === null) { + sortedColumns.push({ column, ascending: true }); + } else { + sortedColumns.push(this._invertSortState(current)); + } return new ColumnSortState(sortedColumns); } - push(column: number) { - return this._sortColumn(-this._sortState(column)); + _invertSortState(state: SingleColumnSortState): SingleColumnSortState { + return { column: state.column, ascending: !state.ascending }; } - _sortState(column: number) { + getStateForColumn(column: string): SingleColumnSortState | null { return this.sortedColumns - .filter((c) => c === column || c === -column) - .concat(-column)[0]; + .filter((c) => c.column === column) + .concat(null)[0]; } - getSortStateArr(sortableColumns: Set): Array { - const arr = new Array(Math.max(...sortableColumns) + 1).fill(null); - for (const column of sortableColumns) { - arr[column] = this._sortState(column) < 0; - } - return arr; + getStateForColumnOrDefault(column: string): SingleColumnSortState { + return this.sortedColumns + .filter((c) => c.column === column) + .concat({ column: column, ascending: true })[0]; + } + + current(): SingleColumnSortState | null { + return this.sortedColumns.length > 0 + ? this.sortedColumns[this.sortedColumns.length - 1] + : null; } } @@ -108,21 +119,26 @@ class TreeViewHeader extends React.PureComponent< }; render() { - const { fixedColumns, mainColumn, columnSortDirections } = this.props; + const { fixedColumns, mainColumn, currentSortedColumn } = this.props; if (fixedColumns.length === 0 && !mainColumn.titleL10nId) { // If there is nothing to display in the header, do not render it. return null; } return (
- {fixedColumns.map((col, i) => { + {fixedColumns.map((col) => { let sortClass = ''; - if (columnSortDirections) { - if (columnSortDirections[i + 1] === true) { - sortClass = 'sortAscending'; - } else if (columnSortDirections[i + 1] === false) { + if ( + currentSortedColumn && + currentSortedColumn.column === col.propName + ) { + if (currentSortedColumn.ascending) { sortClass = 'sortDescending'; + } else { + sortClass = 'sortAscending'; } + } else { + sortClass = 'sortInactive'; } return ( extends React.PureComponent< > @@ -457,10 +473,10 @@ type TreeViewProps = {| +indentWidth: CssPixels, +onKeyDown?: (SyntheticKeyboardEvent<>) => void, // number: column number, -1: first larger, 0: same, 1: first smaller - +compareColumn?: (DisplayData, DisplayData, number) => number, + +compareColumn?: (DisplayData, DisplayData, string) => number, +initialSortedColumns?: ColumnSortState, +onSort?: (ColumnSortState) => void, - +sortableColumns?: Set, + +sortableColumns?: Set, |}; type TreeViewState = {| @@ -507,28 +523,26 @@ export class TreeView extends React.PureComponent< tree: Tree, expandedNodes: Set, sortedColumns: ColumnSortState, - compareColumn: ?(DisplayData, DisplayData, number) => number + compareColumn: ?(DisplayData, DisplayData, string) => number ) => { function sortNodes(nodeIds: Array): Array { if (!compareColumn) { return nodeIds; } let sortedNodeIds = nodeIds; - for (let i = 0; i < sortedColumns.sortedColumns.length; i++) { - const column = sortedColumns.sortedColumns[i]; - const absColumn = Math.abs(column); - const sign = column < 0 ? -1 : 1; + for (const { column, ascending } of sortedColumns.sortedColumns) { + const sign = ascending ? 1 : -1; sortedNodeIds = sortedNodeIds.sort((a, b) => { return ( sign * ((compareColumn: any): ( DisplayData, DisplayData, - number + string ) => number)( tree.getDisplayData(a), tree.getDisplayData(b), - absColumn + column ) ); }); @@ -541,12 +555,12 @@ export class TreeView extends React.PureComponent< if (!expandedNodes.has(nodeId)) { return; } - const children = sortNodes(tree.getChildren(nodeId)); + const children = tree.getChildren(nodeId); for (let i = 0; i < children.length; i++) { _addVisibleRowsFromNode(tree, expandedNodes, arr, children[i]); } } - + // we only sort the root nodes for now const roots = sortNodes(tree.getRoots()); const allRows = []; for (let i = 0; i < roots.length; i++) { @@ -855,15 +869,15 @@ export class TreeView extends React.PureComponent< } } - _onSort = (column: number) => { + _onSort = (column: string) => { if ( !this.props.sortableColumns || !this.props.sortableColumns.has(column) ) { return; } - this.setState((state) => { - const newSortedColumns = state.sortedColumns.push(column); + this.setState((x) => { + const newSortedColumns = x.sortedColumns.sortColumn(column); if (this.props.onSort) { this.props.onSort(newSortedColumns); } @@ -883,20 +897,16 @@ export class TreeView extends React.PureComponent< maxNodeDepth, rowHeight, selectedNodeId, - sortableColumns, } = this.props; const { sortedColumns } = this.state; - const columnSortDirections = - sortableColumns && sortedColumns - ? sortedColumns.getSortStateArr(sortableColumns) - : null; + const currentSortedColumn = sortedColumns.current(); return (
Start Duration Type @@ -514,20 +514,20 @@ exports[`MarkerTable sorts when the start header and duration header is clicked class="treeViewHeader" > Start Duration Type @@ -566,21 +566,21 @@ exports[`MarkerTable sorts when the start header and duration header is clicked > - 0.162s + 0.000s - 1ms + 0s - FileIO + UserTiming
- 0.158s + 0.002s - Log + Paint
- 0.153s + 0.108s - 584.00ns + unknown - Text + IPC
- 0.108s + 0.153s - unknown + 584.00ns - IPC + Text
- 0.002s + 0.158s - Paint + Log
- 0.000s + 0.162s - 0s + 1ms - UserTiming + FileIO
@@ -734,7 +734,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked aria-level="1" aria-selected="false" class="treeViewRow treeViewRowScrolledColumns even" - id="treeViewRow-6" + id="treeViewRow-0" role="treeitem" style="height: 16px; line-height: 16px;" > @@ -748,14 +748,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - (PoisonIOInterposer) create/open — /foo/bar + foobar
@@ -769,15 +769,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - (nsJarProtocol) nsJARChannel::nsJARChannel [this=0x87f1ec80] - + NotifyDidPaint
@@ -791,14 +790,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - setTimeout — 5.5 + IPCOut — PContent::Msg_PreferenceUpdate — sent to 2222
@@ -812,14 +811,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed eget magna sed magna vehicula congue id id nulla. Ut convallis, neque consequat aliquam egestas, dui urna interdum quam, id semper magna erat et nisi. Vivamus molestie quis ligula eget aliquam. Sed facilisis, turpis sed facilisis posuere, risus odio convallis velit, vitae vehicula justo risus at ipsum. Proin non porttitor neque. Vivamus fringilla ex nec iaculis cursus. Vestibulum suscipit mauris sem, vitae gravida ipsum fermentum id. Q… + setTimeout — 5.5
@@ -833,14 +832,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - IPCOut — PContent::Msg_PreferenceUpdate — sent to 2222 + Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed eget magna sed magna vehicula congue id id nulla. Ut convallis, neque consequat aliquam egestas, dui urna interdum quam, id semper magna erat et nisi. Vivamus molestie quis ligula eget aliquam. Sed facilisis, turpis sed facilisis posuere, risus odio convallis velit, vitae vehicula justo risus at ipsum. Proin non porttitor neque. Vivamus fringilla ex nec iaculis cursus. Vestibulum suscipit mauris sem, vitae gravida ipsum fermentum id. Q…
@@ -854,14 +853,15 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - NotifyDidPaint + (nsJarProtocol) nsJARChannel::nsJARChannel [this=0x87f1ec80] +
@@ -875,7 +875,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - foobar + (PoisonIOInterposer) create/open — /foo/bar
@@ -942,20 +942,20 @@ exports[`MarkerTable sorts when the start header and duration header is clicked class="treeViewHeader" > Start Duration Type @@ -1370,20 +1370,20 @@ exports[`MarkerTable sorts when the start header and duration header is clicked class="treeViewHeader" > Start Duration Type @@ -1422,19 +1422,21 @@ exports[`MarkerTable sorts when the start header and duration header is clicked > - 0.158s + 0.000s + title="0s" + > + 0s + - Log + UserTiming
- 0.108s + 0.002s - unknown - + title="" + /> - IPC + Paint
- 0.002s + 0.108s + title="unknown" + > + unknown + - Paint + IPC
- 0.162s + 0.153s - 1ms + 584.00ns - FileIO + Text
- 0.153s + 0.158s - 584.00ns - + title="" + /> - Text + Log
- 0.000s + 0.162s - 0s + 1ms - UserTiming + FileIO
@@ -1590,7 +1590,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked aria-level="1" aria-selected="false" class="treeViewRow treeViewRowScrolledColumns even" - id="treeViewRow-5" + id="treeViewRow-0" role="treeitem" style="height: 16px; line-height: 16px;" > @@ -1604,15 +1604,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - (nsJarProtocol) nsJARChannel::nsJARChannel [this=0x87f1ec80] - + foobar
@@ -1626,14 +1625,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - IPCOut — PContent::Msg_PreferenceUpdate — sent to 2222 + NotifyDidPaint
@@ -1647,14 +1646,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - NotifyDidPaint + IPCOut — PContent::Msg_PreferenceUpdate — sent to 2222
@@ -1668,14 +1667,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - (PoisonIOInterposer) create/open — /foo/bar + setTimeout — 5.5
@@ -1689,14 +1688,14 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - setTimeout — 5.5 + Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed eget magna sed magna vehicula congue id id nulla. Ut convallis, neque consequat aliquam egestas, dui urna interdum quam, id semper magna erat et nisi. Vivamus molestie quis ligula eget aliquam. Sed facilisis, turpis sed facilisis posuere, risus odio convallis velit, vitae vehicula justo risus at ipsum. Proin non porttitor neque. Vivamus fringilla ex nec iaculis cursus. Vestibulum suscipit mauris sem, vitae gravida ipsum fermentum id. Q…
@@ -1710,14 +1709,15 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed eget magna sed magna vehicula congue id id nulla. Ut convallis, neque consequat aliquam egestas, dui urna interdum quam, id semper magna erat et nisi. Vivamus molestie quis ligula eget aliquam. Sed facilisis, turpis sed facilisis posuere, risus odio convallis velit, vitae vehicula justo risus at ipsum. Proin non porttitor neque. Vivamus fringilla ex nec iaculis cursus. Vestibulum suscipit mauris sem, vitae gravida ipsum fermentum id. Q… + (nsJarProtocol) nsJARChannel::nsJARChannel [this=0x87f1ec80] +
@@ -1731,7 +1731,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked - foobar + (PoisonIOInterposer) create/open — /foo/bar
diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index 8bc7a49204..f8e301edc5 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -149,12 +149,12 @@ exports[`ProfileCallTreeView with JS Allocations matches the snapshot for JS all class="treeViewHeader" > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
+
From be61e6deea8e4a7ad90b10a44af470d12cb5d754 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 5 Oct 2022 17:40:28 +0200 Subject: [PATCH 5/8] Fix initial state and add call tree test --- src/components/calltree/CallTree.js | 2 +- src/components/marker-table/index.js | 2 +- src/components/shared/TreeView.js | 4 +- .../components/ProfileCallTreeView.test.js | 11 + .../__snapshots__/MarkerTable.test.js.snap | 8 +- .../ProfileCallTreeView.test.js.snap | 1930 ++++++++++++++++- 6 files changed, 1932 insertions(+), 25 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index c787e8612a..7830de0607 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -91,7 +91,7 @@ class CallTreeImpl extends PureComponent { _treeView: TreeView | null = null; _takeTreeViewRef = (treeView) => (this._treeView = treeView); _sortableColumns = new Set(['self', 'total']); - _sortedColumns = new ColumnSortState([]); + _sortedColumns = new ColumnSortState([{ column: 'total', ascending: false }]); _compareColumn = ( first: CallNodeDisplayData, diff --git a/src/components/marker-table/index.js b/src/components/marker-table/index.js index 0106c24e1b..af37bbc5e7 100644 --- a/src/components/marker-table/index.js +++ b/src/components/marker-table/index.js @@ -177,7 +177,7 @@ class MarkerTableImpl extends PureComponent { _onExpandedNodeIdsChange = () => {}; _treeView: ?TreeView; _takeTreeViewRef = (treeView) => (this._treeView = treeView); - _sortedColumns = new ColumnSortState([]); + _sortedColumns = new ColumnSortState([{ column: 'start', ascending: true }]); getMarkerTree = memoize((...args) => new MarkerTree(...args), { limit: 1 }); diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index e6bdda5e28..f19be583a3 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -489,7 +489,7 @@ export class TreeView extends React.PureComponent< > { _list: VirtualList | null = null; _takeListRef = (list: VirtualList | null) => (this._list = list); - state = { sortedColumns: new ColumnSortState([]) }; + state = { sortedColumns: new ColumnSortState([], '') }; constructor(props: TreeViewProps) { super(props); @@ -531,7 +531,7 @@ export class TreeView extends React.PureComponent< } let sortedNodeIds = nodeIds; for (const { column, ascending } of sortedColumns.sortedColumns) { - const sign = ascending ? 1 : -1; + const sign = ascending ? -1 : 1; sortedNodeIds = sortedNodeIds.sort((a, b) => { return ( sign * diff --git a/src/test/components/ProfileCallTreeView.test.js b/src/test/components/ProfileCallTreeView.test.js index 7637d2dd45..d7656a9968 100644 --- a/src/test/components/ProfileCallTreeView.test.js +++ b/src/test/components/ProfileCallTreeView.test.js @@ -303,6 +303,17 @@ describe('calltree/ProfileCallTreeView', function () { fireFullClick(getByText('Total (samples)')); expect(container.firstChild).toMatchSnapshot(); }); + + it('sorts when the total header and self header is clicked', () => { + const { container, getByText } = setup(); + + fireFullClick(getByText('Total (samples)')); + expect(container).toMatchSnapshot(); + fireFullClick(getByText('Total (samples)')); + expect(container).toMatchSnapshot(); + fireFullClick(getByText('Self')); + expect(container).toMatchSnapshot(); + }); }); describe('calltree/ProfileCallTreeView EmptyReasons', function () { diff --git a/src/test/components/__snapshots__/MarkerTable.test.js.snap b/src/test/components/__snapshots__/MarkerTable.test.js.snap index f42faa47d6..460fbaf483 100644 --- a/src/test/components/__snapshots__/MarkerTable.test.js.snap +++ b/src/test/components/__snapshots__/MarkerTable.test.js.snap @@ -87,7 +87,7 @@ exports[`MarkerTable renders some basic markers and updates when needed 1`] = ` class="treeViewHeader" > Start @@ -514,7 +514,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked class="treeViewHeader" > Start @@ -942,7 +942,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked class="treeViewHeader" > Start @@ -1370,7 +1370,7 @@ exports[`MarkerTable sorts when the start header and duration header is clicked class="treeViewHeader" > Start diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index f8e301edc5..3ec1614e22 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -153,7 +153,7 @@ exports[`ProfileCallTreeView with JS Allocations matches the snapshot for JS all data-column="totalPercent" />
`; + +exports[`calltree/ProfileCallTreeView sorts when the total header and self header is clicked 1`] = ` +
+
+
+
    +
  • + + + +
    • +
  • + +
    • +
+
+ +
+
+
    +
  1. + + Complete “⁨Empty⁩” + +
    2. +
+
+
+ + + Total (samples) + + + Self + + + +
+
+
+
+
+
+
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 67% + + + 2 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + 1 + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+
+
+
+
+
+ + + + + A + + +
+
+ + + + + B + + +
+
+ + + + + C + + +
+
+ + + + + D + + +
+
+ +
+ + +
+
+
+
+
+
+
+
+`; + +exports[`calltree/ProfileCallTreeView sorts when the total header and self header is clicked 2`] = ` +
+
+
+
    +
  • + + + +
    • +
  • + +
    • +
+
+ +
+
+
    +
  1. + + Complete “⁨Empty⁩” + +
    2. +
+
+
+ + + Total (samples) + + + Self + + + +
+
+
+
+
+
+
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 67% + + + 2 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + 1 + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+
+
+
+
+
+ + + + + A + + +
+
+ + + + + B + + +
+
+ + + + + C + + +
+
+ + + + + D + + +
+
+ +
+ + +
+
+
+
+
+
+
+
+`; + +exports[`calltree/ProfileCallTreeView sorts when the total header and self header is clicked 3`] = ` +
+
+
+
    +
  • + + + +
    • +
  • + +
    • +
+
+ +
+
+
    +
  1. + + Complete “⁨Empty⁩” + +
    2. +
+
+
+ + + Total (samples) + + + Self + + + +
+
+
+
+
+
+
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 100% + + + 3 + + + — + + +
+ +
+
+ + 67% + + + 2 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + 1 + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+ + 33% + + + 1 + + + — + + +
+ +
+
+
+
+
+
+
+ + + + + A + + +
+
+ + + + + B + + +
+
+ + + + + C + + +
+
+ + + + + D + + +
+
+ +
+ + +
+
+
+
+
+
+
+
+`; From 81e746d8c00958c01f1b19811e738831ae2751ec Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 19 Oct 2022 11:39:35 +0200 Subject: [PATCH 6/8] Fixed wrong sort order when clicking on different column --- src/components/shared/TreeView.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index f19be583a3..ecdaf0a51b 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -74,7 +74,7 @@ export class ColumnSortState { } sortColumn(column: string, ascending: boolean | null = null) { - const current = this.getStateForColumn(column); + const current = this._isLastSortedColumn(column) ? this.getStateForColumn(column) : null; const sortedColumns = this.sortedColumns.filter((c) => c.column !== column); if (ascending === true || current === null) { sortedColumns.push({ column, ascending: true }); @@ -84,6 +84,11 @@ export class ColumnSortState { return new ColumnSortState(sortedColumns); } + _isLastSortedColumn(column: string) { + const cur = this.current(); + return cur !== null && cur.column === column; + } + _invertSortState(state: SingleColumnSortState): SingleColumnSortState { return { column: state.column, ascending: !state.ascending }; } @@ -95,9 +100,7 @@ export class ColumnSortState { } getStateForColumnOrDefault(column: string): SingleColumnSortState { - return this.sortedColumns - .filter((c) => c.column === column) - .concat({ column: column, ascending: true })[0]; + return this.getStateForColumn(column) || { column: column, ascending: true }; } current(): SingleColumnSortState | null { @@ -489,7 +492,7 @@ export class TreeView extends React.PureComponent< > { _list: VirtualList | null = null; _takeListRef = (list: VirtualList | null) => (this._list = list); - state = { sortedColumns: new ColumnSortState([], '') }; + state = { sortedColumns: new ColumnSortState([]) }; constructor(props: TreeViewProps) { super(props); From e6d7d95dabd6468f5abaf7229428022dca92e613 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 19 Oct 2022 14:15:09 +0200 Subject: [PATCH 7/8] Refactor --- src/components/calltree/CallTree.js | 16 - src/components/marker-table/index.js | 58 +- src/components/shared/TreeView.js | 70 +- src/profile-logic/call-tree.js | 35 +- .../ProfileCallTreeView.test.js.snap | 1040 ++++++++--------- 5 files changed, 614 insertions(+), 605 deletions(-) diff --git a/src/components/calltree/CallTree.js b/src/components/calltree/CallTree.js index 7830de0607..95ca47c65a 100644 --- a/src/components/calltree/CallTree.js +++ b/src/components/calltree/CallTree.js @@ -93,21 +93,6 @@ class CallTreeImpl extends PureComponent { _sortableColumns = new Set(['self', 'total']); _sortedColumns = new ColumnSortState([{ column: 'total', ascending: false }]); - _compareColumn = ( - first: CallNodeDisplayData, - second: CallNodeDisplayData, - column: string - ) => { - switch (column) { - case 'total': - return second.rawTotal - first.rawTotal; - case 'self': - return second.rawSelf - first.rawSelf; - default: - throw new Error('Invalid column ' + column); - } - }; - /** * Call Trees can have different types of "weights" for the data. Choose the * appropriate labels for the call tree based on this weight. @@ -322,7 +307,6 @@ class CallTreeImpl extends PureComponent { onDoubleClick={this._onEnterOrDoubleClick} initialSortedColumns={this._sortedColumns} onSort={this._onSort} - compareColumn={this._compareColumn} sortableColumns={this._sortableColumns} /> ); diff --git a/src/components/marker-table/index.js b/src/components/marker-table/index.js index af37bbc5e7..5c90d72045 100644 --- a/src/components/marker-table/index.js +++ b/src/components/marker-table/index.js @@ -72,12 +72,40 @@ class MarkerTree { this._getMarkerLabel = getMarkerLabel; } - getRoots(): MarkerIndex[] { + getRoots(sort: ColumnSortState | null = null): MarkerIndex[] { + if (sort !== null) { + return sort.sortItemsHelper( + this._markerIndexes, + (first: MarkerIndex, second: MarkerIndex, column: string) => { + const firstData = this.getDisplayData(first); + const secondData = this.getDisplayData(second); + switch (column) { + case 'start': + return secondData.rawStart - firstData.rawStart; + case 'duration': + if (firstData.rawDuration === null) { + return -1; + } + if (secondData.rawDuration === null) { + return 1; + } + return secondData.rawDuration - firstData.rawDuration; + case 'type': + return firstData.type.localeCompare(secondData.type); + default: + throw new Error('Invalid column ' + column); + } + } + ); + } return this._markerIndexes; } - getChildren(markerIndex: MarkerIndex): MarkerIndex[] { - return markerIndex === -1 ? this.getRoots() : []; + getChildren( + markerIndex: MarkerIndex, + sort: ColumnSortState | null = null + ): MarkerIndex[] { + return markerIndex === -1 ? this.getRoots(sort) : []; } hasChildren(_markerIndex: MarkerIndex): boolean { @@ -213,29 +241,6 @@ class MarkerTableImpl extends PureComponent { changeRightClickedMarker(threadsKey, selectedMarker); }; - _compareColumn = ( - first: MarkerDisplayData, - second: MarkerDisplayData, - column: string - ) => { - switch (column) { - case 'start': - return second.rawStart - first.rawStart; - case 'duration': - if (first.rawDuration === null) { - return -1; - } - if (second.rawDuration === null) { - return 1; - } - return second.rawDuration - first.rawDuration; - case 'type': - return first.type.localeCompare(second.type); - default: - throw new Error('Invalid column ' + column); - } - }; - _onSort = (sortedColumns: ColumnSortState) => { this._sortedColumns = sortedColumns; }; @@ -285,7 +290,6 @@ class MarkerTableImpl extends PureComponent { indentWidth={10} initialSortedColumns={this._sortedColumns} onSort={this._onSort} - compareColumn={this._compareColumn} sortableColumns={this._sortableColumns} /> )} diff --git a/src/components/shared/TreeView.js b/src/components/shared/TreeView.js index ecdaf0a51b..5e765e4f17 100644 --- a/src/components/shared/TreeView.js +++ b/src/components/shared/TreeView.js @@ -74,7 +74,9 @@ export class ColumnSortState { } sortColumn(column: string, ascending: boolean | null = null) { - const current = this._isLastSortedColumn(column) ? this.getStateForColumn(column) : null; + const current = this._isLastSortedColumn(column) + ? this.getStateForColumn(column) + : null; const sortedColumns = this.sortedColumns.filter((c) => c.column !== column); if (ascending === true || current === null) { sortedColumns.push({ column, ascending: true }); @@ -100,7 +102,9 @@ export class ColumnSortState { } getStateForColumnOrDefault(column: string): SingleColumnSortState { - return this.getStateForColumn(column) || { column: column, ascending: true }; + return ( + this.getStateForColumn(column) || { column: column, ascending: true } + ); } current(): SingleColumnSortState | null { @@ -108,6 +112,18 @@ export class ColumnSortState { ? this.sortedColumns[this.sortedColumns.length - 1] : null; } + + sortItemsHelper(items: T[], compareColumn: (T, T, string) => number): T[] { + let sorted = items; + for (const { column, ascending } of this.sortedColumns) { + const sign = ascending ? -1 : 1; + + sorted = sorted.sort((a, b) => { + return sign * compareColumn(a, b, column); + }); + } + return sorted; + } } class TreeViewHeader extends React.PureComponent< @@ -446,10 +462,10 @@ class TreeViewRowScrolledColumns< interface Tree { getDepth(NodeIndex): number; - getRoots(): NodeIndex[]; + getRoots(ColumnSortState | null): NodeIndex[]; getDisplayData(NodeIndex): DisplayData; getParent(NodeIndex): NodeIndex; - getChildren(NodeIndex): NodeIndex[]; + getChildren(NodeIndex, ColumnSortState | null): NodeIndex[]; hasChildren(NodeIndex): boolean; getAllDescendants(NodeIndex): Set; } @@ -475,8 +491,6 @@ type TreeViewProps = {| +rowHeight: CssPixels, +indentWidth: CssPixels, +onKeyDown?: (SyntheticKeyboardEvent<>) => void, - // number: column number, -1: first larger, 0: same, 1: first smaller - +compareColumn?: (DisplayData, DisplayData, string) => number, +initialSortedColumns?: ColumnSortState, +onSort?: (ColumnSortState) => void, +sortableColumns?: Set, @@ -525,46 +539,20 @@ export class TreeView extends React.PureComponent< ( tree: Tree, expandedNodes: Set, - sortedColumns: ColumnSortState, - compareColumn: ?(DisplayData, DisplayData, string) => number + sortedColumns: ColumnSortState ) => { - function sortNodes(nodeIds: Array): Array { - if (!compareColumn) { - return nodeIds; - } - let sortedNodeIds = nodeIds; - for (const { column, ascending } of sortedColumns.sortedColumns) { - const sign = ascending ? -1 : 1; - sortedNodeIds = sortedNodeIds.sort((a, b) => { - return ( - sign * - ((compareColumn: any): ( - DisplayData, - DisplayData, - string - ) => number)( - tree.getDisplayData(a), - tree.getDisplayData(b), - column - ) - ); - }); - } - return sortedNodeIds; - } - function _addVisibleRowsFromNode(tree, expandedNodes, arr, nodeId) { arr.push(nodeId); if (!expandedNodes.has(nodeId)) { return; } - const children = tree.getChildren(nodeId); + const children = tree.getChildren(nodeId, sortedColumns); for (let i = 0; i < children.length; i++) { _addVisibleRowsFromNode(tree, expandedNodes, arr, children[i]); } } // we only sort the root nodes for now - const roots = sortNodes(tree.getRoots()); + const roots = tree.getRoots(sortedColumns); const allRows = []; for (let i = 0; i < roots.length; i++) { _addVisibleRowsFromNode(tree, expandedNodes, allRows, roots[i]); @@ -652,12 +640,11 @@ export class TreeView extends React.PureComponent< } _getAllVisibleRows(): NodeIndex[] { - const { tree, compareColumn } = this.props; + const { tree } = this.props; return this._computeAllVisibleRowsMemoized( tree, this._getExpandedNodes(), - this.state.sortedColumns, - compareColumn + this.state.sortedColumns ); } @@ -842,7 +829,12 @@ export class TreeView extends React.PureComponent< } else { // Do KEY_DOWN only if the next element is a child if (this.props.tree.hasChildren(selected)) { - this._select(this.props.tree.getChildren(selected)[0]); + this._select( + this.props.tree.getChildren( + selected, + this.state.sortedColumns + )[0] + ); } } break; diff --git a/src/profile-logic/call-tree.js b/src/profile-logic/call-tree.js index bdd31091dc..47a3ba16c2 100644 --- a/src/profile-logic/call-tree.js +++ b/src/profile-logic/call-tree.js @@ -38,6 +38,7 @@ import { formatCallNodeNumber, formatPercent } from '../utils/format-numbers'; import { assertExhaustiveCheck, ensureExists } from '../utils/flow'; import * as ProfileData from './profile-data'; import type { CallTreeSummaryStrategy } from '../types/actions'; +import { ColumnSortState } from '../components/shared/TreeView'; type CallNodeChildren = IndexIntoCallNodeTable[]; type CallNodeSummary = { @@ -119,11 +120,14 @@ export class CallTree { this._weightType = weightType; } - getRoots() { - return this.getChildren(-1); + getRoots(sort: ColumnSortState | null = null) { + return this.getChildren(-1, sort); } - getChildren(callNodeIndex: IndexIntoCallNodeTable): CallNodeChildren { + getChildren( + callNodeIndex: IndexIntoCallNodeTable, + sort: ColumnSortState | null = null + ): CallNodeChildren { let children = this._children[callNodeIndex]; if (children === undefined) { const childCount = @@ -156,6 +160,31 @@ export class CallTree { ); this._children[callNodeIndex] = children; } + if (sort && Array.isArray(children)) { + return sort.sortItemsHelper( + children.slice(), + ( + first: IndexIntoCallNodeTable, + second: IndexIntoCallNodeTable, + column: string + ) => { + switch (column) { + case 'total': + return ( + this._callNodeSummary.total[first] - + this._callNodeSummary.total[second] + ); + case 'self': + return ( + this._callNodeSummary.self[first] - + this._callNodeSummary.self[second] + ); + default: + throw new Error('Invalid column ' + column); + } + } + ); + } return children; } diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index 3ec1614e22..9c6d9d5ce8 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -273,15 +273,15 @@ allocated or deallocated in the program." > - 80% + 20% - 12 + 3 - 5 + — - 47% + 80% - 7 + 12 - — + 5
- 7 + —
- 20% + 47% - 3 + 7 - — + 7
+
-
@@ -1623,15 +1623,15 @@ allocated or deallocated in the program." > - 73% + 27% - 30 + 11 - 13 + — - 41% + 73% - 17 + 30 - — + 13
- 17 + —
- 27% + 41% - 11 + 17 - — + 17
+
-
@@ -2286,15 +2286,15 @@ allocated or deallocated in the program." > - 80% + 20% - 12 + 3 - 5 + — - 47% + 80% - 7 + 12 - — + 5
- 7 + —
- 20% + 47% - 3 + 7 - — + 7
+
-
@@ -3845,21 +3845,21 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 2 + 1 - — + 2 - 33% + 67% - 1 + 2
- 1 + — +
-
@@ -4537,15 +4537,15 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 33% + 67% - 1 + 2
- 1 + —
- — + 1
+
-
@@ -5620,15 +5620,15 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 33% + 67% - 1 + 2
- 1 + —
- — + 1
+
-
@@ -8875,15 +8875,15 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 33% + 67% - 1 + 2
- 1 + —
- — + 1
+
-
@@ -10135,15 +10135,15 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 33% + 67% - 1 + 2
- 1 + —
- — + 1
+
-
@@ -11398,15 +11398,15 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 33% + 67% - 1 + 2
- 1 + —
- — + 1
+
-
@@ -12030,15 +12030,15 @@ for understanding where time was actually spent in a program." > - 67% + 33% - 2 + 1 - 33% + 67% - 1 + 2
- 1 + —
- — + 1
+
-
From ad0b56bb10d3e6e5adfcb4af167ec5b8d3d77521 Mon Sep 17 00:00:00 2001 From: Johannes Bechberger Date: Wed, 19 Oct 2022 14:50:01 +0200 Subject: [PATCH 8/8] Fix small bug --- src/profile-logic/call-tree.js | 8 +- .../ProfileCallTreeView.test.js.snap | 1242 ++++++++--------- 2 files changed, 625 insertions(+), 625 deletions(-) diff --git a/src/profile-logic/call-tree.js b/src/profile-logic/call-tree.js index 47a3ba16c2..1b8a0f6b21 100644 --- a/src/profile-logic/call-tree.js +++ b/src/profile-logic/call-tree.js @@ -171,13 +171,13 @@ export class CallTree { switch (column) { case 'total': return ( - this._callNodeSummary.total[first] - - this._callNodeSummary.total[second] + this._callNodeSummary.total[second] - + this._callNodeSummary.total[first] ); case 'self': return ( - this._callNodeSummary.self[first] - - this._callNodeSummary.self[second] + this._callNodeSummary.self[second] - + this._callNodeSummary.self[first] ); default: throw new Error('Invalid column ' + column); diff --git a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap index 9c6d9d5ce8..60f3069ad1 100644 --- a/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap +++ b/src/test/components/__snapshots__/ProfileCallTreeView.test.js.snap @@ -273,15 +273,15 @@ allocated or deallocated in the program." > - 20% + 80% - 3 + 12 - — + 5 - 80% + 47% - 12 + 7 - 5 + —
- — + 7
- 47% + 20% - 7 + 3 - 7 + —
-
+
@@ -948,15 +948,15 @@ allocated or deallocated in the program." > - -80% + -20% - -12 + -3 - -5 + — - -47% + -80% - -7 + -12 - — + -5
- -7 + —
- -20% + -47% - -3 + -7 - — + -7
+
-
@@ -1623,15 +1623,15 @@ allocated or deallocated in the program." > - 27% + 73% - 11 + 30 - — + 13 - 73% + 41% - 30 + 17 - 13 + —
- — + 17
- 41% + 27% - 17 + 11 - 17 + —
-
+
@@ -2286,15 +2286,15 @@ allocated or deallocated in the program." > - 20% + 80% - 3 + 12 - — + 5 - 80% + 47% - 12 + 7 - 5 + —
- — + 7
- 47% + 20% - 7 + 3 - 7 + —
-
+ @@ -2949,15 +2949,15 @@ allocated or deallocated in the program." > - -59% + -41% - -24 + -17 - — + -17 - -41% + -59% - -17 + -24 - -17 + —
- Fjs + Gjs
- — + 1 -
+
@@ -4537,15 +4537,15 @@ for understanding where time was actually spent in a program." > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
+ @@ -5620,15 +5620,15 @@ for understanding where time was actually spent in a program." > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
+ @@ -8875,15 +8875,15 @@ for understanding where time was actually spent in a program." > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
+ @@ -10135,15 +10135,15 @@ for understanding where time was actually spent in a program." > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
- - - - - B - - -
-
+ @@ -11398,15 +11398,15 @@ for understanding where time was actually spent in a program." > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
+ @@ -12030,15 +12030,15 @@ for understanding where time was actually spent in a program." > - 33% + 67% - 1 + 2 - 67% + 33% - 2 + 1
- — + 1
- 1 + —
-
+