Implement column sorting in the marker table.

The sort is also persisted in the URL.

Author: mstange

src/actions/profile-view.ts

@@ -82,6 +82,7 @@ import {
 import { changeStoredProfileNameInDb } from 'firefox-profiler/app-logic/uploaded-profiles-db';
 import type { TabSlug } from '../app-logic/tabs-handling';
 import type { CallNodeInfo } from '../profile-logic/call-node-info';
+import type { SingleColumnSortState } from '../components/shared/TreeView';
 import { intersectSets } from 'firefox-profiler/utils/set';
 
 /**
@@ -1615,6 +1616,13 @@ export function changeMarkersSearchString(searchString: string): Action {
   };
 }
 
+export function changeMarkerTableSort(sort: SingleColumnSortState[]): Action {
+  return {
+    type: 'CHANGE_MARKER_TABLE_SORT',
+    sort,
+  };
+}
+
 export function changeNetworkSearchString(searchString: string): Action {
   return {
     type: 'CHANGE_NETWORK_SEARCH_STRING',

src/app-logic/url-handling.ts

@@ -52,8 +52,9 @@ import { tabSlugs } from '../app-logic/tabs-handling';
 import { StringTable } from 'firefox-profiler/utils/string-table';
 import type { ProfileUpgradeInfo } from 'firefox-profiler/profile-logic/processed-profile-versioning';
 import type { ProfileAndProfileUpgradeInfo } from 'firefox-profiler/actions/receive-profile';
+import type { SingleColumnSortState } from '../components/shared/TreeView';
 
-export const CURRENT_URL_VERSION = 16;
+export const CURRENT_URL_VERSION = 17;
 
 /**
  * This static piece of state might look like an anti-pattern, but it's a relatively
@@ -190,6 +191,7 @@ type CallTreeQuery = BaseQuery & {
 type MarkersQuery = BaseQuery & {
   markerSearch: string; // "DOMEvent"
   marker?: MarkerIndex; // Selected marker index for the current thread, e.g. 42
+  markerSort?: string; // "duration:desc,start:asc" — primary first
 };
 
 type NetworkQuery = BaseQuery & {
@@ -228,6 +230,7 @@ type Query = BaseQuery & {
   // Markers specific
   markerSearch?: string;
   marker?: MarkerIndex;
+  markerSort?: string;
 
   // Network specific
   networkSearch?: string;
@@ -394,6 +397,9 @@ export function getQueryStringFromUrlState(urlState: UrlState): string {
         urlState.profileSpecific.selectedMarkers[selectedThreadsKey] !== null
           ? urlState.profileSpecific.selectedMarkers[selectedThreadsKey]
           : undefined;
+      query.markerSort = convertMarkerTableSortToString(
+        urlState.profileSpecific.markerTableSort
+      );
       break;
     case 'network-chart':
       query = baseQuery as NetworkQueryShape;
@@ -632,10 +638,59 @@ export function stateFromLocation(
         ? query.hiddenThreads.split('-').map((index) => Number(index))
         : null,
       selectedMarkers,
+      markerTableSort: convertMarkerTableSortFromString(query.markerSort),
     },
   };
 }
 
+// MarkerTable sort URL encoding. The internal ColumnSortState stores the
+// primary-sorted column last (newest click wins as primary); the URL puts the
+// primary first for human readability.
+const VALID_MARKER_SORT_COLUMNS = new Set(['start', 'duration', 'name']);
+
+function convertMarkerTableSortToString(
+  sort: SingleColumnSortState[]
+): string | undefined {
+  if (sort.length === 0) {
+    return undefined;
+  }
+  // Omit when it matches the marker table's own default.
+  if (sort.length === 1 && sort[0].column === 'start' && sort[0].ascending) {
+    return undefined;
+  }
+  return sort
+    .slice()
+    .reverse()
+    .map((s) => `${s.column}-${s.ascending ? 'asc' : 'desc'}`)
+    .join('~');
+}
+
+function convertMarkerTableSortFromString(
+  raw: string | null | void
+): SingleColumnSortState[] {
+  if (!raw) {
+    return [];
+  }
+  const parsed: SingleColumnSortState[] = [];
+  for (const part of raw.split('~')) {
+    const dashIndex = part.lastIndexOf('-');
+    if (dashIndex === -1) {
+      return [];
+    }
+    const column = part.slice(0, dashIndex);
+    const dir = part.slice(dashIndex + 1);
+    if (
+      !VALID_MARKER_SORT_COLUMNS.has(column) ||
+      (dir !== 'asc' && dir !== 'desc')
+    ) {
+      return [];
+    }
+    parsed.push({ column, ascending: dir === 'asc' });
+  }
+  // URL is primary-first; internal storage is primary-last.
+  return parsed.reverse();
+}
+
 function convertGlobalTrackOrderFromString(
   rawString: string | null | void
 ): TrackIndex[] {
@@ -1443,6 +1498,11 @@ const _upgraders: {
         .join('~');
     }
   },
+  [17]: (_processedLocation: ProcessedLocationBeforeUpgrade) => {
+    // Adds the optional `markerSort` query parameter for the marker table.
+    // No migration is necessary: older URLs simply omit it and the default
+    // (sort by start ascending) is used.
+  },
 };
 
 /**

src/components/marker-table/index.tsx

@@ -6,7 +6,7 @@ import { PureComponent } from 'react';
 import memoize from 'memoize-immutable';
 
 import explicitConnect from '../../utils/connect';
-import { type SortableColumn, type Tree, TreeView } from '../shared/TreeView';
+import { ColumnSortState, TreeView } from '../shared/TreeView';
 import { MarkerTableEmptyReasons } from './MarkerTableEmptyReasons';
 import {
   getZeroAt,
@@ -15,11 +15,15 @@ import {
   getCurrentTableViewOptions,
 } from '../../selectors/profile';
 import { selectedThreadSelectors } from '../../selectors/per-thread';
-import { getSelectedThreadsKey } from '../../selectors/url-state';
+import {
+  getSelectedThreadsKey,
+  getMarkerTableSort,
+} from '../../selectors/url-state';
 import {
   changeSelectedMarker,
   changeRightClickedMarker,
   changeTableViewOptions,
+  changeMarkerTableSort,
 } from '../../actions/profile-view';
 import { MarkerSettings } from '../shared/MarkerSettings';
 import { formatSeconds, formatTimestamp } from '../../utils/format-numbers';
@@ -37,12 +41,21 @@ import type {
   TableViewOptions,
   SelectionContext,
 } from 'firefox-profiler/types';
+import type {
+  SingleColumnSortState,
+  Tree,
+  SortableColumn,
+} from '../shared/TreeView';
 
 import type { ConnectedProps } from '../../utils/connect';
 
 // Limit how many characters in the description get sent to the DOM.
 const MAX_DESCRIPTION_CHARACTERS = 500;
 
+const DEFAULT_MARKER_TABLE_SORT: SingleColumnSortState[] = [
+  { column: 'start', ascending: true },
+];
+
 type MarkerDisplayData = {
   start: string;
   duration: string | null;
@@ -73,8 +86,14 @@ class MarkerTree implements Tree<MarkerDisplayData> {
     this._getMarkerLabel = getMarkerLabel;
   }
 
+  static _sortableColumns: SortableColumn[] = [
+    { name: 'start', prefersDescending: false },
+    { name: 'duration', prefersDescending: true },
+    { name: 'name', prefersDescending: false },
+  ];
+
   getSortableColumns(): SortableColumn[] {
-    return [];
+    return MarkerTree._sortableColumns;
   }
 
   copyTable = (
@@ -171,12 +190,49 @@ class MarkerTree implements Tree<MarkerDisplayData> {
     copy(text);
   };
 
-  getRoots(): MarkerIndex[] {
+  getRoots(sort: ColumnSortState | null = null): MarkerIndex[] {
+    if (sort !== null) {
+      return sort.sortItemsHelper(
+        this._markerIndexes,
+        (first: MarkerIndex, second: MarkerIndex, column: string) => {
+          const firstValue = this._getSortValueForColumn(first, column);
+          const secondValue = this._getSortValueForColumn(second, column);
+          if (typeof firstValue === 'string') {
+            return firstValue.localeCompare(secondValue as string);
+          }
+          return (firstValue as number) - (secondValue as number);
+        }
+      );
+    }
     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) : [];
+  }
+
+  _getSortValueForColumn(
+    markerIndex: MarkerIndex,
+    column: string
+  ): string | number {
+    const marker = this._getMarker(markerIndex);
+    switch (column) {
+      case 'start':
+        return marker.start;
+      case 'duration': {
+        if (marker.incomplete || marker.end === null) {
+          return -Infinity;
+        }
+        return marker.end - marker.start;
+      }
+      case 'name':
+        return marker.name;
+      default:
+        throw new Error('Invalid column ' + column);
+    }
   }
 
   hasChildren(_markerIndex: MarkerIndex): boolean {
@@ -209,10 +265,11 @@ class MarkerTree implements Tree<MarkerDisplayData> {
       }
 
       let duration = null;
+      const markerEnd = marker.end;
       if (marker.incomplete) {
         duration = 'unknown';
-      } else if (marker.end !== null) {
-        duration = formatTimestamp(marker.end - marker.start);
+      } else if (markerEnd !== null) {
+        duration = formatTimestamp(markerEnd - marker.start);
       }
 
       displayData = {
@@ -242,12 +299,14 @@ type StateProps = {
   readonly markerSchemaByName: MarkerSchemaByName;
   readonly getMarkerLabel: (param: MarkerIndex) => string;
   readonly tableViewOptions: TableViewOptions;
+  readonly sort: SingleColumnSortState[];
 };
 
 type DispatchProps = {
   readonly changeSelectedMarker: typeof changeSelectedMarker;
   readonly changeRightClickedMarker: typeof changeRightClickedMarker;
   readonly onTableViewOptionsChange: (param: TableViewOptions) => any;
+  readonly changeMarkerTableSort: typeof changeMarkerTableSort;
 };
 
 type Props = ConnectedProps<{}, StateProps, DispatchProps>;
@@ -284,6 +343,11 @@ class MarkerTableImpl extends PureComponent<Props> {
     this._treeView = treeView;
   };
 
+  _getSortedColumns = memoize(
+    (sort: SingleColumnSortState[]) =>
+      new ColumnSortState(sort.length > 0 ? sort : DEFAULT_MARKER_TABLE_SORT)
+  );
+
   getMarkerTree = memoize(
     (
       getMarker: any,
@@ -335,6 +399,10 @@ class MarkerTableImpl extends PureComponent<Props> {
     changeSelectedMarker(threadsKey, selectedMarker, context);
   };
 
+  _onColumnSortChange = (sortedColumns: ColumnSortState) => {
+    this.props.changeMarkerTableSort(sortedColumns.sortedColumns);
+  };
+
   _onRightClickSelection = (selectedMarker: MarkerIndex) => {
     const { threadsKey, changeRightClickedMarker } = this.props;
     changeRightClickedMarker(threadsKey, selectedMarker);
@@ -385,6 +453,8 @@ class MarkerTableImpl extends PureComponent<Props> {
             indentWidth={10}
             viewOptions={this.props.tableViewOptions}
             onViewOptionsChange={this.props.onTableViewOptionsChange}
+            sortedColumns={this._getSortedColumns(this.props.sort)}
+            onColumnSortChange={this._onColumnSortChange}
           />
         )}
       </div>
@@ -405,12 +475,14 @@ export const MarkerTable = explicitConnect<{}, StateProps, DispatchProps>({
     markerSchemaByName: getMarkerSchemaByName(state),
     getMarkerLabel: selectedThreadSelectors.getMarkerTableLabelGetter(state),
     tableViewOptions: getCurrentTableViewOptions(state),
+    sort: getMarkerTableSort(state),
   }),
   mapDispatchToProps: {
     changeSelectedMarker,
     changeRightClickedMarker,
     onTableViewOptionsChange: (tableViewOptions) =>
       changeTableViewOptions('marker-table', tableViewOptions),
+    changeMarkerTableSort,
   },
   component: MarkerTableImpl,
 });

src/reducers/url-state.ts

@@ -27,6 +27,7 @@ import type {
 } from 'firefox-profiler/types';
 
 import type { TabSlug } from '../app-logic/tabs-handling';
+import type { SingleColumnSortState } from '../components/shared/TreeView';
 import { translateThreadsKey } from 'firefox-profiler/profile-logic/profile-data';
 import { translateTransformStack } from 'firefox-profiler/profile-logic/transforms';
 
@@ -193,6 +194,18 @@ const markersSearchString: Reducer<string> = (state = '', action) => {
   }
 };
 
+const markerTableSort: Reducer<SingleColumnSortState[]> = (
+  state = [],
+  action
+) => {
+  switch (action.type) {
+    case 'CHANGE_MARKER_TABLE_SORT':
+      return action.sort;
+    default:
+      return state;
+  }
+};
+
 const networkSearchString: Reducer<string> = (state = '', action) => {
   switch (action.type) {
     case 'CHANGE_NETWORK_SEARCH_STRING':
@@ -793,6 +806,7 @@ const profileSpecific = combineReducers({
   showJsTracerSummary,
   tabFilter,
   selectedMarkers,
+  markerTableSort,
   // The timeline tracks used to be hidden and sorted by thread indexes, rather than
   // track indexes. The only way to migrate this information to tracks-based data is to
   // first retrieve the profile, so they can't be upgraded by the normal url upgrading

src/selectors/url-state.ts

@@ -36,6 +36,7 @@ import type {
 
 import type { TabSlug } from '../app-logic/tabs-handling';
 import type { MarkerRegExps } from '../profile-logic/marker-data';
+import type { SingleColumnSortState } from '../components/shared/TreeView';
 
 import urlStateReducer from '../reducers/url-state';
 import { formatMetaInfoString } from '../profile-logic/profile-metainfo';
@@ -117,6 +118,8 @@ export const getCurrentSearchString: Selector<string> = (state) =>
   getProfileSpecificState(state).callTreeSearchString;
 export const getMarkersSearchString: Selector<string> = (state) =>
   getProfileSpecificState(state).markersSearchString;
+export const getMarkerTableSort: Selector<SingleColumnSortState[]> = (state) =>
+  getProfileSpecificState(state).markerTableSort;
 export const getNetworkSearchString: Selector<string> = (state) =>
   getProfileSpecificState(state).networkSearchString;
 export const getSelectedTab: Selector<TabSlug> = (state) =>

src/test/components/MarkerTable.test.tsx

@@ -478,7 +478,9 @@ describe('MarkerTable', function () {
       let dividerForFirstColumn = ensureExists(
         document.querySelector('.treeViewHeaderColumnDivider')
       );
-      let firstColumn = screen.getByText('Start');
+      let firstColumn = ensureExists(
+        document.querySelector('.treeViewHeaderColumn.start')
+      );
       expect(firstColumn).toHaveStyle({ width: '90px' });
       fireEvent.mouseDown(dividerForFirstColumn, { clientX: 90 });
 
@@ -505,7 +507,9 @@ describe('MarkerTable', function () {
       );
 
       // Make sure the first column kept its width
-      firstColumn = screen.getByText('Start');
+      firstColumn = ensureExists(
+        document.querySelector('.treeViewHeaderColumn.start')
+      );
       expect(firstColumn).toHaveStyle({ width: '80px' });
 
       // Now double click to reset the style.