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..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 diff --git a/src/odr/internal/odf/odf_chart.cpp b/src/odr/internal/odf/odf_chart.cpp index 94a8663ca..2431d21b5 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,38 +195,60 @@ 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( 1, count_cells(table.child("table:table-header-columns") .child("table:table-column"))); + if (leading > max_cells) { + return false; + } 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 +266,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 +307,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, "