diff --git a/src/odr/internal/common/table_cursor.cpp b/src/odr/internal/common/table_cursor.cpp index ec1ce836..5a8d1885 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 e977d0c1..3422667e 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 80d078ab..40428b2f 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 f171d780..5b07c194 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 39b05865..4869232e 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 73edf0fa..d0a901f8 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 a1fc22a4..a233d571 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,15 @@ 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: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 56831c59..6c8d8cf4 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 c5349e17..3a0dde0c 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 5afa6d8e..6bb277dd 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 2a8cd223..db5c2a7f 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 b9b6e6e0..dbcb67c5 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 b43ace04..7cff6ad1 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 a1379897..e56c7cd1 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 d0334ca9..a19dffea 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 b6058acf..e559ee12 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/data/reference-output/odr-private b/test/data/reference-output/odr-private index 54d96fc2..88acbeee 160000 --- a/test/data/reference-output/odr-private +++ b/test/data/reference-output/odr-private @@ -1 +1 @@ -Subproject commit 54d96fc254ccd78629facaa3d74e2934ba6e6ab0 +Subproject commit 88acbeee24a474b147e1db41511aa8271afc9394 diff --git a/test/data/reference-output/odr-public b/test/data/reference-output/odr-public index d5ae180a..cada946e 160000 --- a/test/data/reference-output/odr-public +++ b/test/data/reference-output/odr-public @@ -1 +1 @@ -Subproject commit d5ae180a2992d28e3be2f6bdf6197b4be492898e +Subproject commit cada946ed3927d0606c209bece66cf4275f985b7 diff --git a/test/src/internal/common/table_cursor_test.cpp b/test/src/internal/common/table_cursor_test.cpp index 0d7d2251..83c34324 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 +}