From 35f6e132b354a9db50ec7229d30620c7bc037b62 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 02:26:06 +0200 Subject: [PATCH 1/2] fix: preserve ODF chart data positions --- CHANGELOG.md | 3 + src/odr/internal/odf/AGENTS.md | 3 + src/odr/internal/odf/odf_chart.cpp | 87 +++++++++++++++--------- test/src/internal/odf/odf_chart_test.cpp | 54 +++++++++++++++ 4 files changed, 116 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5174d8f3f..ae04fbfab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- ODF charts keep missing points aligned with their categories, honor repeated + rows, and retain implicit series. Expanded chart tables are bounded. + - ODF number formats clamp digit counts to 4,096 and the minimum decimals to the decimals, as LibreOffice does, before they expand placeholders. diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index d58d28423..e7160cf11 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -229,3 +229,6 @@ placeholders; a negative count keeps the default. `svg:font-family`, which is a css value, so `Liberation Serif` arrives as `'Liberation Serif'`. The renderer copes, but the public value shows the quotes. + +Chart tables expand at most 1,048,576 cells, including gaps in ragged rows. +Larger tables fall back to the embedded replacement image. diff --git a/src/odr/internal/odf/odf_chart.cpp b/src/odr/internal/odf/odf_chart.cpp index 94a8663ca..16369d9b8 100644 --- a/src/odr/internal/odf/odf_chart.cpp +++ b/src/odr/internal/odf/odf_chart.cpp @@ -126,8 +126,7 @@ class ChartWriter { const pugi::xml_node plot_area = m_chart.child("chart:plot-area"); read_class(); read_plot_box(plot_area); - read_data(plot_area); - if (m_series.empty()) { + if (!read_data(plot_area) || m_series.empty()) { return {}; } @@ -196,11 +195,11 @@ class ChartWriter { /// The `local-table` carries the plotted values; its header columns are the /// categories and its header rows the series labels. - void read_data(const pugi::xml_node plot_area) { + bool read_data(const pugi::xml_node plot_area) { const pugi::xml_node table = m_chart.find_child_by_attribute( "table:table", "table:name", "local-table"); if (!table) { - return; + return false; } const std::size_t leading = std::max( @@ -208,26 +207,45 @@ class ChartWriter { .child("table:table-column"))); std::vector labels; - for (const pugi::xml_node header : - table.child("table:table-header-rows").children("table:table-row")) { - labels = read_row_text(header); - break; + if (const pugi::xml_node header = + table.child("table:table-header-rows").child("table:table-row")) { + const auto cells = read_row(header); + if (!cells) { + return false; + } + labels = *cells | std::views::transform(read_text) | + std::ranges::to>(); } std::vector>> columns; for (const pugi::xml_node row : table.child("table:table-rows").children("table:table-row")) { - const std::vector cells = read_row(row); - if (cells.size() <= leading) { - continue; + const auto cells = read_row(row); + const std::uint32_t repeated = + row.attribute("table:number-rows-repeated").as_uint(1); + if (!cells || repeated == 0 || + repeated > max_cells - m_categories.size()) { + return false; } - m_categories.emplace_back(read_text(cells[leading - 1])); - for (std::size_t i = leading; i < cells.size(); ++i) { - if (columns.size() < i - leading + 1) { - columns.resize(i - leading + 1); - } - columns[i - leading].push_back(read_value(cells[i])); + const std::size_t rows = m_categories.size() + repeated; + const std::size_t width = + std::max(columns.size(), + cells->size() > leading ? cells->size() - leading : 0); + if (width > max_cells / rows) { + return false; + } + columns.resize(width); + for (std::size_t i = 0; i < width; ++i) { + columns[i].resize(m_categories.size()); + columns[i].insert(columns[i].end(), repeated, + leading + i < cells->size() + ? read_value((*cells)[leading + i]) + : std::nullopt); } + m_categories.insert(m_categories.end(), repeated, + leading <= cells->size() + ? read_text((*cells)[leading - 1]) + : std::string()); } std::size_t index = 0; @@ -245,26 +263,38 @@ class ChartWriter { ++index; } // A chart with no `chart:series` at all still has its table. - for (; index < columns.size() && m_series.empty(); ++index) { - m_series.push_back({.label = {}, - .values = columns[index], - .colour = std::string(colour_at(index))}); + if (m_series.empty()) { + for (; index < columns.size(); ++index) { + m_series.push_back({.label = leading + index < labels.size() + ? labels[leading + index] + : std::string(), + .values = std::move(columns[index]), + .colour = std::string(colour_at(index))}); + } } trim(); read_range(); + return true; } + static constexpr std::size_t max_cells = 1048576; + [[nodiscard]] static std::size_t count_cells(const pugi::xml_node column) { std::size_t result = 0; for (pugi::xml_node node = column; node; node = node.next_sibling("table:table-column")) { - result += node.attribute("table:number-columns-repeated").as_uint(1); + const std::size_t repeated = + node.attribute("table:number-columns-repeated").as_uint(1); + if (repeated > max_cells - result) { + return max_cells + 1; + } + result += repeated; } return result; } - [[nodiscard]] static std::vector + [[nodiscard]] static std::optional> read_row(const pugi::xml_node row) { std::vector result; for (const pugi::xml_node cell : row.children()) { @@ -274,19 +304,14 @@ class ChartWriter { } const auto repeated = cell.attribute("table:number-columns-repeated").as_uint(1); - for (unsigned i = 0; i < repeated; ++i) { - result.push_back(cell); + if (repeated == 0 || repeated > max_cells - result.size()) { + return std::nullopt; } + result.insert(result.end(), repeated, cell); } return result; } - [[nodiscard]] static std::vector - read_row_text(const pugi::xml_node row) { - return read_row(row) | std::views::transform(read_text) | - std::ranges::to>(); - } - /// A missing data point is written `office:value="NaN"`. [[nodiscard]] static std::optional read_value(const pugi::xml_node cell) { diff --git a/test/src/internal/odf/odf_chart_test.cpp b/test/src/internal/odf/odf_chart_test.cpp index a0e7366a1..5b135434e 100644 --- a/test/src/internal/odf/odf_chart_test.cpp +++ b/test/src/internal/odf/odf_chart_test.cpp @@ -156,3 +156,57 @@ TEST(OdfChart, a_part_with_no_chart_renders_nothing) { R"(svg:height="9cm" chart:class="chart:bar"/>)")) .has_value()); } + +TEST(OdfChart, ragged_repeated_rows_keep_all_series_on_their_categories) { + for (const bool implicit : {false, true}) { + pugi::xml_document document; + ASSERT_TRUE(document.load_string(bar_chart("chart:scatter").c_str())); + const pugi::xml_node chart = document.document_element() + .child("office:body") + .child("office:chart") + .child("chart:chart"); + if (implicit) { + chart.child("chart:plot-area").remove_children(); + } + pugi::xml_node first = + chart.child("table:table").child("table:table-rows").first_child(); + first.remove_child(first.last_child()); + first.append_attribute("table:number-rows-repeated").set_value(2); + + const auto svg = render_chart(document.document_element()); + ASSERT_TRUE(svg); + pugi::xml_document output; + ASSERT_TRUE(output.load_string(svg->c_str())); + const auto points = output.select_nodes("/svg/circle"); + ASSERT_EQ(points.size(), 3u); + EXPECT_NEAR(points[0].node().attribute("cx").as_double(), 3333.33, 0.01); + EXPECT_NEAR(points[1].node().attribute("cx").as_double(), 8000, 0.01); + EXPECT_NEAR(points[2].node().attribute("cx").as_double(), 12666.7, 0.1); + EXPECT_EQ(count(*svg, " Date: Mon, 5 Oct 2026 20:48:28 +0200 Subject: [PATCH 2/2] fix(odf): refuse a chart with too many header columns count_cells returns more than the limit for an oversized header, which read_data then used as the number of leading columns. Refuse it there, so the replacement image shows. Move the AGENTS.md note on the chart limit out of the open work list. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- src/odr/internal/odf/AGENTS.md | 6 +++--- src/odr/internal/odf/odf_chart.cpp | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index e7160cf11..b7b2c4fae 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -206,6 +206,9 @@ Unreadable geometry is dropped rather than emitting invalid SVG. Number-format digit counts are clamped to 4,096 per field before expanding placeholders; a negative count keeps the default. +Chart tables expand at most 1,048,576 cells, including gaps in ragged rows. +Larger tables fall back to the embedded replacement image. + ## Open work 1. **Save never re-encrypts.** A document decrypted from a password-protected @@ -229,6 +232,3 @@ placeholders; a negative count keeps the default. `svg:font-family`, which is a css value, so `Liberation Serif` arrives as `'Liberation Serif'`. The renderer copes, but the public value shows the quotes. - -Chart tables expand at most 1,048,576 cells, including gaps in ragged rows. -Larger tables fall back to the embedded replacement image. diff --git a/src/odr/internal/odf/odf_chart.cpp b/src/odr/internal/odf/odf_chart.cpp index 16369d9b8..2431d21b5 100644 --- a/src/odr/internal/odf/odf_chart.cpp +++ b/src/odr/internal/odf/odf_chart.cpp @@ -205,6 +205,9 @@ class ChartWriter { const std::size_t leading = std::max( 1, count_cells(table.child("table:table-header-columns") .child("table:table-column"))); + if (leading > max_cells) { + return false; + } std::vector labels; if (const pugi::xml_node header =