From 8fad5af4811fe37f26b3b9e79adb86ad26e06761 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Thu, 20 Nov 2025 13:26:08 +0100 Subject: [PATCH 1/3] [REF] charts: dataset with { value, format } `data` was typed as `any`. This is not a good thing in itself. Moreover, it was only the values without the format. Also typeof is extremely permissive --- .../src/helpers/cells/cell_evaluation.ts | 42 +++++ .../cell_evaluation/evaluation_plugin.ts | 4 +- .../src/types/chart/chart.ts | 4 +- src/helpers/figures/charts/pyramid_chart.ts | 7 +- .../charts/runtime/chart_data_extractor.ts | 154 ++++++++++-------- .../figures/charts/runtime/chartjs_dataset.ts | 65 +++++--- .../figures/charts/runtime/chartjs_scales.ts | 30 +++- .../charts/runtime/chartjs_show_values.ts | 10 +- tests/figures/chart/chart_plugin.test.ts | 49 +++--- tests/test_helpers/getters_helpers.ts | 4 +- 10 files changed, 240 insertions(+), 129 deletions(-) diff --git a/packages/o-spreadsheet-engine/src/helpers/cells/cell_evaluation.ts b/packages/o-spreadsheet-engine/src/helpers/cells/cell_evaluation.ts index ef258f3399..524e70f55e 100644 --- a/packages/o-spreadsheet-engine/src/helpers/cells/cell_evaluation.ts +++ b/packages/o-spreadsheet-engine/src/helpers/cells/cell_evaluation.ts @@ -129,6 +129,48 @@ function _createEvaluatedCell( return textCell(value, format, formattedValue); } +export function isNumberCell( + result: FunctionResultObject | undefined +): result is { value: number } { + return !!result && getEvaluatedCellType(result) === CellValueType.number; +} + +export function isTextCell(result: FunctionResultObject | undefined): result is { value: string } { + return !!result && getEvaluatedCellType(result) === CellValueType.text; +} + +export function isBooleanCell( + result: FunctionResultObject | undefined +): result is { value: boolean } { + return !!result && getEvaluatedCellType(result) === CellValueType.boolean; +} + +export function isEmptyCell(result: FunctionResultObject | undefined): result is { value: null } { + return !!result && getEvaluatedCellType(result) === CellValueType.empty; +} + +export function isErrorCell(result: FunctionResultObject | undefined): result is { value: string } { + return !!result && getEvaluatedCellType(result) === CellValueType.error; +} + +function getEvaluatedCellType({ value, format }: FunctionResultObject): CellValueType { + if (value === null) { + return CellValueType.empty; + } else if (isEvaluationError(value)) { + return CellValueType.error; + } else if (isTextFormat(format)) { + return CellValueType.text; + } + switch (typeof value) { + case "number": + return CellValueType.number; + case "boolean": + return CellValueType.boolean; + case "string": + return CellValueType.text; + } +} + function textCell( value: string, format: string | undefined, diff --git a/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts b/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts index 7eec2e36ba..c789b3ad66 100644 --- a/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts +++ b/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts @@ -259,10 +259,10 @@ export class EvaluationPlugin extends CoreViewPlugin { /** * Return the value of each cell in the range. */ - getRangeValues(range: Range): CellValue[] { + getRangeValues(range: Range): EvaluatedCell[] { const sheet = this.getters.tryGetSheet(range.sheetId); if (sheet === undefined) return []; - return this.mapVisiblePositions(range, (p) => this.getters.getEvaluatedCell(p).value); + return this.mapVisiblePositions(range, (p) => this.getters.getEvaluatedCell(p)); } /** diff --git a/packages/o-spreadsheet-engine/src/types/chart/chart.ts b/packages/o-spreadsheet-engine/src/types/chart/chart.ts index d5aa66502e..aca319ad21 100644 --- a/packages/o-spreadsheet-engine/src/types/chart/chart.ts +++ b/packages/o-spreadsheet-engine/src/types/chart/chart.ts @@ -21,7 +21,7 @@ import { } from "./tree_map_chart"; import { WaterfallChartDefinition, WaterfallChartRuntime } from "./waterfall_chart"; -import { Align, Color, VerticalAlign } from "../.."; +import { Align, Color, FunctionResultObject, VerticalAlign } from "../.."; import { COLORSCHEMES } from "../../helpers/color"; import { Format } from "../format"; import { Locale } from "../locale"; @@ -104,7 +104,7 @@ export interface LabelValues { export interface DatasetValues { readonly label?: string; - readonly data: any[]; + readonly data: FunctionResultObject[]; readonly hidden?: boolean; } diff --git a/src/helpers/figures/charts/pyramid_chart.ts b/src/helpers/figures/charts/pyramid_chart.ts index beb742033d..cc0d615f89 100644 --- a/src/helpers/figures/charts/pyramid_chart.ts +++ b/src/helpers/figures/charts/pyramid_chart.ts @@ -1,5 +1,6 @@ import { CoreGetters, Validator } from "@odoo/o-spreadsheet-engine"; import { BACKGROUND_CHART_COLOR } from "@odoo/o-spreadsheet-engine/constants"; +import { isNumberCell } from "@odoo/o-spreadsheet-engine/helpers/cells/cell_evaluation"; import { AbstractChart } from "@odoo/o-spreadsheet-engine/helpers/figures/charts/abstract_chart"; import { chartFontColor, @@ -197,7 +198,11 @@ export class PyramidChart extends AbstractChart { const chartData = getPyramidChartData(definition, this.dataSets, this.labelRange, getters); const { dataSetsValues } = chartData; const maxValue = Math.max( - ...dataSetsValues.map((dataSet) => Math.max(...dataSet.data.map(Math.abs))) + ...dataSetsValues.map((dataSet) => + Math.max( + ...dataSet.data.map((cell) => (isNumberCell(cell) ? Math.abs(cell.value) : -Infinity)) + ) + ) ); return { ...definition, diff --git a/src/helpers/figures/charts/runtime/chart_data_extractor.ts b/src/helpers/figures/charts/runtime/chart_data_extractor.ts index acaef099fd..92ba1f1fe1 100644 --- a/src/helpers/figures/charts/runtime/chart_data_extractor.ts +++ b/src/helpers/figures/charts/runtime/chart_data_extractor.ts @@ -1,4 +1,11 @@ -import { _t, deepCopy, findNextDefinedValue, isNumber, range } from "@odoo/o-spreadsheet-engine"; +import { + _t, + deepCopy, + DEFAULT_LOCALE, + findNextDefinedValue, + isNumber, + range, +} from "@odoo/o-spreadsheet-engine"; import { ChartTerms } from "@odoo/o-spreadsheet-engine/components/translations_terms"; import { evaluatePolynomial, @@ -8,7 +15,12 @@ import { polynomialRegression, predictLinearValues, } from "@odoo/o-spreadsheet-engine/functions/helper_statistical"; -import { isEvaluationError, toNumber } from "@odoo/o-spreadsheet-engine/functions/helpers"; +import { toNumber } from "@odoo/o-spreadsheet-engine/functions/helpers"; +import { + isErrorCell, + isNumberCell, + isTextCell, +} from "@odoo/o-spreadsheet-engine/helpers/cells/cell_evaluation"; import { shouldRemoveFirstLabel } from "@odoo/o-spreadsheet-engine/helpers/figures/charts/chart_common"; import { DAYS, isDateTimeFormat, MONTHS } from "@odoo/o-spreadsheet-engine/helpers/format/format"; import { createDate } from "@odoo/o-spreadsheet-engine/helpers/pivot/spreadsheet_pivot/date_spreadsheet_pivot"; @@ -41,8 +53,9 @@ import { TreeMapChartDefinition } from "@odoo/o-spreadsheet-engine/types/chart/t import { Point } from "chart.js"; import { CellValue, - DEFAULT_LOCALE, + CellValueType, Format, + FunctionResultObject, GenericDefinition, Getters, Locale, @@ -50,6 +63,10 @@ import { } from "../../../../types"; import { timeFormatLuxonCompatible } from "../../../chart_date"; +const EMPTY = Object.freeze({ value: null }); +const ZERO = Object.freeze({ value: 0 }); +const ONE = Object.freeze({ value: 1 }); + export function getBarChartData( definition: GenericDefinition, dataSets: DataSet[], @@ -125,12 +142,12 @@ function getDateTimeLabel(value: number, stamp: CalendarChartGranularity): strin function computeValuesAndLabels( timeValues: CellValue[], - values: CellValue[], + values: FunctionResultObject[], horizontalGroupBy: CalendarChartGranularity, verticalGroupBy: CalendarChartGranularity, locale: Locale ) { - const grouping = {}; + const grouping: Record> = {}; const xValues: number[] = []; const yValues: number[] = []; const previousYValues: number[] = []; @@ -160,9 +177,12 @@ function computeValuesAndLabels( previousYValues.push(yValue); } if (!(yValue in grouping[xValue])) { - grouping[xValue][yValue] = 0; + grouping[xValue][yValue] = { value: 0 }; + } + const cell = values[i]; + if (isNumberCell(cell)) { + grouping[xValue][yValue].value += cell.value; } - grouping[xValue][yValue] += values[i]; } xValues.sort((a, b) => a - b); @@ -227,11 +247,15 @@ export function getPyramidChartData( const pyramidDatasetValues: DatasetValues[] = []; if (barDataset[0]) { - const pyramidData = barDataset[0].data.map((value) => (value > 0 ? value : 0)); + const pyramidData = barDataset[0].data.map((cell) => + isNumberCell(cell) && cell.value > 0 ? cell : ZERO + ); pyramidDatasetValues.push({ ...barDataset[0], data: pyramidData }); } if (barDataset[1]) { - const pyramidData = barDataset[1].data.map((value) => (value > 0 ? -value : 0)); + const pyramidData = barDataset[1].data.map((cell) => + isNumberCell(cell) && cell.value > 0 ? { value: -cell.value } : ZERO + ); pyramidDatasetValues.push({ ...barDataset[1], data: pyramidData }); } @@ -455,13 +479,17 @@ export function getHierarchalChartData( }; } -export function getTrendDatasetForBarChart(config: TrendConfiguration, data: any[]) { +export function getTrendDatasetForBarChart( + config: TrendConfiguration, + data: FunctionResultObject[] +) { const filteredValues: number[] = []; const filteredLabels: number[] = []; const labels: number[] = []; for (let i = 0; i < data.length; i++) { - if (typeof data[i] === "number") { - filteredValues.push(data[i]); + const cell = data[i]; + if (isNumberCell(cell)) { + filteredValues.push(cell.value); filteredLabels.push(i + 1); } labels.push(i + 1); @@ -474,7 +502,7 @@ export function getTrendDatasetForBarChart(config: TrendConfiguration, data: any export function getTrendDatasetForLineChart( config: TrendConfiguration, - data: any[], + data: FunctionResultObject[], labels: string[], axisType: AxisType, locale: Locale @@ -491,8 +519,9 @@ export function getTrendDatasetForLineChart( switch (axisType) { case "category": for (let i = 0; i < datasetLength; i++) { - if (typeof data[i] === "number") { - filteredValues.push(data[i]); + const cell = data[i]; + if (isNumberCell(cell)) { + filteredValues.push(cell.value); filteredLabels.push(i + 1); } trendLabels.push(i + 1); @@ -504,8 +533,9 @@ export function getTrendDatasetForLineChart( if (isNaN(label)) { continue; } - if (typeof data[i] === "number") { - filteredValues.push(data[i]); + const cell = data[i]; + if (isNumberCell(cell)) { + filteredValues.push(cell.value); filteredLabels.push(label); } trendLabels.push(label); @@ -514,8 +544,9 @@ export function getTrendDatasetForLineChart( case "time": for (let i = 0; i < data.length; i++) { const date = toNumber({ value: labels[i] }, locale); - if (data[i] !== null) { - filteredValues.push(data[i]); + const cell = data[i]; + if (isNumberCell(cell)) { + filteredValues.push(cell.value); filteredLabels.push(date); } trendLabels.push(date); @@ -710,10 +741,10 @@ function canBeLinearChart( labels.shift(); } - if (labels.some((label) => isNaN(Number(label)) && label)) { + if (labels.some((label) => label.type !== CellValueType.number && label.value)) { return false; } - if (labels.every((label) => !label)) { + if (labels.every((label) => !label.value)) { return false; } @@ -747,14 +778,14 @@ function keepOnlyPositiveValues( ...datasets.map((dataset) => dataset.data?.length || 0) ); const filteredIndexes = range(0, numberOfDataPoints).filter((i) => - datasets.some((ds) => typeof ds.data[i] === "number" && ds.data[i] > 0) + datasets.some((ds) => isNumberCell(ds.data[i]) && ds.data[i].value > 0) ); return { labels: filteredIndexes.map((i) => labels[i] || ""), dataSetsValues: datasets.map((ds) => ({ ...ds, data: filteredIndexes.map((i) => - typeof ds.data[i] === "number" && ds.data[i] > 0 ? ds.data[i] : null + isNumberCell(ds.data[i]) && ds.data[i].value > 0 ? ds.data[i] : EMPTY ), })), }; @@ -773,7 +804,7 @@ function fixEmptyLabelsForDateCharts( if (!newLabels[i]) { newLabels[i] = findNextDefinedValue(newLabels, i); for (const ds of newDatasets) { - ds.data[i] = undefined; + ds.data[i] = EMPTY; } } } @@ -783,7 +814,7 @@ function fixEmptyLabelsForDateCharts( /** * Get the data from a dataSet */ -export function getData(getters: Getters, ds: DataSet): (CellValue | undefined)[] { +export function getData(getters: Getters, ds: DataSet): FunctionResultObject[] { if (ds.dataRange) { const labelCellZone = ds.labelCell ? [ds.labelCell.zone] : []; const dataZone = recomputeZones([ds.dataRange.zone], labelCellZone)[0]; @@ -791,7 +822,7 @@ export function getData(getters: Getters, ds: DataSet): (CellValue | undefined)[ return []; } const dataRange = getters.getRangeFromZone(ds.dataRange.sheetId, dataZone); - return getters.getRangeValues(dataRange).map((value) => (value === "" ? undefined : value)); + return getters.getRangeValues(dataRange).map((cell) => (cell.value === "" ? EMPTY : cell)); } return []; } @@ -812,15 +843,13 @@ function filterInvalidDataPoints( const dataPointsIndexes = range(0, numberOfDataPoints).filter((dataPointIndex) => { const label = labels[dataPointIndex]; const values = datasets.map((dataset) => dataset.data?.[dataPointIndex]); - return label || values.some((value) => typeof value === "number"); + return label || values.some(isNumberCell); }); return { labels: dataPointsIndexes.map((i) => labels[i] || ""), dataSetsValues: datasets.map((dataset) => ({ ...dataset, - data: dataPointsIndexes.map((i) => - typeof dataset.data[i] === "number" ? dataset.data[i] : null - ), + data: dataPointsIndexes.map((i) => (isNumberCell(dataset.data[i]) ? dataset.data[i] : EMPTY)), })), }; } @@ -842,15 +871,13 @@ function filterInvalidCalendarDataPoints( const dataPointsIndexes = range(0, numberOfDataPoints).filter((dataPointIndex) => { const label = labels[dataPointIndex]; const values = datasets.map((dataset) => dataset.data?.[dataPointIndex]); - return label && isNumber(label, DEFAULT_LOCALE) && typeof values[0] === "number"; + return label && isNumber(label, DEFAULT_LOCALE) && isNumberCell(values[0]); }); return { labels: dataPointsIndexes.map((i) => labels[i] || ""), dataSetsValues: datasets.map((dataset) => ({ ...dataset, - data: dataPointsIndexes.map((i) => - typeof dataset.data[i] === "number" ? dataset.data[i] : null - ), + data: dataPointsIndexes.map((i) => (isNumberCell(dataset.data[i]) ? dataset.data[i] : EMPTY)), })), }; } @@ -866,17 +893,17 @@ function filterInvalidHierarchicalPoints( values.length, ...hierarchy.map((dataset) => dataset.data?.length || 0) ); - const isEmpty = (value: CellValue) => value === undefined || value === null || value === ""; + const isEmpty = (value: CellValue) => value === null || value === ""; const dataPointsIndexes = range(0, numberOfDataPoints).filter((dataPointIndex) => { const groups = hierarchy.map((dataset) => dataset.data?.[dataPointIndex]); - if (isEmpty(groups[0])) { + if (isEmpty(groups[0]?.value)) { return false; } // Filter points with empty group in the middle let hasFoundEmptyGroup = false; for (const group of groups) { - hasFoundEmptyGroup ||= isEmpty(group); - if (hasFoundEmptyGroup && !isEmpty(group)) { + hasFoundEmptyGroup ||= isEmpty(group?.value); + if (hasFoundEmptyGroup && !isEmpty(group?.value)) { return false; } } @@ -925,7 +952,7 @@ function aggregateDataForLabels( labels: string[], datasets: DatasetValues[] ): { labels: string[]; dataSetsValues: DatasetValues[] } { - const parseNumber = (value) => (typeof value === "number" ? value : 0); + const parseNumber = (value: CellValue) => (typeof value === "number" ? value : 0); const labelSet = new Set(labels); const labelMap: { [key: string]: number[] } = {}; labelSet.forEach((label) => { @@ -935,7 +962,9 @@ function aggregateDataForLabels( for (const indexOfLabel of range(0, labels.length)) { const label = labels[indexOfLabel]; for (const indexOfDataset of range(0, datasets.length)) { - labelMap[label][indexOfDataset] += parseNumber(datasets[indexOfDataset].data[indexOfLabel]); + labelMap[label][indexOfDataset] += parseNumber( + datasets[indexOfDataset].data[indexOfLabel]?.value + ); } } @@ -943,7 +972,7 @@ function aggregateDataForLabels( labels: Array.from(labelSet), dataSetsValues: datasets.map((dataset, indexOfDataset) => ({ ...dataset, - data: Array.from(labelSet).map((label) => labelMap[label][indexOfDataset]), + data: Array.from(labelSet).map((label) => ({ value: labelMap[label][indexOfDataset] })), })), }; } @@ -982,7 +1011,7 @@ function getChartLabelValues( ) { labels = { formattedValues: getters.getRangeFormattedValues(labelRange), - values: getters.getRangeValues(labelRange).map((val) => String(val ?? "")), + values: getters.getRangeValues(labelRange).map(({ value }) => String(value ?? "")), }; } else if (dataSets[0]) { const ranges = getData(getters, dataSets[0]); @@ -1042,16 +1071,12 @@ function getChartDatasetValues(getters: Getters, dataSets: DataSet[]): DatasetVa let data = ds.dataRange ? getData(getters, ds) : []; if ( - data.every((e) => !e || (typeof e === "string" && !isEvaluationError(e))) && - data.filter((e) => typeof e === "string").length > 1 + data.every((cell) => !cell.value || isTextCell(cell)) && + data.filter(isTextCell).length > 1 ) { // Convert categorical data into counts - data = data.map((e) => (e && !isEvaluationError(e) ? 1 : null)); - } else if ( - data.every( - (cell) => cell === undefined || cell === null || !isNumber(cell.toString(), DEFAULT_LOCALE) - ) - ) { + data = data.map((cell) => (!isErrorCell(cell) ? ONE : EMPTY)); + } else if (data.every((cell) => !isNumberCell(cell))) { hidden = true; } datasetValues.push({ data, label, hidden }); @@ -1083,24 +1108,24 @@ function getHierarchicalDatasetValues(getters: Getters, dataSets: DataSet[]): Da } const minLength = Math.min(...dataSetsData.map((ds) => ds.length)); - let currentValues: (CellValue | undefined)[] = []; + let currentValues: FunctionResultObject[] = []; const leafDatasetIndex = dataSets.length - 1; for (let i = 0; i < minLength; i++) { for (let dsIndex = 0; dsIndex < dataSetsData.length; dsIndex++) { - let value = dataSetsData[dsIndex][i]; - if ((value === undefined || value === null) && dsIndex !== leafDatasetIndex) { - value = currentValues[dsIndex]; + let cell = dataSetsData[dsIndex][i]; + if ((cell === undefined || cell.value === null) && dsIndex !== leafDatasetIndex) { + cell = currentValues[dsIndex]; } - if (value !== currentValues[dsIndex]) { + if (cell?.value !== currentValues[dsIndex]?.value) { currentValues = currentValues.slice(0, dsIndex); - currentValues[dsIndex] = value; + currentValues[dsIndex] = cell; } - datasetValues[dsIndex].data.push(value ?? null); + datasetValues[dsIndex].data.push(cell ?? EMPTY); } } - return datasetValues.filter((ds) => ds.data.some((d) => d !== null)); + return datasetValues.filter((ds) => ds.data.some((d) => d.value !== null)); } export function makeDatasetsCumulative( @@ -1108,16 +1133,17 @@ export function makeDatasetsCumulative( order: "asc" | "desc" ): DatasetValues[] { return datasets.map((dataset) => { - const data: number[] = []; + const data: { value: number | null; format?: Format }[] = []; let accumulator = 0; const indexes = - order === "asc" ? Object.keys(dataset.data) : Object.keys(dataset.data).reverse(); + order === "asc" ? range(0, dataset.data.length) : range(0, dataset.data.length).reverse(); for (const i of indexes) { - if (!isNaN(parseFloat(dataset.data[i]))) { - accumulator += parseFloat(dataset.data[i]); - data[i] = accumulator; + const cell = dataset.data[i]; + if (isNumberCell(cell)) { + accumulator += cell.value; + data[i] = { ...cell, value: accumulator }; } else { - data[i] = dataset.data[i]; + data[i] = EMPTY; } } return { ...dataset, data }; diff --git a/src/helpers/figures/charts/runtime/chartjs_dataset.ts b/src/helpers/figures/charts/runtime/chartjs_dataset.ts index 434ba44c0b..4bf540b775 100644 --- a/src/helpers/figures/charts/runtime/chartjs_dataset.ts +++ b/src/helpers/figures/charts/runtime/chartjs_dataset.ts @@ -8,6 +8,8 @@ import { LINE_DATA_POINT_RADIUS, LINE_FILL_TRANSPARENCY, } from "@odoo/o-spreadsheet-engine/constants"; +import { tryToNumber } from "@odoo/o-spreadsheet-engine/functions/helpers"; +import { isNumberCell } from "@odoo/o-spreadsheet-engine/helpers/cells/cell_evaluation"; import { ColorGenerator, colorToRGBA, @@ -81,7 +83,7 @@ export function getBarChartDatasets( const backgroundColor = colors.next(); const dataset: ChartDataset<"bar"> = { label, - data, + data: data.map((cell) => (isNumberCell(cell) ? cell.value : NaN)), hidden, borderColor: definition.background || BACKGROUND_CHART_COLOR, borderWidth: definition.stacked ? 1 : 0, @@ -119,7 +121,8 @@ export function getCalendarChartDatasetAndLabels( const values = dataSetsValues .map((ds) => ds.data) .flat() - .filter(isDefined); + .filter(isNumberCell) + .map((cell) => cell.value); const maxValue = Math.max(...values); const minValue = Math.min(...values); @@ -135,14 +138,14 @@ export function getCalendarChartDatasetAndLabels( label: dataSetValues.label, data: dataSetValues.data.map((v) => 1), backgroundColor: dataSetValues.data.map((v) => - v !== undefined ? colorMap(v) : definition.missingValueColor || COLOR_TRANSPARENT + isNumberCell(v) ? colorMap(v.value) : definition.missingValueColor || COLOR_TRANSPARENT ), borderColor: definition.background || BACKGROUND_CHART_COLOR, borderSkipped: false, borderWidth: 1, barPercentage: 1, categoryPercentage: 1, - values: dataSetValues.data, + values: dataSetValues.data.map((cell) => (isNumberCell(cell) ? cell.value : NaN)), }); } @@ -179,20 +182,20 @@ export function getWaterfallDatasetAndLabels( continue; } for (let i = 0; i < dataSetsValue.data.length; i++) { - const data = dataSetsValue.data[i]; + const cell = dataSetsValue.data[i]; labelsWithSubTotals.push(labels[i]); - if (isNaN(Number(data))) { + if (!isNumberCell(cell)) { datasetValues.push([lastValue, lastValue]); backgroundColor.push(""); continue; } - datasetValues.push([lastValue, data + lastValue]); - let color = data >= 0 ? positiveColor : negativeColor; + datasetValues.push([lastValue, cell.value + lastValue]); + let color = cell.value >= 0 ? positiveColor : negativeColor; if (i === 0 && dataSetsValue === dataSetsValues[0] && definition.firstValueAsSubtotal) { color = subTotalColor; } backgroundColor.push(color); - lastValue += data; + lastValue += cell.value; } if (definition.showSubTotals) { labelsWithSubTotals.push(_t("Subtotal")); @@ -223,16 +226,22 @@ export function getLineChartDatasets( for (let index = 0; index < dataSetsValues.length; index++) { let { label, data, hidden } = dataSetsValues[index]; label = definition.dataSets?.[index].label || label; + let dataValues: (number | { x: number; y: number })[] = []; const color = colors.next(); if (axisType && ["linear", "time"].includes(axisType)) { // Replace empty string labels by undefined to make sure chartJS doesn't decide that "" is the same as 0 - data = data.map((y, index) => ({ x: labels[index] || undefined, y })); + dataValues = data.map((y, index) => ({ + x: labels[index] === "" ? NaN : tryToNumber(labels[index], args.locale) ?? NaN, + y: isNumberCell(y) ? y.value : NaN, + })); + } else { + dataValues = data.map((cell) => (isNumberCell(cell) ? cell.value : NaN)); } const dataset: ChartDataset<"line"> = { label, - data, + data: dataValues, hidden, tension: 0, // 0 -> render straight lines, which is much faster borderColor: color, @@ -282,12 +291,12 @@ export function getPieChartDatasets( if (hidden) continue; const dataset: ChartDataset<"pie"> = { label, - data, + data: data.map((cell) => (isNumberCell(cell) ? cell.value : NaN)), borderColor: definition.background || "#FFFFFF", backgroundColor, hoverOffset: 10, }; - dataSets!.push(dataset); + dataSets.push(dataset); } return dataSets; } @@ -315,7 +324,7 @@ export function getComboChartDatasets( const type = design?.type ?? "line"; const dataset: ChartDataset<"bar" | "line"> = { label: label, - data, + data: data.map((cell) => (isNumberCell(cell) ? cell.value : null)), hidden, borderColor: color, backgroundColor: color, @@ -363,7 +372,7 @@ export function getRadarChartDatasets( const borderColor = colors.next(); const dataset: ChartDataset<"radar"> = { label, - data, + data: data.map((cell) => (isNumberCell(cell) ? cell.value : null)), hidden, borderColor, backgroundColor: borderColor, @@ -397,12 +406,16 @@ export function getGeoChartDatasets( const labelsAndValues: { [featureId: string]: { value: number; label: string } } = {}; if (dataSetsValues[0]) { for (let i = 0; i < dataSetsValues[0].data.length; i++) { - if (!labels[i] || dataSetsValues[0].data[i] === undefined) { + const cell = dataSetsValues[0].data[i]; + if (!labels[i] || cell === undefined) { continue; } const featureId = args.geoFeatureNameToId(regionName, labels[i]); if (featureId) { - labelsAndValues[featureId] = { value: dataSetsValues[0].data[i], label: labels[i] }; + labelsAndValues[featureId] = { + value: isNumberCell(cell) ? cell.value : 0, + label: labels[i], + }; } } } @@ -439,7 +452,13 @@ export function getFunnelChartDatasets( const dataset: ChartDataset<"bar"> = { label: datasetLabel, - data: data.map((value) => (value <= 0 ? [0, 0] : [-value, value])), + data: data.map((cell) => { + if (!isNumberCell(cell)) { + return 0; + } + const value = cell.value; + return value <= 0 ? [0, 0] : [-value, value]; + }), backgroundColor: getFunnelLabelColors(labels, definition.funnelColors), yAxisID: "y", xAxisID: "x", @@ -507,10 +526,8 @@ function getDataEntriesFromDatasets(hierarchicalDatasetValues: DatasetValues[], for (let i = 0; i < maxDatasetLength; i++) { entries[i] = {}; for (let j = 0; j < hierarchicalDatasetValues.length; j++) { - const groupBy = - hierarchicalDatasetValues[j].data[i] === null - ? GHOST_SUNBURST_VALUE - : String(hierarchicalDatasetValues[j].data[i]); + const value = hierarchicalDatasetValues[j].data[i]?.value; + const groupBy = value === null ? GHOST_SUNBURST_VALUE : String(value); entries[i][j] = groupBy; } entries[i].value = Number(values[i]); @@ -602,8 +619,8 @@ export function getTreeMapChartDatasets( for (let i = 0; i < maxDatasetLength; i++) { datasetEntries[i] = {}; for (let j = 0; j < dataSetsValues.length; j++) { - datasetEntries[i][j] = dataSetsValues[j].data[i] - ? String(dataSetsValues[j].data[i]) + datasetEntries[i][j] = dataSetsValues[j].data[i].value + ? String(dataSetsValues[j].data[i].value) : undefined; } datasetEntries[i].value = Number(labels[i]); diff --git a/src/helpers/figures/charts/runtime/chartjs_scales.ts b/src/helpers/figures/charts/runtime/chartjs_scales.ts index a7d28fc5b1..c2a4dc0585 100644 --- a/src/helpers/figures/charts/runtime/chartjs_scales.ts +++ b/src/helpers/figures/charts/runtime/chartjs_scales.ts @@ -6,6 +6,7 @@ import { DEFAULT_CHART_COLOR_SCALE, GRAY_300, } from "@odoo/o-spreadsheet-engine/constants"; +import { isNumberCell } from "@odoo/o-spreadsheet-engine/helpers/cells/cell_evaluation"; import { COLORSCHEMES, getColorScale } from "@odoo/o-spreadsheet-engine/helpers/color"; import { MOVING_AVERAGE_TREND_LINE_XAXIS_ID, @@ -141,7 +142,10 @@ export function getCalendarColorScale( if (!dataSetsValues.length || definition.legendPosition === "none") { return undefined; } - const allValues = dataSetsValues.flatMap((ds) => ds.data).filter(isDefined); + const allValues = dataSetsValues + .flatMap((ds) => ds.data) + .filter(isNumberCell) + .map((cell) => cell.value); const minValue = Math.min(...allValues); const maxValue = Math.max(...allValues); let colorScale: Color[] = []; @@ -283,7 +287,9 @@ export function getPyramidChartScales( scales!.x!.ticks!.callback = (value: number) => scalesXCallback(Math.abs(value)); const maxValue = Math.max( - ...dataSetsValues.map((dataSet) => Math.max(...dataSet.data.map(Math.abs))) + ...dataSetsValues.map((dataSet) => + Math.max(...dataSet.data.filter(isNumberCell).map((x) => Math.abs(x.value))) + ) ); scales!.x!.suggestedMin = -maxValue; scales!.x!.suggestedMax = maxValue; @@ -297,7 +303,9 @@ export function getRadarChartScales( ): ChartScales { const { locale, axisFormats, dataSetsValues } = args; const minValue = Math.min( - ...dataSetsValues.map((ds) => Math.min(...ds.data.filter((x) => !isNaN(x)))) + ...dataSetsValues.map((ds) => + Math.min(...ds.data.filter(isNumberCell).map((x) => x.value as number)) + ) ); return { r: { @@ -376,12 +384,20 @@ export function getFunnelChartScales( border: { display: false }, ticks: { callback: function (tickValue, index, ticks) { - const value = dataSet.data?.[index]; - const baseValue = dataSet.data?.[0]; - if (!baseValue || value === undefined) { + const valueCell = dataSet.data?.[index]; + const baseValueCell = dataSet.data?.[0]; + if ( + !baseValueCell?.value || + valueCell?.value === null || + !isNumberCell(valueCell) || + !isNumberCell(baseValueCell) + ) { return ""; } - return formatValue(value / baseValue, { format: "0%", locale: args.locale }); + return formatValue(valueCell.value / baseValueCell.value, { + format: "0%", + locale: args.locale, + }); }, }, grid: { display: false }, diff --git a/src/helpers/figures/charts/runtime/chartjs_show_values.ts b/src/helpers/figures/charts/runtime/chartjs_show_values.ts index 44fdf80de7..32348639c1 100644 --- a/src/helpers/figures/charts/runtime/chartjs_show_values.ts +++ b/src/helpers/figures/charts/runtime/chartjs_show_values.ts @@ -1,3 +1,4 @@ +import { isNumberCell } from "@odoo/o-spreadsheet-engine/helpers/cells/cell_evaluation"; import { chartFontColor, formatChartDatasetValue, @@ -40,11 +41,10 @@ export function getCalendarChartShowValues( ): ChartShowValuesPluginOptions { const { locale, axisFormats } = args; let background = (_value, dataset, index) => definition.background; - const values = - args.dataSetsValues - .flat() - .map((dsv) => dsv?.data.filter((v) => v !== null && v !== undefined)) - .flat() || []; + const values = args.dataSetsValues + .flat() + .flatMap((dsv) => dsv?.data.filter(isNumberCell)) + .map((cell) => cell.value); if (values.length) { const min = Math.min(...values); const max = Math.max(...values); diff --git a/tests/figures/chart/chart_plugin.test.ts b/tests/figures/chart/chart_plugin.test.ts index 7cb39666c7..fc912993dd 100644 --- a/tests/figures/chart/chart_plugin.test.ts +++ b/tests/figures/chart/chart_plugin.test.ts @@ -439,7 +439,7 @@ describe("datasource tests", function () { const config = getChartConfiguration(model, "43"); // In line/bars charts we want to keep invalid data that have a label to have a discontinuous line/empty space between bars - expect(config.data?.datasets![0].data).toEqual([null, 12]); + expect(config.data?.datasets![0].data).toEqual([NaN, 12]); } ); @@ -1943,8 +1943,8 @@ describe("Chart design configuration", () => { ); const data = getChartConfiguration(model, "1").data; expect(data.labels).toEqual(["P1", "", ""]); - expect(data.datasets![0].data).toEqual([null, 10, null]); - expect(data.datasets![1].data).toEqual([null, null, 20]); + expect(data.datasets![0].data).toEqual([NaN, 10, NaN]); + expect(data.datasets![1].data).toEqual([NaN, NaN, 20]); }); test("value without matching index in the label set", () => { @@ -2836,7 +2836,10 @@ describe("Linear/Time charts", () => { setCellContent(model, "C3", ""); const data = getChartConfiguration(model, chartId).data; expect(data.labels![1]).toEqual("1/17/1900"); - expect(data.datasets![0].data![1]).toEqual({ y: undefined, x: "1/17/1900" }); + expect(data.datasets![0].data![1]).toEqual({ + y: NaN, + x: toNumber("1/17/1900", model.getters.getLocale()), + }); }); test("date chart: rows datasets/labels are supported", () => { @@ -2856,8 +2859,8 @@ describe("Linear/Time charts", () => { const chart = (model.getters.getChartRuntime(chartId) as LineChartRuntime).chartJsConfig; expect(chart.data!.datasets![0].data).toEqual([ - { y: 1, x: "2" }, - { y: 10, x: "01/02/1900" }, + { y: 1, x: 2 }, + { y: 10, x: toNumber("01/02/1900", model.getters.getLocale()) }, ]); expect(chart.options?.scales?.x?.type).toEqual("time"); }); @@ -2881,8 +2884,8 @@ describe("Linear/Time charts", () => { const data = getChartConfiguration(model, chartId).data; expect(data.labels).toEqual(["0", "1"]); expect(data.datasets![0].data).toEqual([ - { y: 0, x: "0" }, - { y: 1, x: "1" }, + { y: 0, x: 0 }, + { y: 1, x: 1 }, ]); }); @@ -2901,7 +2904,7 @@ describe("Linear/Time charts", () => { setCellContent(model, "C3", ""); const data = getChartConfiguration(model, chartId).data; expect(data.labels![1]).toEqual(""); - expect(data.datasets![0].data![1]).toEqual({ y: 11, x: undefined }); + expect(data.datasets![0].data![1]).toEqual({ y: 11, x: NaN }); }); test("can create linear chart with non-number header in the label range", () => { @@ -2920,7 +2923,7 @@ describe("Linear/Time charts", () => { const chart = (model.getters.getChartRuntime(chartId) as LineChartRuntime).chartJsConfig; expect(chart.options?.scales?.x?.type).toEqual("linear"); expect(chart.data!.labels).toEqual(["1"]); - expect(chart.data!.datasets![0].data).toEqual([{ y: 10, x: "1" }]); + expect(chart.data!.datasets![0].data).toEqual([{ y: 10, x: 1 }]); }); test("ChartJS configuration for linear chart", () => { @@ -2942,10 +2945,10 @@ describe("Linear/Time charts", () => { datasets: [ { data: [ - { x: "20", y: 10 }, - { x: "19", y: 11 }, - { x: "18", y: 12 }, - { x: "17", y: 13 }, + { x: 20, y: 10 }, + { x: 19, y: 11 }, + { x: 18, y: 12 }, + { x: 17, y: 13 }, ], }, ], @@ -2980,10 +2983,10 @@ describe("Linear/Time charts", () => { datasets: [ { data: [ - { x: "1/19/1900", y: 10 }, - { x: "1/18/1900", y: 11 }, - { x: "1/17/1900", y: 12 }, - { x: "1/16/1900", y: 13 }, + { x: toNumber("1/19/1900", model.getters.getLocale()), y: 10 }, + { x: toNumber("1/18/1900", model.getters.getLocale()), y: 11 }, + { x: toNumber("1/17/1900", model.getters.getLocale()), y: 12 }, + { x: toNumber("1/16/1900", model.getters.getLocale()), y: 13 }, ], }, ], @@ -3267,8 +3270,8 @@ describe("Cumulative Data line chart", () => { ); const chartData = getChartConfiguration(model, "1").data!.datasets![0].data; - const initialData = [11, 12, 13, null, 30]; // null if for the non-number value with a label - const expectedCumulativeData = [11, 23, 36, null, 66]; + const initialData = [11, 12, 13, NaN, 30]; // NaN if for the non-number value with a label + const expectedCumulativeData = [11, 23, 36, NaN, 66]; expect(chartData).toEqual(initialData); @@ -3296,8 +3299,8 @@ describe("Cumulative Data line chart", () => { const runtime = model.getters.getChartRuntime("chartId") as LineChartRuntime; expect(runtime.chartJsConfig.data!.datasets![0].data).toEqual([ - { x: "1", y: 10 }, - { x: "2", y: 30 }, + { x: 1, y: 10 }, + { x: 2, y: 30 }, ]); }); }); @@ -3335,7 +3338,7 @@ describe("Pie chart invalid values", () => { }, "1" ); - const expectedData = [null, null, null, 42]; // negative & non-number values are replaced by null + const expectedData = [NaN, NaN, NaN, 42]; // negative & non-number values are replaced by NaN const expectedLabels = ["P2", "P3", "P4", ""]; const data = getChartConfiguration(model, "1").data; diff --git a/tests/test_helpers/getters_helpers.ts b/tests/test_helpers/getters_helpers.ts index caf59219d5..443bf2df35 100644 --- a/tests/test_helpers/getters_helpers.ts +++ b/tests/test_helpers/getters_helpers.ts @@ -166,7 +166,9 @@ export function getRangeValues( xc: string, sheetId: UID = model.getters.getActiveSheetId() ): (CellValue | undefined)[] { - return model.getters.getRangeValues(model.getters.getRangeFromSheetXC(sheetId, xc)); + return model.getters + .getRangeValues(model.getters.getRangeFromSheetXC(sheetId, xc)) + .map((cell) => cell.value); } /** From 2e89be35beaad625aa0f0add36134d87aab8bb7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Mon, 24 Nov 2025 16:54:58 +0100 Subject: [PATCH 2/3] [REF] charts: format doesn't depend on ranges The format of chart axis no longer depends on ranges, but now on the chart data. --- .../cell_evaluation/evaluation_plugin.ts | 10 ---- .../charts/runtime/chart_data_extractor.ts | 55 ++++++++++--------- tests/figures/chart/bar_chart_plugin.test.ts | 1 + tests/figures/chart/chart_plugin.test.ts | 5 +- .../figures/chart/combo_chart_plugin.test.ts | 3 +- .../pyramid_chart_plugin.test.ts | 6 +- 6 files changed, 42 insertions(+), 38 deletions(-) diff --git a/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts b/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts index c789b3ad66..e9d612f425 100644 --- a/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts +++ b/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts @@ -150,7 +150,6 @@ export class EvaluationPlugin extends CoreViewPlugin { "getCorrespondingFormulaCell", "getRangeFormattedValues", "getRangeValues", - "getRangeFormats", "getEvaluatedCell", "getEvaluatedCells", "getEvaluatedCellsInZone", @@ -265,15 +264,6 @@ export class EvaluationPlugin extends CoreViewPlugin { return this.mapVisiblePositions(range, (p) => this.getters.getEvaluatedCell(p)); } - /** - * Return the format of each cell in the range. - */ - getRangeFormats(range: Range): (Format | undefined)[] { - const sheet = this.getters.tryGetSheet(range.sheetId); - if (sheet === undefined) return []; - return this.getters.getEvaluatedCellsInZone(sheet.id, range.zone).map((cell) => cell.format); - } - getEvaluatedCell(position: CellPosition): EvaluatedCell { return this.evaluator.getEvaluatedCell(position); } diff --git a/src/helpers/figures/charts/runtime/chart_data_extractor.ts b/src/helpers/figures/charts/runtime/chart_data_extractor.ts index 92ba1f1fe1..8afd103e9f 100644 --- a/src/helpers/figures/charts/runtime/chart_data_extractor.ts +++ b/src/helpers/figures/charts/runtime/chart_data_extractor.ts @@ -85,8 +85,8 @@ export function getBarChartData( ({ labels, dataSetsValues } = aggregateDataForLabels(labels, dataSetsValues)); } - const leftAxisFormat = getChartDatasetFormat(getters, dataSets, "left"); - const rightAxisFormat = getChartDatasetFormat(getters, dataSets, "right"); + const leftAxisFormat = getChartDatasetFormat(dataSets, dataSetsValues, "left"); + const rightAxisFormat = getChartDatasetFormat(dataSets, dataSetsValues, "right"); const axisFormats = definition.horizontal ? { x: leftAxisFormat || rightAxisFormat } : { y: leftAxisFormat, y1: rightAxisFormat }; @@ -216,6 +216,7 @@ export function getCalendarChartData( const locale = getters.getLocale() || DEFAULT_LOCALE; ({ labels, dataSetsValues } = filterInvalidCalendarDataPoints(labels, dataSetsValues, locale)); + const axisFormats = { y: getChartDatasetFormat(dataSets, dataSetsValues, "left") }; ({ labels, dataSetsValues } = computeValuesAndLabels( labels, @@ -225,8 +226,6 @@ export function getCalendarChartData( locale )); - const axisFormats = { y: getChartDatasetFormat(getters, dataSets, "left") }; - return { dataSetsValues, axisFormats, @@ -295,8 +294,8 @@ export function getLineChartData( dataSetsValues = makeDatasetsCumulative(dataSetsValues, "asc"); } - const leftAxisFormat = getChartDatasetFormat(getters, dataSets, "left"); - const rightAxisFormat = getChartDatasetFormat(getters, dataSets, "right"); + const leftAxisFormat = getChartDatasetFormat(dataSets, dataSetsValues, "left"); + const rightAxisFormat = getChartDatasetFormat(dataSets, dataSetsValues, "right"); const labelsFormat = getChartLabelFormat(getters, labelRange, removeFirstLabel); const axisFormats = { y: leftAxisFormat, y1: rightAxisFormat, x: labelsFormat }; @@ -346,7 +345,7 @@ export function getPieChartData( ({ dataSetsValues, labels } = keepOnlyPositiveValues(labels, dataSetsValues)); - const dataSetFormat = getChartDatasetFormat(getters, dataSets, "left"); + const dataSetFormat = getChartDatasetFormat(dataSets, dataSetsValues, "left"); return { dataSetsValues, @@ -376,8 +375,8 @@ export function getRadarChartData( } const dataSetFormat = - getChartDatasetFormat(getters, dataSets, "left") || - getChartDatasetFormat(getters, dataSets, "right"); + getChartDatasetFormat(dataSets, dataSetsValues, "left") || + getChartDatasetFormat(dataSets, dataSetsValues, "right"); const axisFormats = { r: dataSetFormat }; return { @@ -404,8 +403,8 @@ export function getGeoChartData( ({ labels, dataSetsValues } = aggregateDataForLabels(labels, dataSetsValues)); const format = - getChartDatasetFormat(getters, dataSets, "left") || - getChartDatasetFormat(getters, dataSets, "right"); + getChartDatasetFormat(dataSets, dataSetsValues, "left") || + getChartDatasetFormat(dataSets, dataSetsValues, "right"); return { dataSetsValues, @@ -440,8 +439,8 @@ export function getFunnelChartData( } const format = - getChartDatasetFormat(getters, dataSets, "left") || - getChartDatasetFormat(getters, dataSets, "right"); + getChartDatasetFormat(dataSets, dataSetsValues, "left") || + getChartDatasetFormat(dataSets, dataSetsValues, "right"); return { dataSetsValues, @@ -954,17 +953,20 @@ function aggregateDataForLabels( ): { labels: string[]; dataSetsValues: DatasetValues[] } { const parseNumber = (value: CellValue) => (typeof value === "number" ? value : 0); const labelSet = new Set(labels); - const labelMap: { [key: string]: number[] } = {}; + const labelMap: { [key: string]: { value: number; format?: Format }[] } = {}; labelSet.forEach((label) => { - labelMap[label] = new Array(datasets.length).fill(0); + labelMap[label] = new Array(datasets.length); }); for (const indexOfLabel of range(0, labels.length)) { const label = labels[indexOfLabel]; for (const indexOfDataset of range(0, datasets.length)) { - labelMap[label][indexOfDataset] += parseNumber( - datasets[indexOfDataset].data[indexOfLabel]?.value - ); + const cell = datasets[indexOfDataset].data[indexOfLabel]; + if (!labelMap[label][indexOfDataset]) { + labelMap[label][indexOfDataset] = { ...cell, value: parseNumber(cell?.value) }; + } else { + labelMap[label][indexOfDataset].value += parseNumber(cell?.value); + } } } @@ -972,7 +974,7 @@ function aggregateDataForLabels( labels: Array.from(labelSet), dataSetsValues: datasets.map((dataset, indexOfDataset) => ({ ...dataset, - data: Array.from(labelSet).map((label) => ({ value: labelMap[label][indexOfDataset] })), + data: Array.from(labelSet).map((label) => labelMap[label][indexOfDataset]), })), }; } @@ -1043,15 +1045,18 @@ function getChartLabelValues( * found in the dataset ranges that isn't a date format. */ function getChartDatasetFormat( - getters: Getters, - allDataSets: DataSet[], + dataSetDefinitions: DataSet[], + dataSetValues: DatasetValues[], axis: "left" | "right" ): Format | undefined { - const dataSets = allDataSets.filter((ds) => (axis === "right") === !!ds.rightYAxis); + const dataSets = dataSetValues.filter( + (ds, i) => (axis === "right") === !!dataSetDefinitions[i].rightYAxis + ); for (const ds of dataSets) { - const formatsInDataset = getters.getRangeFormats(ds.dataRange); - const format = formatsInDataset.find((f) => f !== undefined && !isDateTimeFormat(f)); - if (format) return format; + const cell = ds.data.find(({ format }) => format !== undefined && !isDateTimeFormat(format)); + if (cell) { + return cell.format; + } } return undefined; } diff --git a/tests/figures/chart/bar_chart_plugin.test.ts b/tests/figures/chart/bar_chart_plugin.test.ts index 959e6dcea9..c8575cbb5d 100644 --- a/tests/figures/chart/bar_chart_plugin.test.ts +++ b/tests/figures/chart/bar_chart_plugin.test.ts @@ -76,6 +76,7 @@ describe("bar chart", () => { type: "bar", dataSets: [{ dataRange: "A1", yAxisId: "y" }], axesDesign: { x: { title: { text: "xAxis" } }, y: { title: { text: "yAxis" } } }, + dataSetsHaveTitle: false, }, "id" ); diff --git a/tests/figures/chart/chart_plugin.test.ts b/tests/figures/chart/chart_plugin.test.ts index fc912993dd..3953f4693c 100644 --- a/tests/figures/chart/chart_plugin.test.ts +++ b/tests/figures/chart/chart_plugin.test.ts @@ -2106,6 +2106,7 @@ describe("Chart design configuration", () => { test.each(["bar", "line", "scatter", "waterfall"])( "Bar/Line chart Y axis, cell with format", (chartType) => { + setCellContent(model, "A2", "10"); setCellFormat(model, "A2", "[$$]#,##0.00"); createChart(model, { ...defaultChart, type: chartType as "bar" | "line" }, "42"); expect( @@ -2134,8 +2135,10 @@ describe("Chart design configuration", () => { }, "42" ); + setCellContent(model, "A2", "10"); + setCellContent(model, "B2", "20"); setCellFormat(model, "A2", "[$$]#,#"); - setCellFormat(model, "B1", "0%"); + setCellFormat(model, "B2", "0%"); const config = model.getters.getChartRuntime("42") as any; const scales = config.chartJsConfig?.options?.scales; diff --git a/tests/figures/chart/combo_chart_plugin.test.ts b/tests/figures/chart/combo_chart_plugin.test.ts index 3b96955350..6dd36d1155 100644 --- a/tests/figures/chart/combo_chart_plugin.test.ts +++ b/tests/figures/chart/combo_chart_plugin.test.ts @@ -45,7 +45,8 @@ describe("combo chart", () => { test("both axis and tooltips formats are based on their data set", () => { const model = new Model(); - + setCellContent(model, "B1", "1000"); + setCellContent(model, "C1", "2000"); setCellFormat(model, "B1", "0.00%"); // first data set setCellFormat(model, "C1", "0.00[$$]"); // second data set diff --git a/tests/figures/chart/pyramid_chart/pyramid_chart_plugin.test.ts b/tests/figures/chart/pyramid_chart/pyramid_chart_plugin.test.ts index 8dd75711c3..baa393bf0a 100644 --- a/tests/figures/chart/pyramid_chart/pyramid_chart_plugin.test.ts +++ b/tests/figures/chart/pyramid_chart/pyramid_chart_plugin.test.ts @@ -78,7 +78,11 @@ describe("population pyramid chart", () => { createChart( model, - { type: "pyramid", dataSets: [{ dataRange: "A1" }, { dataRange: "A2" }] }, + { + type: "pyramid", + dataSets: [{ dataRange: "A1" }, { dataRange: "A2" }], + dataSetsHaveTitle: false, + }, "id" ); const runtime = model.getters.getChartRuntime("id") as any; From f023fb534c6abe59303253f1f45c9a940add490e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20Lef=C3=A8vre=20=28lul=29?= Date: Mon, 24 Nov 2025 17:00:30 +0100 Subject: [PATCH 3/3] [REF] charts: remove useless getter This getter is only used once and the formatted values can be taken from the evaluated cells without the need to iterate over the entire range again. --- .../cell_evaluation/evaluation_plugin.ts | 12 +----------- .../figures/charts/runtime/chart_data_extractor.ts | 5 +++-- tests/test_helpers/getters_helpers.ts | 4 +++- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts b/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts index e9d612f425..7e7a75be61 100644 --- a/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts +++ b/packages/o-spreadsheet-engine/src/plugins/ui_core_views/cell_evaluation/evaluation_plugin.ts @@ -10,7 +10,7 @@ import { invalidateDependenciesCommands, invalidateEvaluationCommands, } from "../../../types/commands"; -import { Format, FormattedValue } from "../../../types/format"; +import { Format } from "../../../types/format"; import { CellPosition, FunctionResultObject, @@ -148,7 +148,6 @@ export class EvaluationPlugin extends CoreViewPlugin { "evaluateFormulaResult", "evaluateCompiledFormula", "getCorrespondingFormulaCell", - "getRangeFormattedValues", "getRangeValues", "getEvaluatedCell", "getEvaluatedCells", @@ -246,15 +245,6 @@ export class EvaluationPlugin extends CoreViewPlugin { return this.evaluator.evaluateCompiledFormula(sheetId, compiledFormula, getSymbolValue); } - /** - * Return the value of each cell in the range as they are displayed in the grid. - */ - getRangeFormattedValues(range: Range): FormattedValue[] { - const sheet = this.getters.tryGetSheet(range.sheetId); - if (sheet === undefined) return []; - return this.mapVisiblePositions(range, (p) => this.getters.getEvaluatedCell(p).formattedValue); - } - /** * Return the value of each cell in the range. */ diff --git a/src/helpers/figures/charts/runtime/chart_data_extractor.ts b/src/helpers/figures/charts/runtime/chart_data_extractor.ts index 8afd103e9f..a48f39892f 100644 --- a/src/helpers/figures/charts/runtime/chart_data_extractor.ts +++ b/src/helpers/figures/charts/runtime/chart_data_extractor.ts @@ -1011,9 +1011,10 @@ function getChartLabelValues( !labelRange.invalidSheetName && !getters.isColHidden(labelRange.sheetId, left) ) { + const cells = getters.getRangeValues(labelRange); labels = { - formattedValues: getters.getRangeFormattedValues(labelRange), - values: getters.getRangeValues(labelRange).map(({ value }) => String(value ?? "")), + formattedValues: cells.map(({ formattedValue }) => formattedValue), + values: cells.map(({ value }) => String(value ?? "")), }; } else if (dataSets[0]) { const ranges = getData(getters, dataSets[0]); diff --git a/tests/test_helpers/getters_helpers.ts b/tests/test_helpers/getters_helpers.ts index 443bf2df35..4feeba51c9 100644 --- a/tests/test_helpers/getters_helpers.ts +++ b/tests/test_helpers/getters_helpers.ts @@ -158,7 +158,9 @@ export function getRangeFormattedValues( xc: string, sheetId: UID = model.getters.getActiveSheetId() ): FormattedValue[] { - return model.getters.getRangeFormattedValues(model.getters.getRangeFromSheetXC(sheetId, xc)); + return model.getters + .getRangeValues(model.getters.getRangeFromSheetXC(sheetId, xc)) + .map((cell) => cell.formattedValue); } export function getRangeValues(