-
Notifications
You must be signed in to change notification settings - Fork 478
Persist the selected marker in the url #5847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
58f2081
b85d330
0fc3136
ea8a817
e227fa2
33174ba
3864ace
008fd2e
a6f157a
680c730
2990331
c4152c4
1b59618
7a763a1
e93812c
946a4ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,8 @@ import type { | |
| NativeSymbolInfo, | ||
| Transform, | ||
| IndexIntoFrameTable, | ||
| MarkerIndex, | ||
| SelectedMarkersPerThread, | ||
| } from 'firefox-profiler/types'; | ||
| import { | ||
| decodeUintArrayFromUrlComponent, | ||
|
|
@@ -49,7 +51,7 @@ import { | |
| import { tabSlugs } from '../app-logic/tabs-handling'; | ||
| import { StringTable } from 'firefox-profiler/utils/string-table'; | ||
|
|
||
| export const CURRENT_URL_VERSION = 13; | ||
| export const CURRENT_URL_VERSION = 14; | ||
|
|
||
| /** | ||
| * This static piece of state might look like an anti-pattern, but it's a relatively | ||
|
|
@@ -184,6 +186,7 @@ type CallTreeQuery = BaseQuery & { | |
|
|
||
| type MarkersQuery = BaseQuery & { | ||
| markerSearch: string; // "DOMEvent" | ||
| marker?: MarkerIndex; // Selected marker index for the current thread, e.g. 42 | ||
| }; | ||
|
|
||
| type NetworkQuery = BaseQuery & { | ||
|
|
@@ -218,6 +221,7 @@ type Query = BaseQuery & { | |
|
|
||
| // Markers specific | ||
| markerSearch?: string; | ||
| marker?: MarkerIndex; | ||
|
|
||
| // Network specific | ||
| networkSearch?: string; | ||
|
|
@@ -367,11 +371,17 @@ export function getQueryStringFromUrlState(urlState: UrlState): string { | |
| query = baseQuery as MarkersQueryShape; | ||
| query.markerSearch = | ||
| urlState.profileSpecific.markersSearchString || undefined; | ||
| query.marker = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we maybe add a TODO for supporting network-chart in the future too |
||
| selectedThreadsKey !== null && | ||
| urlState.profileSpecific.selectedMarkers[selectedThreadsKey] !== null | ||
| ? urlState.profileSpecific.selectedMarkers[selectedThreadsKey] | ||
| : undefined; | ||
| break; | ||
| case 'network-chart': | ||
| query = baseQuery as NetworkQueryShape; | ||
| query.networkSearch = | ||
| urlState.profileSpecific.networkSearchString || undefined; | ||
| // TODO: Add support for query.marker | ||
| break; | ||
| case 'js-tracer': { | ||
| query = baseQuery as JsTracerQueryShape; | ||
|
|
@@ -499,6 +509,19 @@ export function stateFromLocation( | |
| transforms[selectedThreadsKey] = parseTransforms(query.transforms); | ||
| } | ||
|
|
||
| // Parse the selected marker for the current thread | ||
| const selectedMarkers: SelectedMarkersPerThread = {}; | ||
| if ( | ||
| selectedThreadsKey !== null && | ||
| query.marker !== undefined && | ||
| query.marker !== null | ||
| ) { | ||
| const markerIndex = Number(query.marker); | ||
| if (Number.isInteger(markerIndex) && markerIndex >= 0) { | ||
| selectedMarkers[selectedThreadsKey] = markerIndex; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish there was a way to further validate if the marker index is valid at this stage, but I couldn't find a way. |
||
| } | ||
| } | ||
|
|
||
| // tabID is used for the tab selector that we have in our full view. | ||
| let tabID = null; | ||
| if (query.tabID && Number.isInteger(Number(query.tabID))) { | ||
|
|
@@ -587,6 +610,7 @@ export function stateFromLocation( | |
| legacyHiddenThreads: query.hiddenThreads | ||
| ? query.hiddenThreads.split('-').map((index) => Number(index)) | ||
| : null, | ||
| selectedMarkers, | ||
| }, | ||
| }; | ||
| } | ||
|
|
@@ -1196,6 +1220,10 @@ const _upgraders: { | |
| [13]: (_) => { | ||
| // just added the focus-self transform | ||
| }, | ||
| [14]: (_) => { | ||
| // Added marker parameter for persisting highlighted markers in URLs. | ||
| // This is backward compatible as the marker parameter is optional. | ||
| }, | ||
| }; | ||
|
|
||
| for (let destVersion = 1; destVersion <= CURRENT_URL_VERSION; destVersion++) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,49 @@ function getDefaultMarkerColors(isHighlighted: boolean) { | |
| class MarkerChartCanvasImpl extends React.PureComponent<Props> { | ||
| _textMeasurement: TextMeasurement | null = null; | ||
|
|
||
| override componentDidUpdate(prevProps: Props) { | ||
| const viewportDidMount = | ||
| !prevProps.viewport.isSizeSet && this.props.viewport.isSizeSet; | ||
| const viewportResized = | ||
| this.props.viewport.isSizeSet && | ||
| this.props.viewport.containerHeight !== | ||
| prevProps.viewport.containerHeight; | ||
| const selectedMarkerChanged = | ||
| this.props.selectedMarkerIndex !== prevProps.selectedMarkerIndex; | ||
|
|
||
| if (viewportDidMount || viewportResized || selectedMarkerChanged) { | ||
| this._scrollSelectionIntoView(); | ||
| } | ||
| } | ||
|
|
||
| _scrollSelectionIntoView = () => { | ||
| const { selectedMarkerIndex, markerTimingAndBuckets, rowHeight, viewport } = | ||
| this.props; | ||
|
|
||
| if (selectedMarkerIndex === null) { | ||
| return; | ||
| } | ||
|
|
||
| const markerIndexToTimingRow = this._getMarkerIndexToTimingRow( | ||
| markerTimingAndBuckets | ||
| ); | ||
| const rowIndex = markerIndexToTimingRow[selectedMarkerIndex]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we check if the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was there before, I've removed it because
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will still return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, true
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understood why I had that impression. Without having |
||
|
|
||
| if (rowIndex === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| const y: CssPixels = rowIndex * rowHeight; | ||
| const { viewportTop, viewportBottom } = viewport; | ||
|
|
||
| if (y < viewportTop || y + rowHeight > viewportBottom) { | ||
| // Scroll the marker to the vertical center of the viewport. | ||
| const viewportHeight = viewportBottom - viewportTop; | ||
| const targetViewportTop = y + rowHeight / 2 - viewportHeight / 2; | ||
| viewport.moveViewport(0, viewportTop - targetViewportTop); | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Get the fill, stroke, and text colors for a marker based on its schema and data. | ||
| * If the marker schema has a colorField, use that field's value. | ||
|
|
@@ -957,8 +1000,100 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> { | |
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Compute canvas-relative tooltip offset for the selected marker. | ||
| * Returns null if the marker can't be located in the timing data or | ||
| * is outside the visible viewport. | ||
| */ | ||
| _getSelectedItemTooltipOffset(): { | ||
| offsetX: CssPixels; | ||
| offsetY: CssPixels; | ||
| } | null { | ||
| const { selectedMarkerIndex } = this.props; | ||
| if (selectedMarkerIndex === null) { | ||
| return null; | ||
| } | ||
|
|
||
| const { | ||
| rangeStart, | ||
| rangeEnd, | ||
| markerTimingAndBuckets, | ||
| rowHeight, | ||
| marginLeft, | ||
| marginRight, | ||
| viewport: { | ||
| containerWidth, | ||
| containerHeight, | ||
| viewportLeft, | ||
| viewportRight, | ||
| viewportTop, | ||
| }, | ||
| } = this.props; | ||
|
|
||
| // Step 1: Find which row this marker is displayed in | ||
| const markerIndexToTimingRow = this._getMarkerIndexToTimingRow( | ||
| markerTimingAndBuckets | ||
| ); | ||
| const rowIndex = markerIndexToTimingRow[selectedMarkerIndex]; | ||
|
|
||
| // Step 2: Get the timing data for all markers in this row | ||
| const markerTiming = markerTimingAndBuckets[rowIndex]; | ||
| if (!markerTiming || typeof markerTiming === 'string') { | ||
| // Row is empty or is a bucket label (string), not actual marker data | ||
| return null; | ||
| } | ||
|
|
||
| // Step 3: Find the position of our specific marker within this row's data | ||
| let markerTimingIndex = -1; | ||
| for (let i = 0; i < markerTiming.length; i++) { | ||
| if (markerTiming.index[i] === selectedMarkerIndex) { | ||
| markerTimingIndex = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (markerTimingIndex === -1) { | ||
| return null; | ||
| } | ||
|
|
||
| // Step 4: Calculate horizontal (X) position | ||
| const startTimestamp = markerTiming.start[markerTimingIndex]; | ||
| const endTimestamp = markerTiming.end[markerTimingIndex]; | ||
|
|
||
| const markerContainerWidth = containerWidth - marginLeft - marginRight; | ||
| const rangeLength: Milliseconds = rangeEnd - rangeStart; | ||
| const viewportLength: UnitIntervalOfProfileRange = | ||
| viewportRight - viewportLeft; | ||
| const startTime: UnitIntervalOfProfileRange = | ||
| (startTimestamp - rangeStart) / rangeLength; | ||
| const endTime: UnitIntervalOfProfileRange = | ||
| (endTimestamp - rangeStart) / rangeLength; | ||
|
|
||
| const x: CssPixels = | ||
| ((startTime - viewportLeft) * markerContainerWidth) / viewportLength + | ||
| marginLeft; | ||
| const w: CssPixels = | ||
| ((endTime - startTime) * markerContainerWidth) / viewportLength; | ||
|
|
||
| // For instant markers (start === end), use the center point. | ||
| // For interval markers, use a point 1/3 into the marker (or 30px, whichever is smaller). | ||
| const isInstantMarker = startTimestamp === endTimestamp; | ||
| const offsetX = isInstantMarker ? x : x + Math.min(w / 3, 30); | ||
|
|
||
| // Step 5: Calculate vertical (Y) position | ||
| // + 5 offsets the tooltip slightly below the row's top edge. | ||
| const offsetY: CssPixels = rowIndex * rowHeight - viewportTop + 5; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, what's
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| if (offsetY < 0 || offsetY > containerHeight) { | ||
| return null; | ||
| } | ||
|
|
||
| return { offsetX, offsetY }; | ||
| } | ||
|
|
||
| override render() { | ||
| const { containerWidth, containerHeight, isDragging } = this.props.viewport; | ||
| const { selectedMarkerIndex } = this.props; | ||
|
|
||
| return ( | ||
| <ChartCanvas | ||
|
|
@@ -976,6 +1111,8 @@ class MarkerChartCanvasImpl extends React.PureComponent<Props> { | |
| onMouseMove={this.onMouseMove} | ||
| onMouseLeave={this.onMouseLeave} | ||
| stickyTooltips={true} | ||
| selectedItem={selectedMarkerIndex} | ||
| selectedItemTooltipOffset={this._getSelectedItemTooltipOffset()} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,14 @@ type Props<Item> = { | |
| readonly onMouseLeave?: (e: { nativeEvent: MouseEvent }) => unknown; | ||
| // Defaults to false. Set to true if the chart should persist the tooltips on click. | ||
| readonly stickyTooltips?: boolean; | ||
| // Selected item coming from URL or other non-click mechanism. | ||
| readonly selectedItem?: Item | null; | ||
| // Pre-computed canvas-relative offset for the tooltip of the selected item | ||
| // (null when the item is not currently visible in the viewport). | ||
| readonly selectedItemTooltipOffset?: { | ||
| offsetX: CssPixels; | ||
| offsetY: CssPixels; | ||
| } | null; | ||
| }; | ||
|
|
||
| // The naming of the X and Y coordinates here correspond to the ones | ||
|
|
@@ -359,6 +367,28 @@ export class ChartCanvas<Item> extends React.Component< | |
| this._canvas = canvas; | ||
| }; | ||
|
|
||
| _syncSelectedItemFromProp = () => { | ||
| const { selectedItem, selectedItemTooltipOffset } = this.props; | ||
|
|
||
| if (selectedItem === undefined || selectedItem === null) { | ||
| this.setState({ selectedItem: null }); | ||
| return; | ||
| } | ||
|
|
||
| if (!selectedItemTooltipOffset || !this._canvas) { | ||
| // Keep selectedItem set, so the tooltip stays at | ||
| // its last known position rather than disappearing. | ||
| this.setState({ selectedItem }); | ||
| return; | ||
| } | ||
|
|
||
| const { offsetX, offsetY } = selectedItemTooltipOffset; | ||
| const canvasRect = this._canvas.getBoundingClientRect(); | ||
| const pageX = canvasRect.left + window.scrollX + offsetX; | ||
| const pageY = canvasRect.top + window.scrollY + offsetY; | ||
| this.setState({ selectedItem, pageX, pageY }); | ||
| }; | ||
|
|
||
| override UNSAFE_componentWillReceiveProps() { | ||
| // It is possible that the data backing the chart has been | ||
| // changed, for instance after symbolication. Clear the | ||
|
|
@@ -377,6 +407,15 @@ export class ChartCanvas<Item> extends React.Component< | |
|
|
||
| override componentDidMount() { | ||
| window.addEventListener('profiler-theme-change', this._onThemeChange); | ||
|
|
||
| // If the viewport hasn't been laid out yet, | ||
| // componentDidUpdate will pick it up once it becomes non-zero. | ||
| if ( | ||
| this.props.selectedItem !== undefined && | ||
| this.props.containerWidth !== 0 | ||
| ) { | ||
| this._syncSelectedItemFromProp(); | ||
| } | ||
| } | ||
|
|
||
| override componentWillUnmount() { | ||
|
|
@@ -389,6 +428,37 @@ export class ChartCanvas<Item> extends React.Component< | |
|
|
||
| override componentDidUpdate(prevProps: Props<Item>, prevState: State<Item>) { | ||
| if (prevProps !== this.props) { | ||
| if (this.props.selectedItem !== undefined) { | ||
| const selectedItemChanged = | ||
| this.props.selectedItem !== prevProps.selectedItem; | ||
| const canvasJustGotSize = | ||
| prevProps.containerWidth === 0 && this.props.containerWidth !== 0; | ||
| const offsetChanged = | ||
| this.props.selectedItemTooltipOffset !== | ||
| prevProps.selectedItemTooltipOffset; | ||
|
|
||
| // The item was already set internally (eg, via click handler), | ||
| // so skip the prop-driven sync which would overwrite that position. | ||
| const alreadySetInternally = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, what's alreadySetInternally? Could be good to add a comment. |
||
| selectedItemChanged && | ||
| hoveredItemsAreEqual( | ||
| this.state.selectedItem, | ||
| this.props.selectedItem | ||
| ); | ||
|
|
||
| const offsetSyncRelevant = | ||
| offsetChanged && | ||
| !selectedItemChanged && | ||
| this.state.selectedItem !== null; | ||
|
|
||
| if ( | ||
| !alreadySetInternally && | ||
| (selectedItemChanged || canvasJustGotSize || offsetSyncRelevant) | ||
| ) { | ||
| this._syncSelectedItemFromProp(); | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| this.state.selectedItem !== null && | ||
| prevState.selectedItem === this.state.selectedItem | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the url version! It's good to do it.