From aebd38c9d9347a5092730f2523b3c0643e75bbac Mon Sep 17 00:00:00 2001 From: Anthony Hendrickx Date: Thu, 28 Nov 2024 11:21:52 +0100 Subject: [PATCH] [FIX] chart: fix misalignment with trend line axis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task Description When inserting a trend line on a line chart, depending on the data we sometimes have a misplaced trend line axis giving an alignment issue between the trend line and the original plot. For example, if you have these data : A1 : A, B1 : 1 A2 : 2, B2 : 2 A3 : 3, B3 : 3 A4 : 4, B4 : 4 A5 : 5, B5 : 5 A6 : 6, B6 : 6 A7 : 7, B7 : 7 A8 : 8, B8 : 8 When you try to insert a linear trend line, the line won't be aligned to the original line chart. This PR aims to fix this by changing the labels and axis of the trend line. Related Task closes odoo/o-spreadsheet#5282 Task: 4251695 Signed-off-by: Rémi Rahir (rar) --- src/helpers/figures/charts/chart_common.ts | 7 +- .../charts/chart_common_line_scatter.ts | 37 +++-- tests/figures/chart/chart_plugin.test.ts | 147 +++++------------- 3 files changed, 66 insertions(+), 125 deletions(-) diff --git a/src/helpers/figures/charts/chart_common.ts b/src/helpers/figures/charts/chart_common.ts index 7ed05dca34..da7c760e5c 100644 --- a/src/helpers/figures/charts/chart_common.ts +++ b/src/helpers/figures/charts/chart_common.ts @@ -490,13 +490,14 @@ export function getTrendDatasetForBarChart( if (!newValues.length) { return; } - return getFullTrendingLineDataSet(dataset, config, newValues); + return getFullTrendingLineDataSet(dataset, config, newValues, newLabels); } export function getFullTrendingLineDataSet( dataset: ChartDataset<"line" | "bar">, config: TrendConfiguration, - data: number[] + data: number[], + labels: number[] ) { const defaultBorderColor = colorToRGBA(dataset.backgroundColor as Color); defaultBorderColor.a = 1; @@ -508,7 +509,7 @@ export function getFullTrendingLineDataSet( xAxisID: TREND_LINE_XAXIS_ID, yAxisID: dataset.yAxisID, label: dataset.label ? _t("Trend line for %s", dataset.label) : "", - data, + data: data.map((v, i) => ({ x: labels[i], y: v })), order: -1, showLine: true, pointRadius: 0, diff --git a/src/helpers/figures/charts/chart_common_line_scatter.ts b/src/helpers/figures/charts/chart_common_line_scatter.ts index 023a8e1398..91d4647414 100644 --- a/src/helpers/figures/charts/chart_common_line_scatter.ts +++ b/src/helpers/figures/charts/chart_common_line_scatter.ts @@ -174,12 +174,12 @@ export function getTrendDatasetForLineChart( } const numberOfStep = 5 * labels.length; const step = (xmax - xmin) / numberOfStep; - const newLabels = range(xmin, xmax + step / 2, step); - const newValues = interpolateData(config, filteredValues, filteredLabels, newLabels); - if (!newValues.length) { + const trendLabels = range(xmin, xmax + step / 2, step); + const trendValues = interpolateData(config, filteredValues, filteredLabels, trendLabels); + if (!trendValues.length) { return; } - return getFullTrendingLineDataSet(dataset, config, newValues); + return getFullTrendingLineDataSet(dataset, config, trendValues, trendLabels); } export function createLineOrScatterChartRuntime( @@ -314,11 +314,15 @@ export function createLineOrScatterChartRuntime( config.options.scales!.x!.ticks!.callback = (value) => formatValue(value, { format: labelFormat, locale }); config.options.plugins!.tooltip!.callbacks!.label = (tooltipItem) => { - const dataSetPoint = dataSetsValues[tooltipItem.datasetIndex!].data![tooltipItem.dataIndex!]; - let label: string | number = - tooltipItem.dataset.xAxisID === TREND_LINE_XAXIS_ID - ? "" - : tooltipItem.label || labelValues.values[tooltipItem.dataIndex!]; + let dataSetPoint: number; + let label: string | number; + if (tooltipItem.dataset.xAxisID === TREND_LINE_XAXIS_ID) { + dataSetPoint = dataSetsValues[tooltipItem.datasetIndex!].data![tooltipItem.dataIndex!].y; + label = ""; + } else { + dataSetPoint = dataSetsValues[tooltipItem.datasetIndex!].data![tooltipItem.dataIndex!]; + label = labelValues.values[tooltipItem.dataIndex!]; + } if (isNumber(label, locale)) { label = toNumber(label, locale); } @@ -397,16 +401,19 @@ export function createLineOrScatterChartRuntime( } } if (trendDatasets.length) { - /* We add a second x axis here to draw the trend lines, with the labels length being - * set so that the second axis points match the classical x axis - */ config.options.scales[TREND_LINE_XAXIS_ID] = { ...xAxis, - type: "category", - labels: range(0, maxLength).map((x) => x.toString()), - offset: false, display: false, }; + if (axisType === "category") { + /* We add a second x axis here to draw the trend lines, with the labels length being + * set so that the second axis points match the classical x axis + */ + config.options.scales[TREND_LINE_XAXIS_ID]["labels"] = range(0, maxLength).map((x) => + x.toString() + ); + config.options.scales[TREND_LINE_XAXIS_ID]["offset"] = false; + } /* These datasets must be inserted after the original datasets to ensure the way we * distinguish the originals and trendLine datasets after */ diff --git a/tests/figures/chart/chart_plugin.test.ts b/tests/figures/chart/chart_plugin.test.ts index b09f851002..b4a5143df2 100644 --- a/tests/figures/chart/chart_plugin.test.ts +++ b/tests/figures/chart/chart_plugin.test.ts @@ -1,4 +1,4 @@ -import { ChartType as ChartJSType, TooltipItem } from "chart.js"; +import { ChartType as ChartJSType, Point, TooltipItem } from "chart.js"; import { CommandResult, Model } from "../../../src"; import { ChartDefinition } from "../../../src/types"; import { @@ -49,7 +49,8 @@ import { import { ChartTerms } from "../../../src/components/translations_terms"; import { FIGURE_ID_SPLITTER, MAX_CHAR_LABEL } from "../../../src/constants"; -import { range, toZone, zoneToXc } from "../../../src/helpers"; +import { toNumber } from "../../../src/functions/helpers"; +import { toZone, zoneToXc } from "../../../src/helpers"; import { BarChart } from "../../../src/helpers/figures/charts"; import { ChartPlugin } from "../../../src/plugins/core"; import { ScatterChartRuntime } from "../../../src/types/chart/scatter_chart"; @@ -3075,47 +3076,6 @@ test("trend line dataset are put after original dataset in the runtime", async ( expect(datasets[1]).toMatchObject({ label: "serie_2" }); }); -test("trend line with time axis use time values as labels", () => { - setFormat(model, "C2:C5", "m/d/yyyy"); - createChart( - model, - { - type: "line", - dataSets: [{ dataRange: "B2:B5", trend: { display: true, type: "polynomial", order: 2 } }], - labelRange: "C2:C5", - labelsAsText: false, - }, - "1" - ); - let config = getChartConfiguration(model, "1"); - expect(config.options?.scales?.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 16).map((v) => v.toString()), - }); -}); - -test("trend line with categorical axis use values as labels", () => { - createChart( - model, - { - type: "line", - dataSets: [{ dataRange: "B2:B5", trend: { display: true, type: "polynomial", order: 2 } }], - labelRange: "C2:C5", - labelsAsText: false, - }, - "1" - ); - const config = getChartConfiguration(model, "1"); - expect(config.options?.scales?.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 16).map((v) => v.toString()), - }); -}); - describe("trending line", () => { beforeEach(() => { setGrid(model, { @@ -3142,59 +3102,42 @@ describe("trending line", () => { "1" ); }); + test("trend line works with numerical values as labels", () => { - const config = getChartConfiguration(model, "1"); - expect(config.options.scales.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 26).map((v) => v.toString()), - }); - const runtime = model.getters.getChartRuntime("1") as LineChartRuntime; const step = (6 - 1) / 25; - const data = runtime.chartJsConfig.data.datasets[1].data; + const data = getChartConfiguration(model, "1").data.datasets[1].data; for (let i = 0; i < data.length; i++) { - const value = data[i]; - const expectedValue = Math.pow(1 + i * step, 2); - expect(value).toBeCloseTo(expectedValue); + const value = data[i] as Point; + const expectedLabel = 1 + i * step; + const expectedValue = Math.pow(expectedLabel, 2); + expect(value.x).toEqual(expectedLabel); + expect(value.y).toBeCloseTo(expectedValue); } }); test("trend line works with datetime values as labels", () => { setFormat(model, "C1:C5", "m/d/yyyy"); mockChart(); - const config = getChartConfiguration(model, "1"); - expect(config.options.scales.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 26).map((v) => v.toString()), - }); - const runtime = model.getters.getChartRuntime("1") as LineChartRuntime; const step = (6 - 1) / 25; - const data = runtime.chartJsConfig.data.datasets[1].data; + const data = getChartConfiguration(model, "1").data.datasets[1].data; for (let i = 0; i < data.length; i++) { - const value = data[i]; - const expectedValue = Math.pow(1 + i * step, 2); - expect(value).toBeCloseTo(expectedValue); + const value = data[i] as Point; + const expectedLabel = 1 + i * step; + const expectedValue = Math.pow(expectedLabel, 2); + expect(value.x).toEqual(expectedLabel); + expect(value.y).toBeCloseTo(expectedValue); } }); test("trend line works with categorical values as labels", () => { - const config = getChartConfiguration(model, "1"); - expect(config.options.scales.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 26).map((v) => v.toString()), - }); - const runtime = model.getters.getChartRuntime("1") as LineChartRuntime; const step = (6 - 1) / 25; - const data = runtime.chartJsConfig.data.datasets[1].data; + const data = getChartConfiguration(model, "1").data.datasets[1].data; for (let i = 0; i < data.length; i++) { - const value = data[i]; - const expectedValue = Math.pow(1 + i * step, 2); - expect(value).toBeCloseTo(expectedValue); + const value = data[i] as Point; + const expectedLabel = 1 + i * step; + const expectedValue = Math.pow(expectedLabel, 2); + expect(value.x).toEqual(expectedLabel); + expect(value.y).toBeCloseTo(expectedValue); } }); @@ -3211,20 +3154,14 @@ describe("trending line", () => { dataSets: [{ dataRange: "B1:B10", trend: { display: true, type: "polynomial", order: 2 } }], labelRange: "C1:C10", }); - const config = getChartConfiguration(model, "1"); - expect(config.options.scales.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 51).map((v) => v.toString()), - }); - const runtime = model.getters.getChartRuntime("1") as LineChartRuntime; const step = (10 - 1) / 50; - const data = runtime.chartJsConfig.data.datasets[1].data; + const data = getChartConfiguration(model, "1").data.datasets[1].data; for (let i = 0; i < data.length; i++) { - const value = data[i]; - const expectedValue = Math.pow(1 + i * step, 2); - expect(value).toBeCloseTo(expectedValue); + const value = data[i] as Point; + const expectedLabel = 1 + i * step; + const expectedValue = Math.pow(expectedLabel, 2); + expect(value.x).toEqual(expectedLabel); + expect(value.y).toBeCloseTo(expectedValue); } }); @@ -3255,21 +3192,15 @@ describe("trending line", () => { B5: "36", C5: "1/12/2024", }); - const config = getChartConfiguration(model, "1"); - expect(config.options.scales.x1).toMatchObject({ - type: "category", - display: false, - offset: false, - labels: range(0, 26).map((v) => v.toString()), - }); - const runtime = model.getters.getChartRuntime("1") as LineChartRuntime; const step = (6 - 1) / 25; - //@ts-ignore - const data = runtime.chartJsConfig.data.datasets[1].data; + const data = getChartConfiguration(model, "1").data.datasets[1].data; + const initialValue = toNumber("1/7/2024", model.getters.getLocale()) - 1; for (let i = 0; i < data.length; i++) { - const value = data[i]; - const expectedValue = Math.pow(1 + i * step, 2); - expect(value).toBeCloseTo(expectedValue); + const value = data[i] as Point; + const expectedLabel = 1 + i * step; + const expectedValue = Math.pow(expectedLabel, 2); + expect(value.x).toEqual(initialValue + expectedLabel); + expect(value.y).toBeCloseTo(expectedValue); } }); @@ -3320,9 +3251,11 @@ describe("trending line", () => { const data = runtime.chartJsConfig.data.datasets[1]?.data; const step = (4 - 1) / (data.length - 1); for (let i = 0; i < data.length; i++) { - const value = data[i]; - const expectedValue = Math.pow(1 + i * step, 2); - expect(value).toBeCloseTo(expectedValue); + const value = data[i] as Point; + const expectedLabel = 1 + i * step; + const expectedValue = Math.pow(expectedLabel, 2); + expect(value.x).toEqual(expectedLabel); + expect(value.y).toBeCloseTo(expectedValue); } });