feat: Allow "Even X-Axis Spacing" to be set via URL parameter This commit enhances the "Even X-Axis Spacing" feature in perf: - Moves the `evenXAxisSpacing` property into the `State` class, allowing it to be (de)serialized from the URL. - If the `evenXAxisSpacing` parameter is not present in the URL, the setting will default to the value stored in localStorage. - Ensures the toggle state is synchronized across multiple graphs in `ExploreMultiSk` and `ReportPageSk`. - New graphs added in `ExploreMultiSk` will now inherit the current `evenXAxisSpacing` state. - Updated unit tests to verify URL parameter handling and state persistence. This allows users to share links with a specific x-axis spacing preference. Bug: 418808644 Change-Id: I3e5c6a7b8f9d0e1f2a3b4c5d6e7f8a9b0c1d2e3f Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1142008 Commit-Queue: Eduardo Yap <eduardoyap@google.com> Reviewed-by: Farid (Mojtaba) Faridzad <faridzad@google.com>
diff --git a/perf/modules/explore-multi-sk/explore-multi-sk.ts b/perf/modules/explore-multi-sk/explore-multi-sk.ts index a0264c0..802df63 100644 --- a/perf/modules/explore-multi-sk/explore-multi-sk.ts +++ b/perf/modules/explore-multi-sk/explore-multi-sk.ts
@@ -112,6 +112,8 @@ dateAxis: boolean = false; + evenXAxisSpacing: boolean = false; + // TODO(eduardoyap): Handle browser history changes correctly in manual_plot_mode. // TODO(eduardoyap): Ensure new graphs in manual_plot_mode sync time ranges. manual_plot_mode: boolean = false; @@ -743,11 +745,15 @@ // It will sync the state across all the graphs. private _onEvenXAxisSpacingChanged = (e: Event) => { const detail = (e as CustomEvent).detail; + this.state.evenXAxisSpacing = detail.value; this.exploreElements.forEach((elem) => { if (elem !== e.target) { elem.setUseDiscreteAxis(detail.value); } }); + if (this.stateHasChanged) { + this.stateHasChanged(); + } }; constructor() { @@ -1522,6 +1528,7 @@ hide_paramset: true, graph_index: index, doNotQueryData: doNotQueryData, + evenXAxisSpacing: this.state.evenXAxisSpacing, }; explore.state = newState; }
diff --git a/perf/modules/explore-multi-sk/explore-multi-sk_test.ts b/perf/modules/explore-multi-sk/explore-multi-sk_test.ts index b6bdb4c..8c8e05d 100644 --- a/perf/modules/explore-multi-sk/explore-multi-sk_test.ts +++ b/perf/modules/explore-multi-sk/explore-multi-sk_test.ts
@@ -807,9 +807,17 @@ }); graph1.dispatchEvent(event); + assert.isTrue(element.state.evenXAxisSpacing, 'MultiSk state should be updated'); assert.isTrue(spy1.notCalled, 'Source graph setUseDiscreteAxis should not be called by sync'); assert.isTrue(spy2.calledOnceWith(true), 'Target graph setUseDiscreteAxis should be called'); - assert.isTrue(graph2.evenXAxisSpacing, 'Target graph state should be updated'); + assert.isTrue(graph2.state.evenXAxisSpacing, 'Target graph state should be updated'); + }); + + it('initializes new graphs with the current enableDiscrete state', () => { + element.state.evenXAxisSpacing = true; + const newGraph = element['addEmptyGraph']()!; + element['addStateToExplore'](newGraph, new GraphConfig(), false); + assert.isTrue(newGraph.state.evenXAxisSpacing); }); }); @@ -901,7 +909,8 @@ hide_paramset: false, horizontal_zoom: false, graph_index: 0, - doNotQueryData: true, + doNotQueryData: false, + evenXAxisSpacing: false, }; element['addStateToExplore'](simpleSk, new GraphConfig(), false);
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk.ts b/perf/modules/explore-simple-sk/explore-simple-sk.ts index 5b1a012..0503875 100644 --- a/perf/modules/explore-simple-sk/explore-simple-sk.ts +++ b/perf/modules/explore-simple-sk/explore-simple-sk.ts
@@ -368,6 +368,9 @@ // boolean indicating if the element should disable querying of data from backend. doNotQueryData: boolean = false; + + // boolean indicating if the chart should space x-axis points evenly, treating them as categories. + evenXAxisSpacing: boolean = false; } // TODO(jcgregorio) Move to a 'key' module. @@ -644,12 +647,6 @@ private readonly uniqueId = `${ExploreSimpleSk.nextUniqueId++}`; - private _evenXAxisSpacing: boolean = false; - - public get evenXAxisSpacing(): boolean { - return this._evenXAxisSpacing; - } - public get dataLoading(): boolean { return this.dfRepo.value?.loading ?? false; } @@ -769,7 +766,7 @@ <md-switch form="form" id="even-x-axis-spacing-switch" - ?selected="${ele.evenXAxisSpacing}" + ?selected="${ele._state.evenXAxisSpacing}" @change=${(e: InputEvent) => ele.switchEvenXAxisSpacing(e.target as MdSwitch)}></md-switch> Even X-Axis Spacing @@ -786,7 +783,7 @@ .domain=${ele._state.domain} ${ref(ele.googleChartPlot)} .highlightAnomalies=${ele._state.highlight_anomalies} - .useDiscreteAxis=${ele.evenXAxisSpacing} + .useDiscreteAxis=${ele._state.evenXAxisSpacing} @plot-data-select=${ele.onChartSelect} @plot-data-mouseover=${ele.onChartOver} @plot-data-mousedown=${ele.onChartMouseDown} @@ -1247,8 +1244,11 @@ } const cachedEvenSpacing = localStorage.getItem(CACHE_KEY_EVEN_X_AXIS_SPACING); - this._evenXAxisSpacing = cachedEvenSpacing === 'true'; - this.setUseDiscreteAxis(this._evenXAxisSpacing); + if (cachedEvenSpacing === 'true') { + this._state.evenXAxisSpacing = true; + } + + this.setUseDiscreteAxis(this._state.evenXAxisSpacing); this._initialized = true; this.render(); @@ -1626,26 +1626,26 @@ private switchEvenXAxisSpacing(target: MdSwitch | null) { if (!target) return; - const newValue = target.selected; + this._state.evenXAxisSpacing = target.selected; - localStorage.setItem(CACHE_KEY_EVEN_X_AXIS_SPACING, String(newValue)); + localStorage.setItem(CACHE_KEY_EVEN_X_AXIS_SPACING, String(this._state.evenXAxisSpacing)); - this._evenXAxisSpacing = newValue; - this.setUseDiscreteAxis(this._evenXAxisSpacing); + this.setUseDiscreteAxis(this._state.evenXAxisSpacing); + this.render(); this.dispatchEvent( new CustomEvent('even-x-axis-spacing-changed', { - detail: { value: newValue, graph_index: this._state.graph_index }, + detail: { value: this._state.evenXAxisSpacing, graph_index: this._state.graph_index }, bubbles: true, }) ); + this._stateHasChanged(); } public setUseDiscreteAxis(value: boolean) { - this._evenXAxisSpacing = value; + this._state.evenXAxisSpacing = value; if (this.googleChartPlot.value) { this.googleChartPlot.value.useDiscreteAxis = value; } - this.render(); } private closeQueryDialog(): void { @@ -3904,8 +3904,9 @@ state = this.rationalizeTimeRange(state); const cachedEvenSpacing = localStorage.getItem(CACHE_KEY_EVEN_X_AXIS_SPACING); - this._evenXAxisSpacing = cachedEvenSpacing === 'true'; - + if (cachedEvenSpacing === 'true') { + state.evenXAxisSpacing = true; + } this._state = state; // Synchronize the xAxisSwitch with the state's domain
diff --git a/perf/modules/explore-simple-sk/explore-simple-sk_test.ts b/perf/modules/explore-simple-sk/explore-simple-sk_test.ts index 894db31..456b0c3 100644 --- a/perf/modules/explore-simple-sk/explore-simple-sk_test.ts +++ b/perf/modules/explore-simple-sk/explore-simple-sk_test.ts
@@ -166,7 +166,7 @@ const newExplore = setUpElementUnderTest<ExploreSimpleSk>('explore-simple-sk')(); await waitForRender(newExplore); - assert.isTrue(newExplore.evenXAxisSpacing); + assert.isTrue(newExplore.state.evenXAxisSpacing); const switchEl = newExplore.querySelector( '#settings-dialog #even-x-axis-spacing-switch' ) as MdSwitch; @@ -196,7 +196,7 @@ explore.state = newState; await waitForRender(explore); - assert.isTrue(explore.evenXAxisSpacing); // Should still be true due to cache + assert.isTrue(explore.state.evenXAxisSpacing); // Should still be true due to cache const switchEl = explore.querySelector( '#settings-dialog #even-x-axis-spacing-switch' ) as MdSwitch; @@ -204,7 +204,7 @@ }); it('should default to false if no value in localStorage', async () => { - assert.isFalse(explore.evenXAxisSpacing); + assert.isFalse(explore.state.evenXAxisSpacing); const switchEl = explore.querySelector( '#settings-dialog #even-x-axis-spacing-switch' ) as MdSwitch; @@ -1448,7 +1448,7 @@ it('should be unchecked by default', () => { assert.isFalse(switchEl.selected); - assert.isFalse(explore.evenXAxisSpacing); + assert.isFalse(explore.state.evenXAxisSpacing); }); it('should update state and fire event when toggled on', async () => { @@ -1458,7 +1458,7 @@ await waitForRender(explore); await new Promise((resolve) => setTimeout(resolve, 0)); // Add delay - assert.isTrue(explore.evenXAxisSpacing); + assert.isTrue(explore.state.evenXAxisSpacing); assert.isTrue(eventSpy.calledOnce); const event = eventSpy.firstCall.args[0]; @@ -1483,7 +1483,7 @@ await waitForRender(explore); - assert.isFalse(explore.evenXAxisSpacing); + assert.isFalse(explore.state.evenXAxisSpacing); assert.isTrue(eventSpy.calledOnce); const event = eventSpy.firstCall.args[0];
diff --git a/perf/modules/report-page-sk/report-page-sk.ts b/perf/modules/report-page-sk/report-page-sk.ts index c80c565..0708722 100644 --- a/perf/modules/report-page-sk/report-page-sk.ts +++ b/perf/modules/report-page-sk/report-page-sk.ts
@@ -512,6 +512,7 @@ const graph = graphNode as ExploreSimpleSk; // Skip graph that sent the event. if (graph.state.graph_index !== e.detail.graph_index) { + graph.state.evenXAxisSpacing = newValue; graph.setUseDiscreteAxis(newValue); graph.render(); }