From a38774ac4b11be062f6dcbf2ae6d0e05d80c3121 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 20:11:35 +0200 Subject: [PATCH 1/3] feat(ooxml): style a cell of an xlsx file setCellStyle now writes into .xlsx. The writer starts from the xf that the cell shows (its own s, else the row s where the row states customFormat, else the column style), copies it and its font, and applies the delta. It appends a font, a fill and an xf only where no equal one exists, and sets applyFont, applyFill and applyAlignment. A font keeps the child order of CT_Font, and an empty styleSheet first gets the entries that every xf needs. styles.xml is now a written part, so every save writes it from its dom. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01NXnz6EZY8YpyiyyE1GpsWy --- CHANGELOG.md | 4 +- docs/design/spreadsheet-editing.md | 3 +- src/odr/internal/ooxml/spreadsheet/AGENTS.md | 11 +- .../ooxml_spreadsheet_document.cpp | 64 ++++- .../ooxml_spreadsheet_document.hpp | 1 + .../spreadsheet/ooxml_spreadsheet_style.cpp | 227 +++++++++++++++++- .../spreadsheet/ooxml_spreadsheet_style.hpp | 9 + test/CMakeLists.txt | 1 + .../ooxml_spreadsheet_style_write_test.cpp | 225 +++++++++++++++++ .../ooxml/ooxml_spreadsheet_test_util.hpp | 8 +- 10 files changed, 540 insertions(+), 13 deletions(-) create mode 100644 test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 9458df38b..67ced8986 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,8 +16,8 @@ The release run heads these entries with the version and opens a fresh ## Unreleased -- A cell of an ods file takes a style: `Sheet::set_cell_style` and the - `setCellStyle` op write the fill, the horizontal alignment, and bold, +- A cell of an ods or xlsx file takes a style: `Sheet::set_cell_style` and + the `setCellStyle` op write the fill, the horizontal alignment, and bold, italic, underline, strikethrough, colour and size. - **Fix**: in an ods file, a value written into a covered cell landed outside diff --git a/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index eebac7a6c..f4ba83a38 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -195,7 +195,8 @@ separate scripts. The coordinates are the ones an op names, never a DOM index. ## Cell formatting -Status: `.ods` writes a cell style. The steps land as a stack, in this order: +Status: `.ods` and `.xlsx` write a cell style. The steps land as a stack, in +this order: 1. The xlsx reader reads what the writer writes: a solid fill from `fgColor`, theme colours with their `tint`, italic, underline and strikethrough, and diff --git a/src/odr/internal/ooxml/spreadsheet/AGENTS.md b/src/odr/internal/ooxml/spreadsheet/AGENTS.md index 1308472ef..01059e325 100644 --- a/src/odr/internal/ooxml/spreadsheet/AGENTS.md +++ b/src/odr/internal/ooxml/spreadsheet/AGENTS.md @@ -7,7 +7,7 @@ The design of the xlsx module. The feature checklist is in Scope: read `xl/workbook.xml`, its sheets, the shared strings and the drawings into the abstract model, one table per sheet. Cell styles resolve from -`xl/styles.xml`. Write a cell value, and save. +`xl/styles.xml`. Write a cell value and a cell style, and save. ## Design decisions @@ -60,6 +60,15 @@ their ids and stop being reachable. A shared string is never written back into it, so the cell becomes `t="inlineStr"`. A covered cell, a cell holding an `f`, and a date, time or error value throw `UnsupportedOperation`. +**A cell style is a new `xf`, never an edit of the one the cell names.** +`sheet_set_cell_style` starts from the cell's `s`, else its row's where the +row states `customFormat`, else its column's `style`. `create_cell_format` +copies that `xf` and its `font`, applies the delta, and appends a `font`, a +`fill` and an `xf` only where no equal one exists, with `applyFont`, +`applyFill` and `applyAlignment` set. A `font` keeps the child order of +`CT_Font`. An empty `styleSheet` first gets the entries every `xf` needs. +`styles.xml` is a written part, so every save writes it from its dom. + A position the file states no `c` for is stated by `insert_cell`: the `c` goes into its row in column order, a missing `row` into `sheetData` in row order, and `dimension` widens around it. The cell map is keyed by position, so an diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index ee8c01923..dec8bc3fa 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -32,7 +32,7 @@ namespace odr::internal::ooxml::spreadsheet { namespace { std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry); +create_element_adapter(Document &document, ElementRegistry ®istry); /// The `workbook` `calcPr`, appended where it is missing. ECMA-376 18.2.27 /// orders the children, so a new one goes before the first that must follow it. @@ -60,7 +60,9 @@ Document::Document(std::shared_ptr files) const AbsPath workbook_path("/xl/workbook.xml"); const auto [workbook_xml, workbook_relations] = parse_xml_(workbook_path); m_written_parts.push_back(workbook_path); - const auto [styles_xml, _] = parse_xml_(AbsPath("/xl/styles.xml")); + const AbsPath styles_path("/xl/styles.xml"); + const auto [styles_xml, _] = parse_xml_(styles_path); + m_written_parts.push_back(styles_path); for (pugi::xml_node sheet_node : workbook_xml.document_element().child("sheets").children("sheet")) { @@ -112,6 +114,8 @@ const StyleRegistry &Document::style_registry() const { return m_style_registry; } +StyleRegistry &Document::style_registry() { return m_style_registry; } + bool Document::is_editable() const noexcept { return true; } bool Document::is_savable(const bool encrypted) const noexcept { @@ -184,7 +188,7 @@ using AdapterBase = internal::RegistryElementAdapter< class ElementAdapter final : public AdapterBase { public: - ElementAdapter(const Document &document, ElementRegistry ®istry) + ElementAdapter(Document &document, ElementRegistry ®istry) : AdapterBase(registry), m_document(&document) {} [[nodiscard]] std::string @@ -347,6 +351,56 @@ class ElementAdapter final : public AdapterBase { } return result; } + /// [ECMA-376] 18.3.1.4: a `c` without `s` shows its row's `s` where the + /// row states `customFormat`, else its column's `style`. + void sheet_set_cell_style(const ElementIdentifier element_id, + const std::uint32_t column, const std::uint32_t row, + const TableCellStyle &cell_style, + const TextStyle &text_style) const override { + const ElementRegistry::Sheet &sheet = + m_registry->sheet_element_at(element_id); + const ElementRegistry::Sheet::Cell *cell = sheet.cell(column, row); + + ElementIdentifier cell_id = null_element_id; + if (cell == nullptr) { + cell_id = insert_cell(element_id, column, row); + } else { + cell_id = cell->element_id; + if (m_registry->sheet_cell_element_at(cell_id).is_covered) { + throw UnsupportedOperation(); + } + } + + pugi::xml_node node = get_node(cell_id); + std::uint32_t base = 0; + if (const pugi::xml_attribute style = node.attribute("s")) { + base = style.as_uint(); + } else if (const pugi::xml_node row_node = node.parent(); + row_node.attribute("customFormat").as_bool()) { + base = row_node.attribute("s").as_uint(); + } else { + for (const pugi::xml_node col : + get_node(element_id).child("cols").children("col")) { + if (col.attribute("min").as_uint() <= column + 1 && + column + 1 <= col.attribute("max").as_uint()) { + base = col.attribute("style").as_uint(); + break; + } + } + } + + const std::uint32_t format = + m_document->style_registry().create_cell_format(base, cell_style, + text_style); + pugi::xml_attribute attribute = node.attribute("s"); + if (!attribute) { + const pugi::xml_attribute reference = node.attribute("r"); + attribute = reference ? node.insert_attribute_after("s", reference) + : node.prepend_attribute("s"); + } + attribute.set_value(format); + } + [[nodiscard]] TableCellStyle sheet_cell_style(const ElementIdentifier element_id, const std::uint32_t column, @@ -593,7 +647,7 @@ class ElementAdapter final : public AdapterBase { } private: - const Document *m_document{nullptr}; + Document *m_document{nullptr}; [[nodiscard]] pugi::xml_node get_node(const ElementIdentifier element_id) const { @@ -763,7 +817,7 @@ class ElementAdapter final : public AdapterBase { }; std::unique_ptr -create_element_adapter(const Document &document, ElementRegistry ®istry) { +create_element_adapter(Document &document, ElementRegistry ®istry) { return std::make_unique(document, registry); } diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp index a9059847e..999a7cd7b 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.hpp @@ -21,6 +21,7 @@ class Document final : public internal::Document { [[nodiscard]] const ElementRegistry &element_registry() const; [[nodiscard]] const StyleRegistry &style_registry() const; + StyleRegistry &style_registry(); [[nodiscard]] bool is_editable() const noexcept override; [[nodiscard]] bool is_savable(bool encrypted) const noexcept override; diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp index fc3e74449..e08f377a7 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp @@ -3,9 +3,17 @@ #include #include +#include +#include #include +#include +#include +#include +#include #include +#include + namespace odr::internal::ooxml::spreadsheet { namespace { @@ -68,12 +76,136 @@ bool read_toggle(const pugi::xml_node node) { return node && node.attribute("val").as_bool(true); } +/// The child @p name of @p parent, made at its place in @p order. +pugi::xml_node ordered_child(pugi::xml_node parent, const char *name, + const std::span order) { + if (const pugi::xml_node existing = parent.child(name)) { + return existing; + } + const auto rank = [&](const std::string_view child) { + return std::ranges::find(order, child) - std::begin(order); + }; + for (const pugi::xml_node child : parent.children()) { + if (rank(child.name()) > rank(name)) { + return parent.insert_child_before(name, child); + } + } + return parent.append_child(name); +} + +/// The @p name child of `styleSheet`, made in the order [ECMA-376] 18.8.39 +/// gives them where it is missing. +pugi::xml_node collection_of(pugi::xml_node root, const char *name) { + static constexpr std::array order{ + "numFmts", "fonts", "fills", "borders", + "cellStyleXfs", "cellXfs", "cellStyles", "dxfs", + "tableStyles", "colors", "extLst"}; + return ordered_child(root, name, order); +} + +void set_attribute(pugi::xml_node node, const char *name, const char *value) { + pugi::xml_attribute attribute = node.attribute(name); + if (!attribute) { + attribute = node.append_attribute(name); + } + attribute.set_value(value); +} + +std::string argb_of(const Color &color) { + return fmt::format("FF{:06X}", color.rgb()); +} + +/// [ECMA-376] 18.8.22 `CT_Font` is a sequence. +constexpr std::array font_order{ + "b", "i", "strike", "condense", "extend", "outline", "shadow", "u", + "vertAlign", "sz", "color", "name", "family", "charset", "scheme"}; + +/// `val` is dropped for on, as Excel writes it, and is `0` for off. +void set_toggle(pugi::xml_node font, const char *name, const bool on) { + pugi::xml_node node = ordered_child(font, name, font_order); + node.remove_attribute("val"); + if (!on) { + node.append_attribute("val").set_value("0"); + } +} + +std::string serialized(const pugi::xml_node node) { + std::ostringstream out; + node.print(out, "", pugi::format_raw); + return out.str(); +} + +/// The index of the child of @p collection equal to @p node, which is +/// appended where none is, with the `count` kept. +std::uint32_t intern(pugi::xml_node collection, const pugi::xml_node node) { + const std::string wanted = serialized(node); + std::uint32_t index = 0; + for (const pugi::xml_node child : collection.children()) { + if (serialized(child) == wanted) { + return index; + } + ++index; + } + collection.append_copy(node); + set_attribute(collection, "count", std::to_string(index + 1).c_str()); + return index; +} + +/// What an empty `styleSheet` lacks and every `xf` needs a first entry of. +void state_defaults(pugi::xml_node root) { + if (pugi::xml_node fonts = collection_of(root, "fonts"); + !fonts.first_child()) { + pugi::xml_node font = fonts.append_child("font"); + font.append_child("sz").append_attribute("val").set_value("11"); + font.append_child("name").append_attribute("val").set_value("Calibri"); + set_attribute(fonts, "count", "1"); + } + if (pugi::xml_node fills = collection_of(root, "fills"); + !fills.first_child()) { + // 18.8.21: the first two are reserved + fills.append_child("fill") + .append_child("patternFill") + .append_attribute("patternType") + .set_value("none"); + fills.append_child("fill") + .append_child("patternFill") + .append_attribute("patternType") + .set_value("gray125"); + set_attribute(fills, "count", "2"); + } + if (pugi::xml_node borders = collection_of(root, "borders"); + !borders.first_child()) { + borders.append_child("border"); + set_attribute(borders, "count", "1"); + } + const auto default_xf = [](pugi::xml_node xf) { + xf.append_attribute("numFmtId").set_value("0"); + xf.append_attribute("fontId").set_value("0"); + xf.append_attribute("fillId").set_value("0"); + xf.append_attribute("borderId").set_value("0"); + return xf; + }; + if (pugi::xml_node masters = collection_of(root, "cellStyleXfs"); + !masters.first_child()) { + default_xf(masters.append_child("xf")); + set_attribute(masters, "count", "1"); + } + if (pugi::xml_node formats = collection_of(root, "cellXfs"); + !formats.first_child()) { + default_xf(formats.append_child("xf")) + .append_attribute("xfId") + .set_value("0"); + set_attribute(formats, "count", "1"); + } +} + } // namespace StyleRegistry::StyleRegistry() = default; StyleRegistry::StyleRegistry(const pugi::xml_node styles_root, - const pugi::xml_node theme_root) { + const pugi::xml_node theme_root) + : m_styles_root{styles_root} { const pugi::xml_node scheme = theme_root.child("a:themeElements").child("a:clrScheme"); for (const char *name : {"a:lt1", "a:dk1", "a:lt2", "a:dk2", "a:accent1", @@ -222,7 +354,100 @@ void StyleRegistry::resolve_border_(const std::uint32_t i, result.table_cell_style.border.bottom = side(border.child("bottom")); } +std::uint32_t +StyleRegistry::create_cell_format(const std::uint32_t base, + const TableCellStyle &cell_style, + const TextStyle &text_style) { + state_defaults(m_styles_root); + generate_indices_(m_styles_root); + + pugi::xml_document scratch; + pugi::xml_node xf = scratch.append_copy( + m_cell_formats_index.at(base < m_cell_formats_index.size() ? base : 0)); + + if (text_style.font_weight || text_style.font_style || + text_style.font_underline || text_style.font_line_through || + text_style.font_color || text_style.font_size) { + const std::uint32_t font_id = xf.attribute("fontId").as_uint(); + pugi::xml_node font = scratch.append_copy( + m_fonts_index.at(font_id < m_fonts_index.size() ? font_id : 0)); + if (text_style.font_weight) { + set_toggle(font, "b", *text_style.font_weight == FontWeight::bold); + } + if (text_style.font_style) { + set_toggle(font, "i", *text_style.font_style == FontStyle::italic); + } + if (text_style.font_line_through) { + set_toggle(font, "strike", *text_style.font_line_through); + } + if (text_style.font_underline) { + pugi::xml_node underline = ordered_child(font, "u", font_order); + underline.remove_attribute("val"); + if (!*text_style.font_underline) { + underline.append_attribute("val").set_value("none"); + } + } + if (text_style.font_size) { + set_attribute(ordered_child(font, "sz", font_order), "val", + fmt::format("{:g}", points(*text_style.font_size)).c_str()); + } + if (text_style.font_color) { + pugi::xml_node color = ordered_child(font, "color", font_order); + color.remove_attributes(); + color.append_attribute("rgb").set_value( + argb_of(*text_style.font_color).c_str()); + } + set_attribute( + xf, "fontId", + std::to_string(intern(m_styles_root.child("fonts"), font)).c_str()); + set_attribute(xf, "applyFont", "1"); + } + + if (cell_style.background_color) { + pugi::xml_node fill = scratch.append_child("fill"); + pugi::xml_node pattern = fill.append_child("patternFill"); + if (cell_style.background_color->alpha == 0) { + pattern.append_attribute("patternType").set_value("none"); + } else { + pattern.append_attribute("patternType").set_value("solid"); + pattern.append_child("fgColor").append_attribute("rgb").set_value( + argb_of(*cell_style.background_color).c_str()); + pattern.append_child("bgColor").append_attribute("indexed").set_value( + "64"); + } + set_attribute( + xf, "fillId", + std::to_string(intern(m_styles_root.child("fills"), fill)).c_str()); + set_attribute(xf, "applyFill", "1"); + } + + if (cell_style.horizontal_align) { + pugi::xml_node alignment = xf.child("alignment"); + if (!alignment) { + alignment = xf.prepend_child("alignment"); + } + const char *horizontal = "left"; + if (*cell_style.horizontal_align == HorizontalAlign::center) { + horizontal = "center"; + } else if (*cell_style.horizontal_align == HorizontalAlign::right) { + horizontal = "right"; + } + set_attribute(alignment, "horizontal", horizontal); + set_attribute(xf, "applyAlignment", "1"); + } + + const std::uint32_t result = intern(m_styles_root.child("cellXfs"), xf); + generate_indices_(m_styles_root); + return result; +} + void StyleRegistry::generate_indices_(const pugi::xml_node styles_root) { + m_fonts_index.clear(); + m_fills_index.clear(); + m_borders_index.clear(); + m_cell_masters_index.clear(); + m_cell_formats_index.clear(); + for (const pugi::xml_node font : styles_root.child("fonts")) { m_fonts_index.push_back(font); } diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.hpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.hpp index 528741598..25dda9725 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.hpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.hpp @@ -2,6 +2,8 @@ #include +#include + #include #include @@ -17,7 +19,14 @@ class StyleRegistry final { [[nodiscard]] ResolvedStyle cell_style(std::uint32_t i) const; + /// The `cellXfs` index of @p base with the delta applied. An equal `xf`, + /// `font` or `fill` is reused, else one is appended. + std::uint32_t create_cell_format(std::uint32_t base, + const TableCellStyle &cell_style, + const TextStyle &text_style); + private: + pugi::xml_node m_styles_root; /// `lt1`, `dk1`, `lt2`, `dk2`, `accent1` to `accent6`, `hlink`, `folHlink`: /// the order a `theme` index counts in. std::vector> m_theme_colors; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index f2c243510..6d272478b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -101,6 +101,7 @@ add_executable(odr_test "src/internal/ooxml/ooxml_text_style_test.cpp" "src/internal/ooxml/ooxml_spreadsheet_merge_test.cpp" "src/internal/ooxml/ooxml_spreadsheet_style_test.cpp" + "src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp" "src/internal/ooxml/ooxml_spreadsheet_value_test.cpp" "src/internal/ooxml/ooxml_spreadsheet_write_test.cpp" "src/internal/ooxml/ooxml_util_test.cpp" diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp new file mode 100644 index 000000000..7ebb80134 --- /dev/null +++ b/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp @@ -0,0 +1,225 @@ +#include +#include +#include +#include +#include +#include +#include + +#include + +#include + +#include +#include +#include +#include +#include + +using namespace odr; +using namespace odr::test::ooxml; + +namespace { + +constexpr const char *one_string = + R"(a)"; + +/// Two cells sharing `xf` 1, which fills them red. +constexpr const char *two_red = + R"()" + R"(a)" + R"(b)" + R"()"; +constexpr const char *red_styles = + R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + R"()" + R"()"; + +TableCellStyle fill(const Color color) { + TableCellStyle style; + style.background_color = color; + return style; +} + +TextStyle bold() { + TextStyle style; + style.font_weight = FontWeight::bold; + return style; +} + +std::optional fill_at(const Sheet &sheet, + const std::uint32_t column, + const std::uint32_t row) { + const std::optional color = + sheet.cell_style(column, row).background_color; + return color ? std::optional(color->rgb()) : std::nullopt; +} + +/// The style the one run of the cell shows. +TextStyle text_style_at(const Sheet &sheet, const std::uint32_t column) { + return sheet.cell(column, 0).first_child().as_text().style(); +} + +Document reopened(const Document &document) { + std::ostringstream saved; + document.save(saved); + return open(File::from_memory(saved.str())).as_document_file().document(); +} + +std::string styles_of(const Document &document) { + std::ostringstream xml; + xml << reopened(document) + .as_filesystem() + .open("/xl/styles.xml") + .stream() + ->rdbuf(); + return xml.str(); +} + +std::size_t count(const std::string &text, const std::string &part) { + std::size_t result = 0; + for (std::size_t at = text.find(part); at != std::string::npos; + at = text.find(part, at + 1)) { + ++result; + } + return result; +} + +} // namespace + +TEST(OoxmlSpreadsheetStyleWrite, + a_fill_survives_a_save_of_an_empty_style_sheet) { + const Document document = decode(workbook(one_string)); + + first_sheet(document).set_cell_style(0, 0, fill(0xffff00_rgb), {}); + + EXPECT_EQ(fill_at(first_sheet(document), 0, 0), 0xffff00u); + const Document saved = reopened(document); + EXPECT_EQ(fill_at(first_sheet(saved), 0, 0), 0xffff00u); + EXPECT_EQ(first_sheet(saved).cell(0, 0).value().text(), "a"); +} + +TEST(OoxmlSpreadsheetStyleWrite, a_font_takes_every_text_key) { + const Document document = decode(workbook(one_string)); + TextStyle style; + style.font_weight = FontWeight::bold; + style.font_style = FontStyle::italic; + style.font_underline = true; + style.font_line_through = true; + style.font_color = 0xcc0000_rgb; + style.font_size = Measure("16pt"); + + first_sheet(document).set_cell_style(0, 0, {}, style); + + const TextStyle read = text_style_at(first_sheet(reopened(document)), 0); + EXPECT_EQ(read.font_weight, FontWeight::bold); + EXPECT_EQ(read.font_style, FontStyle::italic); + EXPECT_EQ(read.font_underline, true); + EXPECT_EQ(read.font_line_through, true); + ASSERT_TRUE(read.font_color.has_value()); + EXPECT_EQ(read.font_color->rgb(), 0xcc0000u); + EXPECT_EQ(read.font_size, Measure("16pt")); +} + +TEST(OoxmlSpreadsheetStyleWrite, a_format_shared_with_another_cell_is_copied) { + const Document document = + decode(workbook(two_red, "", "", "", "", red_styles)); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, {}, bold()); + + EXPECT_EQ(text_style_at(sheet, 0).font_weight, FontWeight::bold); + EXPECT_EQ(fill_at(sheet, 0, 0), 0xff0000u); + EXPECT_NE(text_style_at(sheet, 1).font_weight, FontWeight::bold); + EXPECT_EQ(fill_at(sheet, 1, 0), 0xff0000u); +} + +TEST(OoxmlSpreadsheetStyleWrite, one_delta_on_one_base_is_one_format) { + const Document document = + decode(workbook(two_red, "", "", "", "", red_styles)); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, fill(0x00ff00_rgb), bold()); + sheet.set_cell_style(1, 0, fill(0x00ff00_rgb), bold()); + + const std::string styles = styles_of(document); + EXPECT_EQ(count(styles, ")"), std::string::npos); + EXPECT_EQ(count(styles, ""), 2); + EXPECT_EQ(count(styles, ""), 4); +} + +TEST(OoxmlSpreadsheetStyleWrite, a_fill_taken_away_is_no_pattern) { + const Document document = + decode(workbook(two_red, "", "", "", "", red_styles)); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, fill(Color(0, 0, 0, 0)), {}); + + EXPECT_EQ(fill_at(sheet, 0, 0), std::nullopt); + EXPECT_EQ(fill_at(sheet, 1, 0), 0xff0000u); +} + +TEST(OoxmlSpreadsheetStyleWrite, an_alignment_lands_in_the_format) { + const Document document = decode(workbook(one_string)); + TableCellStyle right; + right.horizontal_align = HorizontalAlign::right; + + first_sheet(document).set_cell_style(0, 0, right, {}); + + EXPECT_EQ(first_sheet(reopened(document)).cell_style(0, 0).horizontal_align, + HorizontalAlign::right); +} + +TEST(OoxmlSpreadsheetStyleWrite, a_cell_the_file_does_not_state_is_made) { + const Document document = decode(workbook(one_string)); + + first_sheet(document).set_cell_style(2, 3, fill(0x0000ff_rgb), {}); + + EXPECT_EQ(fill_at(first_sheet(reopened(document)), 2, 3), 0x0000ffu); +} + +TEST(OoxmlSpreadsheetStyleWrite, a_cell_without_a_format_starts_from_its_row) { + const Document document = decode( + workbook(R"()" + R"(a)", + "", "", "", "", red_styles)); + + first_sheet(document).set_cell_style(0, 0, {}, bold()); + + const Document saved = reopened(document); + const Sheet sheet = first_sheet(saved); + EXPECT_EQ(fill_at(sheet, 0, 0), 0xff0000u); + EXPECT_EQ(text_style_at(sheet, 0).font_weight, FontWeight::bold); +} + +TEST(OoxmlSpreadsheetStyleWrite, a_covered_cell_refuses) { + const Document document = decode(workbook( + R"(a)", + R"()")); + + EXPECT_THROW( + first_sheet(document).set_cell_style(1, 0, fill(0xffff00_rgb), {}), + UnsupportedOperation); +} + +TEST(OoxmlSpreadsheetStyleWrite, the_op_carries_the_fill_and_the_text_keys) { + const Document document = decode(workbook(one_string)); + + document.edit( + R"({"version": 2, "ops": [{"op": "setCellStyle", "sheet": 0,)" + R"( "column": 0, "row": 0,)" + R"( "style": {"fill": "#ffff00", "bold": true, "align": "center"}}]})"); + + const Sheet sheet = first_sheet(document); + EXPECT_EQ(fill_at(sheet, 0, 0), 0xffff00u); + EXPECT_EQ(text_style_at(sheet, 0).font_weight, FontWeight::bold); + EXPECT_EQ(sheet.cell_style(0, 0).horizontal_align, HorizontalAlign::center); +} diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp index 9c7f309e3..8171ab151 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_test_util.hpp @@ -27,12 +27,13 @@ inline void insert(internal::zip::ZipArchive &zip, const std::string &path, /// @p sheet_data and which carries @p sheet_extra - ``, say - /// after it and @p sheet_prefix - `` - before it. /// @p shared_strings writes a `sharedStrings.xml` where it is given, and -/// @p workbook_extra follows `` in `workbook.xml`. +/// @p workbook_extra follows `` in `workbook.xml`, and @p styles is +/// what `styles.xml` holds. inline std::shared_ptr workbook(const std::string &sheet_data, const std::string &sheet_extra = "", const std::string &shared_strings = "", const std::string &workbook_extra = "", - const std::string &sheet_prefix = "") { + const std::string &sheet_prefix = "", const std::string &styles = "") { internal::zip::ZipArchive zip; insert( zip, "[Content_Types].xml", @@ -58,7 +59,8 @@ workbook(const std::string &sheet_data, const std::string &sheet_extra = "", R"()"); insert( zip, "xl/styles.xml", - R"()"); + R"()" + + styles + R"()"); insert( zip, "xl/worksheets/sheet1.xml", R"()" + From d2963627b7dcfe4605a8cd2a7631bf65b1b32104 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 21:08:10 +0200 Subject: [PATCH 2/3] fix(ooxml): an xlsx column range starts at its min The registry looked up a column by the first range that ends at or after it, and it did not check where that range starts. So a column in a gap between two `` ranges took the width and the style of the next one. The registry now keeps the first column of each range, and the style writer asks the registry and does not walk `` again. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Tigcf9he3w1aHfNPJoEGsd --- CHANGELOG.md | 3 +++ .../spreadsheet/ooxml_spreadsheet_document.cpp | 12 ++++-------- .../ooxml_spreadsheet_element_registry.cpp | 12 +++++++----- .../ooxml_spreadsheet_element_registry.hpp | 1 + .../ooxml/ooxml_spreadsheet_style_write_test.cpp | 14 ++++++++++++++ 5 files changed, 29 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ced8986..f8a148af1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ The release run heads these entries with the version and opens a fresh the merge, and one written into an empty cell right of a merge failed. The first now refuses, and the second lands in its cell. +- **Fix**: an xlsx column in a gap between `` ranges took the width and + the style of the next range. It now has neither. + - **Fix**: many xlsx fills showed a wrong colour. A cell now shows its fill, theme colours and tints, italic, underline, strikethrough and every alignment. diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp index dec8bc3fa..99801113a 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_document.cpp @@ -379,14 +379,10 @@ class ElementAdapter final : public AdapterBase { row_node.attribute("customFormat").as_bool()) { base = row_node.attribute("s").as_uint(); } else { - for (const pugi::xml_node col : - get_node(element_id).child("cols").children("col")) { - if (col.attribute("min").as_uint() <= column + 1 && - column + 1 <= col.attribute("max").as_uint()) { - base = col.attribute("style").as_uint(); - break; - } - } + base = m_registry->sheet_element_at(element_id) + .column_node(column) + .attribute("style") + .as_uint(); } const std::uint32_t format = 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 d639ed2ef..b55a335e7 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.cpp @@ -71,10 +71,10 @@ void ElementRegistry::append_sheet_cell(const ElementIdentifier sheet_id, element_at(cell_id).parent_id = sheet_id; } -void ElementRegistry::Sheet::register_column( - [[maybe_unused]] const std::uint32_t column_min, - const std::uint32_t column_max, const pugi::xml_node element) { - columns[column_max] = {.node = element}; +void ElementRegistry::Sheet::register_column(const std::uint32_t column_min, + const std::uint32_t column_max, + const pugi::xml_node element) { + columns[column_max] = {.min = column_min, .node = element}; } void ElementRegistry::Sheet::register_row(const std::uint32_t row, @@ -93,8 +93,10 @@ void ElementRegistry::Sheet::register_cell(const std::uint32_t column, const ElementRegistry::Sheet::Column * ElementRegistry::Sheet::column(const std::uint32_t column) const { + // the ranges leave gaps, so the one ending at or after `column` may start + // past it if (const auto it = util::map::lookup_greater_or_equals(columns, column); - it != std::end(columns)) { + it != std::end(columns) && it->second.min <= column) { return &it->second; } return nullptr; 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 5833d5a2b..164f9c1b4 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.hpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_element_registry.hpp @@ -37,6 +37,7 @@ class ElementRegistry final struct Sheet final { struct Column final { + std::uint32_t min{0}; pugi::xml_node node; }; diff --git a/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp b/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp index 7ebb80134..d3b00a041 100644 --- a/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp +++ b/test/src/internal/ooxml/ooxml_spreadsheet_style_write_test.cpp @@ -200,6 +200,20 @@ TEST(OoxmlSpreadsheetStyleWrite, a_cell_without_a_format_starts_from_its_row) { EXPECT_EQ(text_style_at(sheet, 0).font_weight, FontWeight::bold); } +TEST(OoxmlSpreadsheetStyleWrite, a_cell_starts_from_its_own_column_only) { + const Document document = decode( + workbook(one_string, "", "", "", + R"()", red_styles)); + + first_sheet(document).set_cell_style(0, 0, {}, bold()); + first_sheet(document).set_cell_style(2, 0, {}, bold()); + + const Document saved = reopened(document); + const Sheet sheet = first_sheet(saved); + EXPECT_EQ(fill_at(sheet, 0, 0), std::nullopt); + EXPECT_EQ(fill_at(sheet, 2, 0), 0xff0000u); +} + TEST(OoxmlSpreadsheetStyleWrite, a_covered_cell_refuses) { const Document document = decode(workbook( R"(a)", From 197bec9f6e9c00971b9f9cca27b3e1b9f08fb9b7 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 21:08:10 +0200 Subject: [PATCH 3/3] refactor: share set_attribute, and reuse the ooxml sequence helpers The odf and the xlsx style writers each had a copy of `set_attribute`, so `xml::set_attribute` now serves both. The xlsx writer inserts a child through `insert_in_sequence` and spells a colour through `hex_color`, which `ooxml_util` already has. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Tigcf9he3w1aHfNPJoEGsd --- src/odr/internal/odf/odf_style.cpp | 28 ++++------ .../spreadsheet/ooxml_spreadsheet_style.cpp | 54 +++++++------------ src/odr/internal/xml/xml_util.cpp | 9 ++++ src/odr/internal/xml/xml_util.hpp | 4 ++ 4 files changed, 42 insertions(+), 53 deletions(-) diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index b9e3b62c9..272174506 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -749,19 +750,11 @@ std::string color_value(const Color &color) { return fmt::format("#{:06x}", color.rgb()); } -void set_attribute(pugi::xml_node node, const char *name, const char *value) { - pugi::xml_attribute attribute = node.attribute(name); - if (!attribute) { - attribute = node.append_attribute(name); - } - attribute.set_value(value); -} - /// Writes the set fields of @p style as attributes of @p properties, the /// asian and complex variants beside each western one. void write_text_properties(pugi::xml_node properties, const TextStyle &style) { const auto set = [&](const char *name, const std::string &value) { - set_attribute(properties, name, value.c_str()); + xml::set_attribute(properties, name, value.c_str()); }; if (style.font_size.has_value()) { @@ -874,7 +867,7 @@ std::string StyleRegistry::create_cell_style(pugi::xml_node automatic_styles, if (base_node && std::strcmp(base_node.parent().name(), "office:automatic-styles") == 0) { node = automatic_styles.append_copy(base_node); - set_attribute(node, "style:name", name.c_str()); + xml::set_attribute(node, "style:name", name.c_str()); } else { node = automatic_styles.append_child("style:style"); node.append_attribute("style:name").set_value(name.c_str()); @@ -885,15 +878,16 @@ std::string StyleRegistry::create_cell_style(pugi::xml_node automatic_styles, } if (cell.background_color.has_value()) { - set_attribute(properties_of(node, "style:table-cell-properties"), - "fo:background-color", - color_value(*cell.background_color).c_str()); + xml::set_attribute(properties_of(node, "style:table-cell-properties"), + "fo:background-color", + color_value(*cell.background_color).c_str()); } if (cell.horizontal_align.has_value()) { - set_attribute(properties_of(node, "style:table-cell-properties"), - "style:text-align-source", "fix"); - set_attribute(properties_of(node, "style:paragraph-properties"), - "fo:text-align", text_align_value(*cell.horizontal_align)); + xml::set_attribute(properties_of(node, "style:table-cell-properties"), + "style:text-align-source", "fix"); + xml::set_attribute(properties_of(node, "style:paragraph-properties"), + "fo:text-align", + text_align_value(*cell.horizontal_align)); } TextStyle text_properties = text; text_properties.background_color.reset(); diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp index e08f377a7..8c6de19c5 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp @@ -2,11 +2,10 @@ #include #include +#include -#include #include #include -#include #include #include #include @@ -82,15 +81,7 @@ pugi::xml_node ordered_child(pugi::xml_node parent, const char *name, if (const pugi::xml_node existing = parent.child(name)) { return existing; } - const auto rank = [&](const std::string_view child) { - return std::ranges::find(order, child) - std::begin(order); - }; - for (const pugi::xml_node child : parent.children()) { - if (rank(child.name()) > rank(name)) { - return parent.insert_child_before(name, child); - } - } - return parent.append_child(name); + return insert_in_sequence(parent, name, order); } /// The @p name child of `styleSheet`, made in the order [ECMA-376] 18.8.39 @@ -103,17 +94,7 @@ pugi::xml_node collection_of(pugi::xml_node root, const char *name) { return ordered_child(root, name, order); } -void set_attribute(pugi::xml_node node, const char *name, const char *value) { - pugi::xml_attribute attribute = node.attribute(name); - if (!attribute) { - attribute = node.append_attribute(name); - } - attribute.set_value(value); -} - -std::string argb_of(const Color &color) { - return fmt::format("FF{:06X}", color.rgb()); -} +std::string argb_of(const Color &color) { return "FF" + hex_color(color); } /// [ECMA-376] 18.8.22 `CT_Font` is a sequence. constexpr std::array font_order{ @@ -147,7 +128,7 @@ std::uint32_t intern(pugi::xml_node collection, const pugi::xml_node node) { ++index; } collection.append_copy(node); - set_attribute(collection, "count", std::to_string(index + 1).c_str()); + xml::set_attribute(collection, "count", std::to_string(index + 1).c_str()); return index; } @@ -158,7 +139,7 @@ void state_defaults(pugi::xml_node root) { pugi::xml_node font = fonts.append_child("font"); font.append_child("sz").append_attribute("val").set_value("11"); font.append_child("name").append_attribute("val").set_value("Calibri"); - set_attribute(fonts, "count", "1"); + xml::set_attribute(fonts, "count", "1"); } if (pugi::xml_node fills = collection_of(root, "fills"); !fills.first_child()) { @@ -171,12 +152,12 @@ void state_defaults(pugi::xml_node root) { .append_child("patternFill") .append_attribute("patternType") .set_value("gray125"); - set_attribute(fills, "count", "2"); + xml::set_attribute(fills, "count", "2"); } if (pugi::xml_node borders = collection_of(root, "borders"); !borders.first_child()) { borders.append_child("border"); - set_attribute(borders, "count", "1"); + xml::set_attribute(borders, "count", "1"); } const auto default_xf = [](pugi::xml_node xf) { xf.append_attribute("numFmtId").set_value("0"); @@ -188,14 +169,14 @@ void state_defaults(pugi::xml_node root) { if (pugi::xml_node masters = collection_of(root, "cellStyleXfs"); !masters.first_child()) { default_xf(masters.append_child("xf")); - set_attribute(masters, "count", "1"); + xml::set_attribute(masters, "count", "1"); } if (pugi::xml_node formats = collection_of(root, "cellXfs"); !formats.first_child()) { default_xf(formats.append_child("xf")) .append_attribute("xfId") .set_value("0"); - set_attribute(formats, "count", "1"); + xml::set_attribute(formats, "count", "1"); } } @@ -388,8 +369,9 @@ StyleRegistry::create_cell_format(const std::uint32_t base, } } if (text_style.font_size) { - set_attribute(ordered_child(font, "sz", font_order), "val", - fmt::format("{:g}", points(*text_style.font_size)).c_str()); + xml::set_attribute( + ordered_child(font, "sz", font_order), "val", + fmt::format("{:g}", points(*text_style.font_size)).c_str()); } if (text_style.font_color) { pugi::xml_node color = ordered_child(font, "color", font_order); @@ -397,10 +379,10 @@ StyleRegistry::create_cell_format(const std::uint32_t base, color.append_attribute("rgb").set_value( argb_of(*text_style.font_color).c_str()); } - set_attribute( + xml::set_attribute( xf, "fontId", std::to_string(intern(m_styles_root.child("fonts"), font)).c_str()); - set_attribute(xf, "applyFont", "1"); + xml::set_attribute(xf, "applyFont", "1"); } if (cell_style.background_color) { @@ -415,10 +397,10 @@ StyleRegistry::create_cell_format(const std::uint32_t base, pattern.append_child("bgColor").append_attribute("indexed").set_value( "64"); } - set_attribute( + xml::set_attribute( xf, "fillId", std::to_string(intern(m_styles_root.child("fills"), fill)).c_str()); - set_attribute(xf, "applyFill", "1"); + xml::set_attribute(xf, "applyFill", "1"); } if (cell_style.horizontal_align) { @@ -432,8 +414,8 @@ StyleRegistry::create_cell_format(const std::uint32_t base, } else if (*cell_style.horizontal_align == HorizontalAlign::right) { horizontal = "right"; } - set_attribute(alignment, "horizontal", horizontal); - set_attribute(xf, "applyAlignment", "1"); + xml::set_attribute(alignment, "horizontal", horizontal); + xml::set_attribute(xf, "applyAlignment", "1"); } const std::uint32_t result = intern(m_styles_root.child("cellXfs"), xf); diff --git a/src/odr/internal/xml/xml_util.cpp b/src/odr/internal/xml/xml_util.cpp index caf471864..434c5512f 100644 --- a/src/odr/internal/xml/xml_util.cpp +++ b/src/odr/internal/xml/xml_util.cpp @@ -89,6 +89,15 @@ pugi::xml_document xml::parse(std::istream &in) { void xml::check_xml_file(std::istream &in) { std::ignore = parse(in); } +void xml::set_attribute(pugi::xml_node node, const char *name, + const char *value) { + pugi::xml_attribute attribute = node.attribute(name); + if (!attribute) { + attribute = node.append_attribute(name); + } + attribute.set_value(value); +} + std::string xml::read_declared_encoding(std::istream &in) { static constexpr std::size_t probe_size = 1024; static constexpr std::string_view space = " \t\r\n"; diff --git a/src/odr/internal/xml/xml_util.hpp b/src/odr/internal/xml/xml_util.hpp index 9ad1d9adc..7667a507c 100644 --- a/src/odr/internal/xml/xml_util.hpp +++ b/src/odr/internal/xml/xml_util.hpp @@ -7,6 +7,7 @@ namespace pugi { class xml_document; +class xml_node; } // namespace pugi namespace odr::internal::abstract { @@ -33,6 +34,9 @@ pugi::xml_document parse(std::istream &); pugi::xml_document parse(const abstract::File &); pugi::xml_document parse(const abstract::ReadableFilesystem &, const AbsPath &); +/// Sets the attribute @p name of @p node, appending it where it is missing. +void set_attribute(pugi::xml_node node, const char *name, const char *value); + /// Throws unless @p in holds a well formed xml document. void check_xml_file(std::istream &in);