From f0599f62f1c5982d68cb9cbf0d8ce719f66bc285 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 16 Jul 2026 19:31:02 +0200 Subject: [PATCH 1/4] feat(ooxml): pptx slide size + tables, xlsx merged cells + value types, docx table merges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pptx: - read the real slide size from p:presentation/p:sldSz (ECMA-376 default 10in x 7.5in when absent) instead of a hardcoded 11.02in x 8.27in; slide names from p:cSld@name - parse tables: a:tbl sits inside p:graphicFrame/a:graphic/a:graphicData, which the parser never descended into; wire create_table_element + append_column (a:tblGrid/a:gridCol), rows/cells, spans from gridSpan/rowSpan, covered cells from hMerge/vMerge, column widths and row heights, and p:xfrm positioning for graphic frames xlsx: - merged cells: parse mergeCells into anchor span + covered flags on the SheetCell side map; ranges whose anchor cell is absent from sheetData are dropped to keep the renderer's cursor consistent - cell value types: derive number-vs-string from c/@t so numeric cells get the odr-value-type-float class docx: - replace dead ODF-name code in the table adapter: dimensions from w:tblGrid/w:gridCol + w:tr, colspan from w:gridSpan, vMerge continuations report covered and restart cells compute their rowspan by walking following rows at the same grid column - bookmark_name reads w:name instead of ODF's text:name TableCursor: fix a latent infinite loop exposed by xlsx merges — the per-row pending covered ranges were appended unsorted, so handle_rowspan_ missed them when a long-rowspan cell was followed by later rowspan anchors at smaller columns and the sheet renderer never advanced (hung on ruski.xlsx). Insert sorted, skip tolerantly, advance the cursor on the (normally unreachable) covered-cell branch, and add a regression test. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KH4XvPFfdUvH7HMp8rmqZT --- src/odr/internal/common/table_cursor.cpp | 12 +- src/odr/internal/html/document_element.cpp | 4 + src/odr/internal/ooxml/presentation/AGENTS.md | 45 +++--- src/odr/internal/ooxml/presentation/README.md | 10 +- .../ooxml_presentation_document.cpp | 130 +++++++++--------- .../ooxml_presentation_document.hpp | 4 + .../ooxml_presentation_parser.cpp | 41 +++++- src/odr/internal/ooxml/spreadsheet/AGENTS.md | 23 ++-- src/odr/internal/ooxml/spreadsheet/README.md | 7 +- .../ooxml_spreadsheet_document.cpp | 29 ++-- .../ooxml_spreadsheet_element_registry.cpp | 6 + .../ooxml_spreadsheet_element_registry.hpp | 3 + .../spreadsheet/ooxml_spreadsheet_parser.cpp | 34 +++++ src/odr/internal/ooxml/text/AGENTS.md | 15 +- src/odr/internal/ooxml/text/README.md | 3 +- .../ooxml/text/ooxml_text_document.cpp | 107 +++++++++----- .../src/internal/common/table_cursor_test.cpp | 19 ++- 17 files changed, 331 insertions(+), 161 deletions(-) diff --git a/src/odr/internal/common/table_cursor.cpp b/src/odr/internal/common/table_cursor.cpp index ec1ce8361..5a8d18852 100644 --- a/src/odr/internal/common/table_cursor.cpp +++ b/src/odr/internal/common/table_cursor.cpp @@ -1,5 +1,6 @@ #include +#include #include namespace odr::internal { @@ -30,14 +31,17 @@ void TableCursor::add_cell(const std::uint32_t colspan, const std::uint32_t repeat) { const std::uint32_t next_column = m_column + colspan * repeat; - // handle rowspan + // handle rowspan; keep each row's ranges sorted by start so + // `handle_rowspan_` can skip them front-to-back auto it = std::begin(m_sparse); for (std::uint32_t i = 1; i < rowspan; ++i) { if (std::next(it) == std::end(m_sparse)) { m_sparse.emplace_back(); } ++it; - it->emplace_back(Range{m_column, next_column}); + const auto pos = std::ranges::find_if( + *it, [&](const Range &range) { return range.start > m_column; }); + it->insert(pos, Range{m_column, next_column}); } m_column = next_column; @@ -55,8 +59,8 @@ std::uint32_t TableCursor::row() const noexcept { return m_row; } void TableCursor::handle_rowspan_() noexcept { auto &s = m_sparse.front(); auto it = std::begin(s); - while (it != std::end(s) && m_column == it->start) { - m_column = it->end; + while (it != std::end(s) && it->start <= m_column) { + m_column = std::max(m_column, it->end); ++it; } s.erase(std::begin(s), it); diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index e977d0c14..3422667ea 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -145,6 +145,10 @@ void html::translate_sheet(const Sheet &sheet, const WritingState &state) { const SheetCell cell = sheet.cell(column_index, row_index); if (cell.is_covered()) { + // normally unreachable: the cursor skips positions covered by an + // anchor's span; advance one column so inconsistent spans cannot + // starve the loop + cursor.add_cell(); continue; } diff --git a/src/odr/internal/ooxml/presentation/AGENTS.md b/src/odr/internal/ooxml/presentation/AGENTS.md index 80d078ab7..40428b2f7 100644 --- a/src/odr/internal/ooxml/presentation/AGENTS.md +++ b/src/odr/internal/ooxml/presentation/AGENTS.md @@ -6,7 +6,7 @@ OOXML mechanics (registry/adapter pattern, OPC relationships, encryption) in **Scope.** Read `ppt/presentation.xml` and each slide's shape tree into the abstract model so the generic renderer lays out positioned frames. Paragraphs, -runs, tables (see caveat below), inline text styling. +runs, tables, inline text styling. ## Design decisions @@ -15,8 +15,11 @@ relationship target of `presentation.xml` into an `rId → xml` map (masters, layouts, theme included — only slides are actually walked). Slide **order = the document order of `p:sldId` in `p:sldIdLst`** (not filename/rId order); each slide's `r:id` looks up its part, and parsing descends `p:cSld/p:spTree`. -Dispatch table: `p:sp`→**frame** (shapes are frames), `p:txBody`→group, -`a:p`→paragraph, `a:r`→span, `a:t`→text, `a:tbl`→table. +Dispatch table: `p:sp`→**frame** (shapes are frames), `p:graphicFrame`→frame +(descends `a:graphic/a:graphicData`), `p:txBody`→group, `a:p`→paragraph, +`a:r`→span, `a:t`→text, `a:tbl`→table (columns from `a:tblGrid/a:gridCol` via +`append_column`, rows/cells from `a:tr`/`a:tc`; spans from +`gridSpan`/`rowSpan`, covered cells from `hMerge`/`vMerge`). **Styles are resolved inline — there is no `StyleRegistry`.** Free functions in the document read `a:rPr` / `a:pPr` attributes directly (font, size in @@ -26,8 +29,10 @@ hundredth-points, bold/italic/underline/strike/shadow/colour/highlight; align, computed on-demand from the XML with no cached or master/default-style contribution. -**Frame positioning is EMU-based.** `p:spPr/a:xfrm/a:off` + `a:ext` give -`x/y/width/height` in EMUs; anchor type is always `at_page`. +**Frame positioning is EMU-based.** `p:spPr/a:xfrm/a:off` + `a:ext` (`p:xfrm` +for `p:graphicFrame`) give `x/y/width/height` in EMUs; anchor type is always +`at_page`. Slide size comes from `p:presentation/p:sldSz` (ECMA-376 default +10in × 7.5in when absent). ## Module layout @@ -43,23 +48,15 @@ contribution. Coverage is in [`README.md`](README.md). Foundational gaps, roughly by value: -1. **Slide geometry is hardcoded.** `slide_page_layout` returns a fixed - 11.02in × 8.27in; `slide_master_page` and `slide_name` return empty. Real - slide size (`p:presentation/p:sldSz`), master/layout inheritance, and slide - names are all TODO. -2. **Tables are broken.** `a:tbl` is created with the plain builder, not - `create_table_element`, so no `m_tables` entry exists and `table_first_column` - throws; `table_dimensions` iterates ODF names (`table:table-column/-row`, - copy-paste from ODF) → 0×0. Despite the README checkbox, tables need real - wiring (`create_table_element`/`append_column` + `a:gridCol`/`a:tr`/`a:tc` - parsers). -3. **Images not modelled** — no `p:pic`/`a:blip` parser entry; `image_href` +1. **No master/layout inheritance.** `slide_master_page` returns empty; master + and layout parts are loaded into the relationship map but never consulted + (no inherited placeholders, backgrounds, or styles). +2. **Images not modelled** — no `p:pic`/`a:blip` parser entry; `image_href` reads ODF-style `xlink:href` (wrong for pptx `r:embed`). -4. **`is_text_node` copy-paste bug.** It matches `w:t`/`w:tab` (wordprocessing) - instead of `a:t`/`a:tab`, so consecutive `a:t` runs are never coalesced — - each becomes its own text Element (harmless, but an artifact to fix). -5. **Read-only, inconsistently.** `Document::is_editable`/`save` say no, yet the - adapter's `element_is_editable` returns true and `text_set_content` is fully - implemented — the machinery for text editing exists but is not exposed. Wiring - edit + save (mirroring docx) is a natural next step. -6. **Listings, comments/annotations** not modelled. +3. **Table cell styles unresolved.** Tables are wired (grid, spans, covered + cells, column widths/row heights), but `a:tcPr` (fills, borders, margins) + is not translated. +4. **Read-only.** `text_set_content` machinery exists but is dormant + (`element_is_editable` → false); wiring edit + save (mirroring docx) is a + natural next step. +5. **Listings, comments/annotations** not modelled. diff --git a/src/odr/internal/ooxml/presentation/README.md b/src/odr/internal/ooxml/presentation/README.md index f171d7804..5b07c1949 100644 --- a/src/odr/internal/ooxml/presentation/README.md +++ b/src/odr/internal/ooxml/presentation/README.md @@ -19,7 +19,8 @@ Roughly ordered by importance. - [x] open - [x] slides - [x] shapes (`p:sp`), text bodies - - [x] slide master page / page layout + - [x] slide size (`p:sldSz`) and slide names + - [ ] slide master / layout inheritance - [x] text extraction - [ ] edit - [ ] save @@ -28,7 +29,8 @@ Roughly ordered by importance. - [x] paragraphs, runs / spans - [x] line breaks -- [x] tables (grid columns, rows, cells) +- [x] tables (`p:graphicFrame` / `a:tbl`: grid columns incl. widths, rows, + cells, merged cells) - [ ] images - [ ] listings - [ ] annotations / comments @@ -46,8 +48,8 @@ Roughly ordered by importance. - [x] paragraph - [x] alignment - [x] indentation / left & right margins -- [x] tables -- [x] page layout (via master page) +- [x] tables (column widths, row heights; no `a:tcPr` cell styles) +- [x] page layout (slide size) - [ ] graphic / drawing styles ## References diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp index 39b058652..4869232e2 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp @@ -8,12 +8,13 @@ #include #include #include -#include #include #include #include #include +#include + namespace odr::internal::ooxml::presentation { namespace { @@ -32,6 +33,21 @@ Document::Document(std::shared_ptr files) util::xml::parse(*m_files, AbsPath("/ppt").join(RelPath(target))); } + // ECMA-376 default slide size when p:sldSz is absent. + m_slide_layout.width = Measure("10 in"); + m_slide_layout.height = Measure("7.5 in"); + if (const pugi::xml_node slide_size = + m_document_xml.document_element().child("p:sldSz")) { + if (const std::optional width = + read_emus_attribute(slide_size.attribute("cx"))) { + m_slide_layout.width = width; + } + if (const std::optional height = + read_emus_attribute(slide_size.attribute("cy"))) { + m_slide_layout.height = height; + } + } + const ParseContext parse_context(m_slides_xml); m_root_element = parse_tree(m_element_registry, parse_context, m_document_xml.document_element()); @@ -39,6 +55,8 @@ Document::Document(std::shared_ptr files) m_element_adapter = create_element_adapter(*this, m_element_registry); } +const PageLayout &Document::slide_layout() const { return m_slide_layout; } + const ElementRegistry &Document::element_registry() const { return m_element_registry; } @@ -257,21 +275,15 @@ class ElementAdapter final : public abstract::ElementAdapter, [[nodiscard]] PageLayout slide_page_layout( [[maybe_unused]] const ElementIdentifier element_id) const override { - // TODO - return { - .width = Measure("11.02 in"), - .height = Measure("8.27 in"), - .print_orientation = {}, - .margin = {}, - }; + return m_document->slide_layout(); } [[nodiscard]] ElementIdentifier slide_master_page( [[maybe_unused]] const ElementIdentifier element_id) const override { return {}; // TODO } - [[nodiscard]] std::string slide_name( - [[maybe_unused]] const ElementIdentifier element_id) const override { - return {}; // TODO + [[nodiscard]] std::string + slide_name(const ElementIdentifier element_id) const override { + return get_node(element_id).child("p:cSld").attribute("name").value(); } [[nodiscard]] TextStyle @@ -392,25 +404,10 @@ class ElementAdapter final : public abstract::ElementAdapter, const pugi::xml_node node = get_node(element_id); TableDimensions result; - TableCursor cursor; - - for (auto column : node.children("table:table-column")) { - const auto columns_repeated = - column.attribute("table:number-columns-repeated").as_uint(1); - cursor.add_column(columns_repeated); - } - - result.columns = cursor.column(); - cursor = {}; - - for (auto row : node.children("table:table-row")) { - const auto rows_repeated = - row.attribute("table:number-rows-repeated").as_uint(1); - cursor.add_row(rows_repeated); - } - - result.rows = cursor.row(); - + result.columns = static_cast( + std::ranges::distance(node.child("a:tblGrid").children("a:gridCol"))); + result.rows = static_cast( + std::ranges::distance(node.children("a:tr"))); return result; } [[nodiscard]] ElementIdentifier @@ -428,21 +425,35 @@ class ElementAdapter final : public abstract::ElementAdapter, [[nodiscard]] TableColumnStyle table_column_style(const ElementIdentifier element_id) const override { - return get_partial_style(element_id).table_column_style; + TableColumnStyle result; + if (const std::optional width = + read_emus_attribute(get_node(element_id).attribute("w"))) { + result.width = width; + } + return result; } [[nodiscard]] TableRowStyle table_row_style(const ElementIdentifier element_id) const override { - return get_partial_style(element_id).table_row_style; + TableRowStyle result; + if (const std::optional height = + read_emus_attribute(get_node(element_id).attribute("h"))) { + result.height = height; + } + return result; } - [[nodiscard]] bool table_cell_is_covered( - [[maybe_unused]] const ElementIdentifier element_id) const override { - return false; + [[nodiscard]] bool + table_cell_is_covered(const ElementIdentifier element_id) const override { + const pugi::xml_node node = get_node(element_id); + return node.attribute("hMerge").as_bool() || + node.attribute("vMerge").as_bool(); } - [[nodiscard]] TableDimensions table_cell_span( - [[maybe_unused]] const ElementIdentifier element_id) const override { - return {1, 1}; // TODO + [[nodiscard]] TableDimensions + table_cell_span(const ElementIdentifier element_id) const override { + const pugi::xml_node node = get_node(element_id); + return {node.attribute("rowSpan").as_uint(1), + node.attribute("gridSpan").as_uint(1)}; } [[nodiscard]] ValueType table_cell_value_type( [[maybe_unused]] const ElementIdentifier element_id) const override { @@ -459,48 +470,32 @@ class ElementAdapter final : public abstract::ElementAdapter, } [[nodiscard]] std::optional frame_x(const ElementIdentifier element_id) const override { - if (const std::optional x = - read_emus_attribute(get_node(element_id) - .child("p:spPr") - .child("a:xfrm") - .child("a:off") - .attribute("x"))) { + if (const std::optional x = read_emus_attribute( + get_frame_xfrm(element_id).child("a:off").attribute("x"))) { return x->to_string(); } return {}; } [[nodiscard]] std::optional frame_y(const ElementIdentifier element_id) const override { - if (const std::optional y = - read_emus_attribute(get_node(element_id) - .child("p:spPr") - .child("a:xfrm") - .child("a:off") - .attribute("y"))) { + if (const std::optional y = read_emus_attribute( + get_frame_xfrm(element_id).child("a:off").attribute("y"))) { return y->to_string(); } return {}; } [[nodiscard]] std::optional frame_width(const ElementIdentifier element_id) const override { - if (const std::optional cx = - read_emus_attribute(get_node(element_id) - .child("p:spPr") - .child("a:xfrm") - .child("a:ext") - .attribute("cx"))) { + if (const std::optional cx = read_emus_attribute( + get_frame_xfrm(element_id).child("a:ext").attribute("cx"))) { return cx->to_string(); } return {}; } [[nodiscard]] std::optional frame_height(const ElementIdentifier element_id) const override { - if (const std::optional cy = - read_emus_attribute(get_node(element_id) - .child("p:spPr") - .child("a:xfrm") - .child("a:ext") - .attribute("cy"))) { + if (const std::optional cy = read_emus_attribute( + get_frame_xfrm(element_id).child("a:ext").attribute("cy"))) { return cy->to_string(); } return {}; @@ -542,6 +537,17 @@ class ElementAdapter final : public abstract::ElementAdapter, return m_registry->element_at(element_id).node; } + /// `p:sp` carries its transform in `p:spPr/a:xfrm`, `p:graphicFrame` in + /// `p:xfrm`. + [[nodiscard]] pugi::xml_node + get_frame_xfrm(const ElementIdentifier element_id) const { + const pugi::xml_node node = get_node(element_id); + if (const pugi::xml_node xfrm = node.child("p:spPr").child("a:xfrm")) { + return xfrm; + } + return node.child("p:xfrm"); + } + [[nodiscard]] static std::string get_text(const pugi::xml_node node) { const std::string name = node.name(); diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp index 73edf0fa0..d0a901f82 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include #include @@ -14,6 +16,7 @@ class Document final : public internal::Document { explicit Document(std::shared_ptr files); [[nodiscard]] const ElementRegistry &element_registry() const; + [[nodiscard]] const PageLayout &slide_layout() const; [[nodiscard]] bool is_editable() const noexcept override; [[nodiscard]] bool is_savable(bool encrypted) const noexcept override; @@ -24,6 +27,7 @@ class Document final : public internal::Document { private: pugi::xml_document m_document_xml; std::unordered_map m_slides_xml; + PageLayout m_slide_layout; ElementRegistry m_element_registry; }; diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp index a1fc22a44..4452c80dc 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp @@ -96,6 +96,41 @@ void parse_slide_children(ElementRegistry ®istry, node.child("p:cSld").child("p:spTree")); } +void parse_graphic_frame_children(ElementRegistry ®istry, + const ParseContext &context, + const ElementIdentifier parent_id, + const pugi::xml_node node) { + parse_any_element_children(registry, context, parent_id, + node.child("a:graphic").child("a:graphicData")); +} + +std::tuple +parse_table_element(ElementRegistry ®istry, const ParseContext &context, + const pugi::xml_node node) { + if (!node) { + return {null_element_id, pugi::xml_node()}; + } + + const auto &[element_id, unused1, unused2] = + registry.create_table_element(node); + + for (const pugi::xml_node column_node : + node.child("a:tblGrid").children("a:gridCol")) { + const auto &[column_id, _] = + registry.create_element(ElementType::table_column, column_node); + registry.append_column(element_id, column_id); + } + + for (const pugi::xml_node row_node : node.children("a:tr")) { + const auto [row_id, _] = + parse_element_tree(registry, context, ElementType::table_row, row_node, + parse_any_element_children); + registry.append_child(element_id, row_id); + } + + return {element_id, node.next_sibling()}; +} + void parse_presentation_children(ElementRegistry ®istry, const ParseContext &context, const ElementIdentifier root_id, @@ -130,14 +165,14 @@ parse_any_element_tree(ElementRegistry ®istry, const ParseContext &context, ElementType::root, parse_presentation_children)}, {"p:sld", create_default_tree_parser(ElementType::slide)}, {"p:sp", create_default_tree_parser(ElementType::frame)}, + {"p:graphicFrame", create_default_tree_parser( + ElementType::frame, parse_graphic_frame_children)}, {"p:txBody", create_default_tree_parser(ElementType::group)}, {"a:t", parse_text_element}, {"a:tab", parse_text_element}, {"a:p", create_default_tree_parser(ElementType::paragraph)}, {"a:r", create_default_tree_parser(ElementType::span)}, - {"a:tbl", create_default_tree_parser(ElementType::table)}, - {"a:gridCol", create_default_tree_parser(ElementType::table_column)}, - {"a:tr", create_default_tree_parser(ElementType::table_row)}, + {"a:tbl", parse_table_element}, {"a:tc", create_default_tree_parser(ElementType::table_cell)}, }; diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index 56831c594..6c8d8cf47 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/src/odr/internal/ooxml/spreadsheet/AGENTS.md @@ -26,11 +26,14 @@ via `Sheet.cells` (`(col,row) → {node, element_id}`). Columns are a with `lookup_greater_or_equals`. Shapes hang off the sheet in a separate chain (`first_shape_id`). Dimensions come from ``. -**Values are always strings.** A cell with `t="s"` reads `` as an index into -the shared-string table and parses the referenced ``; otherwise its own -``/`` children. `get_text` concatenates `t` and `v` nodes verbatim, so a -**formula's cached `` result is shown and `` is never evaluated**. No -numeric/date/bool typing — `sheet_cell_value_type` is hardcoded to `string`. +**Values are strings; types are advisory.** A cell with `t="s"` reads `` as +an index into the shared-string table and parses the referenced ``; +otherwise its own ``/`` children. `get_text` concatenates `t` and `v` +nodes verbatim, so a **formula's cached `` result is shown and `` is +never evaluated**. `sheet_cell_value_type` derives number-vs-string from +`c/@t` (default "n" → `float_number` when a `` exists; dates/booleans/errors +report `string`). Merged ranges from `mergeCells` land in the `SheetCell` side +map as anchor `span` + `is_covered` flags at parse time. **Style resolution: styles.xml index vectors.** `StyleRegistry` loads positional `fonts`/`fills`/`borders`/`cellStyleXfs`/`cellXfs`. A cell's `s` attribute @@ -54,14 +57,12 @@ inheritance). Coverage is in [`README.md`](README.md). Foundational gaps, roughly by value: -1. **Cell value types & formulas.** Everything is a string; numeric/date/bool - typing and formula evaluation are absent (`sheet_cell_value_type` → `string`). +1. **Formulas & rich value types.** `` is never evaluated (the cached `` + shows); dates, booleans, and errors are typed as plain strings. 2. **Content-range detection.** `sheet_content` ignores the requested range and returns the full `` — no trim to the populated range. -3. **Merged cells.** `sheet_cell_span` → `{1,1}`, `sheet_cell_is_covered` → - false; `mergeCells` not read. -4. **No named/master cell-style inheritance** (`cellStyleXfs` loaded but unused); +3. **No named/master cell-style inheritance** (`cellStyleXfs` loaded but unused); borders rendered as `0.75pt solid` regardless of actual style (`// TODO thin only`); cell protection unhandled. -5. **Read-only.** `text_set_content` is a no-op stub; `save` throws. Links and +4. **Read-only.** `text_set_content` is a no-op stub; `save` throws. Links and comments/annotations not modelled. diff --git a/src/odr/internal/ooxml/spreadsheet/README.md b/src/odr/internal/ooxml/spreadsheet/README.md index c5349e171..3a0dde0c3 100644 --- a/src/odr/internal/ooxml/spreadsheet/README.md +++ b/src/odr/internal/ooxml/spreadsheet/README.md @@ -21,9 +21,12 @@ Roughly ordered by importance. - [x] columns, rows, cells - [x] dimensions - [x] shared strings + - [x] merged cells (`mergeCells`) - [x] shapes / images anchored to a sheet (`xdr:twoCellAnchor`) - - [ ] cell value types (everything is reported as string) - - [ ] computed values (formulas are not evaluated) + - [x] cell value types (number vs. string; dates/booleans/errors reported as + string) + - [ ] computed values (formulas are not evaluated; the cached `` result is + shown) - [ ] edit - [ ] save diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index 5afa6d8e3..6bb277dd3 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -281,17 +281,28 @@ class ElementAdapter final : public abstract::ElementAdapter, sheet_cell_position(const ElementIdentifier element_id) const override { return m_registry->sheet_cell_element_at(element_id).position; } - [[nodiscard]] bool sheet_cell_is_covered( - [[maybe_unused]] const ElementIdentifier element_id) const override { - return false; // TODO + [[nodiscard]] bool + sheet_cell_is_covered(const ElementIdentifier element_id) const override { + return m_registry->sheet_cell_element_at(element_id).is_covered; } - [[nodiscard]] TableDimensions sheet_cell_span( - [[maybe_unused]] const ElementIdentifier element_id) const override { - return {1, 1}; // TODO + [[nodiscard]] TableDimensions + sheet_cell_span(const ElementIdentifier element_id) const override { + return m_registry->sheet_cell_element_at(element_id).span; } - [[nodiscard]] ValueType sheet_cell_value_type( - [[maybe_unused]] const ElementIdentifier element_id) const override { - return ValueType::string; // TODO + [[nodiscard]] ValueType + sheet_cell_value_type(const ElementIdentifier element_id) const override { + // ECMA-376 `c/@t` defaults to "n" (number); strings come as shared ("s"), + // inline ("inlineStr"), or formula ("str") cells. + const pugi::xml_node node = get_node(element_id); + const std::string type = node.attribute("t").value(); + if (type == "s" || type == "str" || type == "inlineStr" || type == "b" || + type == "e" || type == "d") { + return ValueType::string; + } + if (node.child("v")) { + return ValueType::float_number; + } + return ValueType::string; } [[nodiscard]] TextStyle diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.cpp index 2a8cd2233..db5c2a7f6 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.cpp @@ -82,6 +82,12 @@ ElementRegistry::sheet_element_at(const ElementIdentifier id) { return m_sheets.at(id); } +ElementRegistry::SheetCell & +ElementRegistry::sheet_cell_element_at(const ElementIdentifier id) { + check_sheet_cell_id(id); + return m_sheet_cells.at(id); +} + const ElementRegistry::Element & ElementRegistry::element_at(const ElementIdentifier id) const { check_element_id(id); diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.hpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.hpp index b9b6e6e04..dbcb67c5a 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.hpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.hpp @@ -80,6 +80,8 @@ class ElementRegistry final { struct SheetCell final { TablePosition position; + TableDimensions span{1, 1}; + bool is_covered{false}; }; void clear() noexcept; @@ -101,6 +103,7 @@ class ElementRegistry final { [[nodiscard]] Element &element_at(ElementIdentifier id); [[nodiscard]] Sheet &sheet_element_at(ElementIdentifier id); + [[nodiscard]] SheetCell &sheet_cell_element_at(ElementIdentifier id); [[nodiscard]] const Element &element_at(ElementIdentifier id) const; [[nodiscard]] const Text &text_element_at(ElementIdentifier id) const; diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp index b43ace04d..7cff6ad18 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_parser.cpp @@ -123,6 +123,40 @@ parse_sheet_element(ElementRegistry ®istry, const ParseContext &context, } } + for (const pugi::xml_node merge_node : + node.child("mergeCells").children("mergeCell")) { + const std::string ref = merge_node.attribute("ref").value(); + if (ref.find(':') == std::string::npos) { + continue; + } + const TableRange range(ref); + + // The renderer's cursor learns covered positions only from the anchor's + // span, so a range whose anchor cell is absent from sheetData must be + // dropped entirely — stray covered flags would starve the cursor. + const ElementRegistry::Sheet::Cell *anchor = + sheet.cell(range.from().column, range.from().row); + if (anchor == nullptr) { + continue; + } + registry.sheet_cell_element_at(anchor->element_id).span = + TableDimensions(range.to().row - range.from().row + 1, + range.to().column - range.from().column + 1); + + for (std::uint32_t row = range.from().row; row <= range.to().row; ++row) { + for (std::uint32_t column = range.from().column; + column <= range.to().column; ++column) { + if (row == range.from().row && column == range.from().column) { + continue; + } + if (const ElementRegistry::Sheet::Cell *cell = sheet.cell(column, row); + cell != nullptr) { + registry.sheet_cell_element_at(cell->element_id).is_covered = true; + } + } + } + } + { const std::string dimension_ref = node.child("dimension").attribute("ref").value(); diff --git a/src/odr/internal/ooxml/text/AGENTS.md b/src/odr/internal/ooxml/text/AGENTS.md index a1379897a..e56c7cd1b 100644 --- a/src/odr/internal/ooxml/text/AGENTS.md +++ b/src/odr/internal/ooxml/text/AGENTS.md @@ -18,6 +18,12 @@ README. Element; `get_text` maps `w:tab`→`\t`. Unknown tags are skipped (children still visited). +**Table merges are resolved in the adapter.** Colspan is `w:tcPr/w:gridSpan`; +a `w:vMerge` continuation (`w:val` absent or "continue") reports the cell as +covered, and the restart cell computes its rowspan by walking following `w:tr` +siblings for a continuation at the same grid column (grid column = sum of +preceding cells' `gridSpan`s). + **Lists are detected structurally**, before the tag table: a paragraph with `w:pPr/w:numPr` is a list item, nesting synthesised from the `w:ilvl` level. @@ -61,12 +67,7 @@ Style/element coverage is in [`README.md`](README.md). Foundational gaps: 3. **Theme fonts unhandled.** `w:rFonts w:asciiTheme="minorHAnsi"` (etc.) is ignored — only literal `w:ascii` names are read (README example `Sample large docx.docx`). -4. **Latent copy-paste from the ODF adapter.** The table/cell adapter methods - query ODF node names (`table:covered-table-cell`, `office:value-type`, - `table:number-columns-repeated`) that never match docx `w:` nodes, so - dimension/span/covered-cell logic is effectively dead for docx. Verify and - rewrite against `w:` names when tables get proper attention. -5. **Style stubs**: `resolve_table_row_style_` and `resolve_graphic_style_` are +4. **Style stubs**: `resolve_table_row_style_` and `resolve_graphic_style_` are empty; table cell width is parsed but not applied; the `w:default="1"` style flag is ignored. -6. **Comments / annotations** not modelled. +5. **Comments / annotations** not modelled. diff --git a/src/odr/internal/ooxml/text/README.md b/src/odr/internal/ooxml/text/README.md index d0334ca90..a19dffea4 100644 --- a/src/odr/internal/ooxml/text/README.md +++ b/src/odr/internal/ooxml/text/README.md @@ -29,7 +29,8 @@ Roughly ordered by importance. - [x] line breaks - [x] hyperlinks - [x] bookmarks -- [x] tables (grid columns, rows, cells) +- [x] tables (grid columns, rows, cells, merged cells via + `w:gridSpan`/`w:vMerge`) - [x] images (`w:drawing`) - [x] structured document tags (rendered as generic groups) - [x] listings diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.cpp b/src/odr/internal/ooxml/text/ooxml_text_document.cpp index b6058acf4..e559ee124 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.cpp @@ -6,7 +6,6 @@ #include #include -#include #include #include #include @@ -16,6 +15,7 @@ #include #include +#include #include namespace odr::internal::ooxml::text { @@ -352,7 +352,7 @@ class ElementAdapter final : public abstract::ElementAdapter, [[nodiscard]] std::string bookmark_name(const ElementIdentifier element_id) const override { - return get_node(element_id).attribute("text:name").value(); + return get_node(element_id).attribute("w:name").value(); } [[nodiscard]] TextStyle @@ -365,25 +365,10 @@ class ElementAdapter final : public abstract::ElementAdapter, const pugi::xml_node node = get_node(element_id); TableDimensions result; - TableCursor cursor; - - for (auto column : node.children("table:table-column")) { - const auto columns_repeated = - column.attribute("table:number-columns-repeated").as_uint(1); - cursor.add_column(columns_repeated); - } - - result.columns = cursor.column(); - cursor = {}; - - for (auto row : node.children("table:table-row")) { - const auto rows_repeated = - row.attribute("table:number-rows-repeated").as_uint(1); - cursor.add_row(rows_repeated); - } - - result.rows = cursor.row(); - + result.columns = static_cast( + std::ranges::distance(node.child("w:tblGrid").children("w:gridCol"))); + result.rows = static_cast( + std::ranges::distance(node.children("w:tr"))); return result; } [[nodiscard]] ElementIdentifier @@ -421,22 +406,39 @@ class ElementAdapter final : public abstract::ElementAdapter, [[nodiscard]] bool table_cell_is_covered(const ElementIdentifier element_id) const override { - const pugi::xml_node node = get_node(element_id); - return std::strcmp(node.name(), "table:covered-table-cell") == 0; + // A cell continues a vertical merge when `w:vMerge` is present without + // `w:val` or with `w:val="continue"`. + return is_merge_continuation( + get_node(element_id).child("w:tcPr").child("w:vMerge")); } [[nodiscard]] TableDimensions table_cell_span(const ElementIdentifier element_id) const override { const pugi::xml_node node = get_node(element_id); - return {node.attribute("table:number-rows-spanned").as_uint(1), - node.attribute("table:number-columns-spanned").as_uint(1)}; - } - [[nodiscard]] ValueType - table_cell_value_type(const ElementIdentifier element_id) const override { - const pugi::xml_node node = get_node(element_id); - if (const char *value_type = node.attribute("office:value-type").value(); - std::strcmp("float", value_type) == 0) { - return ValueType::float_number; + const pugi::xml_node properties = node.child("w:tcPr"); + + TableDimensions result{1, 1}; + result.columns = + properties.child("w:gridSpan").attribute("w:val").as_uint(1); + + if (const pugi::xml_node merge = properties.child("w:vMerge"); + merge && !is_merge_continuation(merge)) { + const std::uint32_t grid_column = get_grid_column(node); + for (pugi::xml_node row_node = node.parent().next_sibling("w:tr"); + row_node; row_node = row_node.next_sibling("w:tr")) { + const pugi::xml_node cell_node = + get_cell_at_grid_column(row_node, grid_column); + if (!is_merge_continuation( + cell_node.child("w:tcPr").child("w:vMerge"))) { + break; + } + ++result.rows; + } } + + return result; + } + [[nodiscard]] ValueType table_cell_value_type( + [[maybe_unused]] const ElementIdentifier element_id) const override { return ValueType::string; } [[nodiscard]] TableCellStyle @@ -541,6 +543,47 @@ class ElementAdapter final : public abstract::ElementAdapter, return {}; } + [[nodiscard]] static bool is_merge_continuation(const pugi::xml_node merge) { + if (!merge) { + return false; + } + const pugi::xml_attribute val = merge.attribute("w:val"); + return !val || std::strcmp(val.value(), "continue") == 0; + } + + /// Grid column a `w:tc` starts at, i.e. the preceding cells' `w:gridSpan`s. + [[nodiscard]] static std::uint32_t + get_grid_column(const pugi::xml_node cell_node) { + std::uint32_t grid_column = 0; + for (pugi::xml_node sibling = cell_node.previous_sibling("w:tc"); sibling; + sibling = sibling.previous_sibling("w:tc")) { + grid_column += sibling.child("w:tcPr") + .child("w:gridSpan") + .attribute("w:val") + .as_uint(1); + } + return grid_column; + } + + [[nodiscard]] static pugi::xml_node + get_cell_at_grid_column(const pugi::xml_node row_node, + const std::uint32_t grid_column) { + std::uint32_t column = 0; + for (const pugi::xml_node cell_node : row_node.children("w:tc")) { + if (column == grid_column) { + return cell_node; + } + if (column > grid_column) { + break; + } + column += cell_node.child("w:tcPr") + .child("w:gridSpan") + .attribute("w:val") + .as_uint(1); + } + return {}; + } + [[nodiscard]] const Relations &get_document_relations() const { return m_document->document_relations(); } diff --git a/test/src/internal/common/table_cursor_test.cpp b/test/src/internal/common/table_cursor_test.cpp index 0d7d2251c..83c343240 100644 --- a/test/src/internal/common/table_cursor_test.cpp +++ b/test/src/internal/common/table_cursor_test.cpp @@ -2,8 +2,6 @@ #include -#include - using namespace odr::internal; TEST(TableCursor, test) { @@ -30,3 +28,20 @@ TEST(TableCursor, test) { EXPECT_EQ(0, cursor.column()); EXPECT_EQ(3, cursor.row()); } + +// A long-rowspan cell followed in a later row by rowspan cells at smaller +// columns used to leave the pending covered ranges unsorted, so the skip in +// `handle_rowspan_` missed them (infinite loop in the sheet renderer). +TEST(TableCursor, out_of_order_rowspans) { + TableCursor cursor; + cursor.add_cell(1, 1); // A1 + cursor.add_cell(1, 3); // B1, covers B2 + B3 + cursor.add_cell(1, 1); // C1 + cursor.add_row(); + EXPECT_EQ(0, cursor.column()); + cursor.add_cell(1, 2); // A2, covers A3 + EXPECT_EQ(2, cursor.column()); // B2 covered, skipped + cursor.add_cell(1, 1); // C2 + cursor.add_row(); + EXPECT_EQ(2, cursor.column()); // A3 and B3 covered, skipped +} From 4cabcdfaee03b857d1bf2a2f0a1f0a46c2fa7fda Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 16 Jul 2026 21:25:39 +0200 Subject: [PATCH 2/4] fix(pptx): parse a:txBody in table cells Table cell text sits under a:tc/a:txBody/a:p, but only p:txBody was registered in the tag dispatch table, so cell text was dropped. Addresses review feedback on #607. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AEvWWwe1EyeWH7ANHoAvPH --- .../internal/ooxml/presentation/ooxml_presentation_parser.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp index 4452c80dc..a233d5719 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_parser.cpp @@ -168,6 +168,7 @@ parse_any_element_tree(ElementRegistry ®istry, const ParseContext &context, {"p:graphicFrame", create_default_tree_parser( ElementType::frame, parse_graphic_frame_children)}, {"p:txBody", create_default_tree_parser(ElementType::group)}, + {"a:txBody", create_default_tree_parser(ElementType::group)}, {"a:t", parse_text_element}, {"a:tab", parse_text_element}, {"a:p", create_default_tree_parser(ElementType::paragraph)}, From 8482ba37fd8f25ee0920574da9d69a0d46a95cb9 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 16 Jul 2026 23:27:56 +0200 Subject: [PATCH 3/4] update refs --- test/data/reference-output/odr-private | 2 +- test/data/reference-output/odr-public | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/data/reference-output/odr-private b/test/data/reference-output/odr-private index 54d96fc25..81035e219 160000 --- a/test/data/reference-output/odr-private +++ b/test/data/reference-output/odr-private @@ -1 +1 @@ -Subproject commit 54d96fc254ccd78629facaa3d74e2934ba6e6ab0 +Subproject commit 81035e219046c7149d6c68b4fe1d0b5f3815cc68 diff --git a/test/data/reference-output/odr-public b/test/data/reference-output/odr-public index d5ae180a2..21d53d9f8 160000 --- a/test/data/reference-output/odr-public +++ b/test/data/reference-output/odr-public @@ -1 +1 @@ -Subproject commit d5ae180a2992d28e3be2f6bdf6197b4be492898e +Subproject commit 21d53d9f89f3e5ff5fe69d4622f3544060b053c4 From 62aa9e0ee544a8f7bb5121ab4d99d52f3d75d10f Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Fri, 17 Jul 2026 13:03:39 +0200 Subject: [PATCH 4/4] update refs again --- test/data/reference-output/odr-private | 2 +- test/data/reference-output/odr-public | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/data/reference-output/odr-private b/test/data/reference-output/odr-private index 81035e219..88acbeee2 160000 --- a/test/data/reference-output/odr-private +++ b/test/data/reference-output/odr-private @@ -1 +1 @@ -Subproject commit 81035e219046c7149d6c68b4fe1d0b5f3815cc68 +Subproject commit 88acbeee24a474b147e1db41511aa8271afc9394 diff --git a/test/data/reference-output/odr-public b/test/data/reference-output/odr-public index 21d53d9f8..cada946ed 160000 --- a/test/data/reference-output/odr-public +++ b/test/data/reference-output/odr-public @@ -1 +1 @@ -Subproject commit 21d53d9f89f3e5ff5fe69d4622f3544060b053c4 +Subproject commit cada946ed3927d0606c209bece66cf4275f985b7