From 88b82820950c4fcfbc90543cd4ee0b97bbfc45f5 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 20:04:18 +0200 Subject: [PATCH 1/2] feat(document): style a cell of an ods file Sheet::set_cell_style and the setCellStyle op write the fill, the horizontal alignment, and bold, italic, underline, strikethrough, colour and size onto one cell. The op takes the keys of setTextStyle, and fill and align in place of highlight. The ods writer claims the cell as a value write does, and points table:style-name at a fresh automatic ce style. That style copies the automatic style the cell shows (its own, else the row or column default) with the delta, or inherits from a named one. One base and one delta make one style, so a range of cells adds one. A covered position now refuses a value and a style. The index holds no covered cell, so is_covered walks the row. claim_cell took the start of a run from the end of the cell before it, which is wrong after a span; it now takes it from the run itself. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01NXnz6EZY8YpyiyyE1GpsWy --- CHANGELOG.md | 8 + docs/design/spreadsheet-editing.md | 8 +- src/odr/document.cpp | 42 +++ src/odr/document_element.cpp | 19 ++ src/odr/document_element.hpp | 7 + src/odr/internal/abstract/document.hpp | 10 + src/odr/internal/odf/AGENTS.md | 9 + src/odr/internal/odf/odf_document.cpp | 95 +++++- src/odr/internal/odf/odf_style.cpp | 120 ++++++++ src/odr/internal/odf/odf_style.hpp | 8 + test/CMakeLists.txt | 1 + .../src/internal/odf/odf_sheet_style_test.cpp | 280 ++++++++++++++++++ 12 files changed, 591 insertions(+), 16 deletions(-) create mode 100644 test/src/internal/odf/odf_sheet_style_test.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 025da7e78..9458df38b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,14 @@ 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, + italic, underline, strikethrough, colour and size. + +- **Fix**: in an ods file, a value written into a covered cell landed outside + 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**: 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/docs/design/spreadsheet-editing.md b/docs/design/spreadsheet-editing.md index defa628a4..eebac7a6c 100644 --- a/docs/design/spreadsheet-editing.md +++ b/docs/design/spreadsheet-editing.md @@ -195,7 +195,7 @@ separate scripts. The coordinates are the ones an op names, never a DOM index. ## Cell formatting -Status: planned. The steps land as a stack, in this order: +Status: `.ods` writes 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 @@ -219,7 +219,7 @@ Status: planned. The steps land as a stack, in this order: | `bold`, `italic`, `underline`, `strikethrough` | a bool | `style:text-properties`, as for a run | `b`, `i`, `u`, `strike` in a `font` | | `color` | `#rrggbb` | `fo:color` | `font/color/@rgb` | | `size` | a length | `fo:font-size` | `font/sz`, in points | -| `align` | `left`, `center`, `right` or null | `fo:text-align` in `style:paragraph-properties` and `style:text-align-source="fix"`; null is `value-type` | `alignment/@horizontal`; null is `general` | +| `align` | `left`, `center` or `right` | `fo:text-align` in `style:paragraph-properties` and `style:text-align-source="fix"` | `alignment/@horizontal` | The text keys are the ones of `setTextStyle`, and off is written, never removed (`document-editing.md` decision 9). A cell has no `highlight`: `fill` is the @@ -239,7 +239,7 @@ An `.ods` cell style and an `.xlsx` `xf` are shared by every cell naming them. `style:family="table-cell"` automatic style that copies the one the cell shows (its own, else the row's or the column's default) with the delta applied, and points `table:style-name` at it. A formula cell and a rich cell - take a style: nothing in their content changes. + take a style: nothing in their content changes. A covered cell refuses. - `.xlsx`: the writer appends a `font`, a `fill` and an `xf`, each only where no equal one exists, with `applyFont`, `applyFill` and `applyAlignment` set, sets `c/@s`, and saves `styles.xml`. A cell the file does not state is made @@ -294,6 +294,8 @@ cell's inline style before the gesture, and one gesture is one undo step. their own kinds. This also fixes `.xlsx` serials on the read side. - Multi-line cells, insert and delete of rows and columns. - A style for a whole row or column, so cells past the rendered extent take it. +- `align` back to the alignment by value type: `TableCellStyle` has no value + that says it. - `.csv` save. - Sheets past `spreadsheet_limit` or `spreadsheet_cell_limit` are not in the page; the mode should say so where a view reports a `sheet_cut`. diff --git a/src/odr/document.cpp b/src/odr/document.cpp index e85c1e539..c3ec462ba 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -172,6 +172,38 @@ TextStyle parse_text_style(const nlohmann::json &json) { return style; } +/// The `style` of a `setCellStyle` op: the keys of `setTextStyle` but +/// `highlight`, and `fill` and `align`. +std::pair +parse_cell_style(const nlohmann::json &json) { + TableCellStyle cell_style; + nlohmann::json text_keys = nlohmann::json::object(); + for (const auto &[key, value] : json.items()) { + if (key == "fill") { + cell_style.background_color = value.is_null() + ? Color(0, 0, 0, 0) + : parse_color(value.get()); + } else if (key == "align") { + const auto align = value.get(); + if (align == "left") { + cell_style.horizontal_align = HorizontalAlign::left; + } else if (align == "center") { + cell_style.horizontal_align = HorizontalAlign::center; + } else if (align == "right") { + cell_style.horizontal_align = HorizontalAlign::right; + } else { + throw std::invalid_argument("unknown alignment " + align); + } + } else if (key == "highlight") { + throw std::invalid_argument( + "a cell has no highlight; `fill` is its ground"); + } else { + text_keys[key] = value; + } + } + return {cell_style, parse_text_style(text_keys)}; +} + /// The @p ordinal -th sheet in document order, which is how an op names one. Sheet sheet_at(const Element root, const std::uint32_t ordinal) { std::uint32_t seen = 0; @@ -270,6 +302,16 @@ void Document::edit(const std::string_view operations, continue; } + if (name == "setCellStyle") { + const auto [cell_style, text_style] = + parse_cell_style(operation.at("style")); + sheet_at(root_element(), operation.at("sheet").get()) + .set_cell_style(operation.at("column").get(), + operation.at("row").get(), cell_style, + text_style); + continue; + } + if (name == "setText") { text_of(operation, "id") .set_content(operation.at("text").get()); diff --git a/src/odr/document_element.cpp b/src/odr/document_element.cpp index 32c3c68bb..c700dc9d8 100644 --- a/src/odr/document_element.cpp +++ b/src/odr/document_element.cpp @@ -425,6 +425,25 @@ void Sheet::clear_cell(const std::uint32_t column, set_cell(column, row, CellValue()); } +void Sheet::set_cell_style(const std::uint32_t column, const std::uint32_t row, + const TableCellStyle &cell_style, + const TextStyle &text_style) const { + if (!exists_()) { + return; + } + const auto stated = [](const auto &sides) { + return sides.right || sides.top || sides.left || sides.bottom; + }; + if (cell_style.vertical_align || stated(cell_style.padding) || + stated(cell_style.border) || cell_style.text_rotation || + cell_style.wrap_text || text_style.font_name || text_style.font_shadow || + text_style.background_color || text_style.font_position) { + throw UnsupportedOperation(); + } + m_adapter2->sheet_set_cell_style(m_identifier, column, row, cell_style, + text_style); +} + TableStyle Sheet::style() const { return exists_() ? m_adapter2->sheet_style(m_identifier) : TableStyle(); } diff --git a/src/odr/document_element.hpp b/src/odr/document_element.hpp index e38dbd356..987eaa3d7 100644 --- a/src/odr/document_element.hpp +++ b/src/odr/document_element.hpp @@ -388,6 +388,13 @@ class Sheet final : public ElementBase { /// Takes the cell's value away, keeping the style it carries. Not the same /// as writing an empty string. void clear_cell(std::uint32_t column, std::uint32_t row) const; + /// States the set fields of both styles on the cell and leaves the rest; a + /// fill of alpha 0 takes the fill away. + /// @throws UnsupportedOperation where the cell cannot be styled, or a style + /// sets a field no engine writes. + void set_cell_style(std::uint32_t column, std::uint32_t row, + const TableCellStyle &cell_style, + const TextStyle &text_style) const; [[nodiscard]] TableStyle style() const; [[nodiscard]] TableColumnStyle column_style(std::uint32_t column) const; diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index a6a750ab0..7932fc163 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -291,6 +291,16 @@ class SheetAdapter { virtual void sheet_set_cell(ElementIdentifier element_id, std::uint32_t column, std::uint32_t row, const CellValue &value) const = 0; + /// States the set fields of @p cell_style and @p text_style on the cell at + /// (@p column, @p row) and leaves the rest. The cell's content is untouched. + virtual void + sheet_set_cell_style([[maybe_unused]] ElementIdentifier element_id, + [[maybe_unused]] std::uint32_t column, + [[maybe_unused]] std::uint32_t row, + [[maybe_unused]] const TableCellStyle &cell_style, + [[maybe_unused]] const TextStyle &text_style) const { + throw UnsupportedOperation(); + } [[nodiscard]] virtual TableStyle sheet_style(ElementIdentifier element_id) const = 0; diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 1c225fdc8..6f7ec48d5 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -141,6 +141,15 @@ unknown mimetype are tolerated. the same walk `spreadsheet.js::runOf` makes over the page. A cell that holds a formula, a link, a line break or several paragraphs refuses. Refusals are decided before any node is cut. +- **A cell style.** `sheet_set_cell_style` claims the cell as a value write + does and points `table:style-name` at a fresh automatic style `ce` + (`StyleRegistry::create_cell_style`): a copy of the automatic style the + cell shows (its own, else the row's or column's default) plus the delta, or + a child of a named one. One base and one delta make one style. The fill is + `fo:background-color`, the alignment `fo:text-align` with + `style:text-align-source="fix"`, and the text keys are `text_set_style`'s. +- **A covered position** refuses a value and a style. The index holds no + covered cell, so `is_covered` walks the row's DOM. - **A repeated cell** is written by cutting the run: `claim_cell` copies the `table:table-row` and the `table:table-cell` around the position and leaves the original node as the one written, so its element survives. diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 2fb230a89..d40a771fc 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -494,6 +494,9 @@ class ElementAdapter final : public AdapterBase { if (!writable(value)) { throw UnsupportedOperation(); } + if (is_covered(element_id, column, row)) { + throw UnsupportedOperation(); + } const ElementRegistry::Sheet::Cell *cell = m_registry->sheet_element_at(element_id).cell(column, row); @@ -550,6 +553,40 @@ class ElementAdapter final : public AdapterBase { drop_stale_results(element_id, column, row); } + 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::Cell *cell = + m_registry->sheet_element_at(element_id).cell(column, row); + + if (is_covered(element_id, column, row)) { + throw UnsupportedOperation(); + } + + ElementIdentifier cell_id = null_element_id; + if (cell == nullptr) { + cell_id = grow_to_cell(element_id, column, row); + } else { + cell_id = cell->element_id; + if (cell_id == null_element_id || + m_registry->sheet_cell_element_at(cell_id).is_repeated) { + cell_id = claim_cell(element_id, column, row); // `cell` is stale after + } + } + + pugi::xml_node node = get_node(cell_id); + const std::string name = m_document->style_registry().create_cell_style( + automatic_styles_of(node), + cell_style_name(element_id, cell_id, {column, row}), cell_style, + text_style); + pugi::xml_attribute attribute = node.attribute("table:style-name"); + if (!attribute) { + attribute = node.prepend_attribute("table:style-name"); + } + attribute.set_value(name.c_str()); + } + /// The sheets of the document in the order an operation names them by. [[nodiscard]] std::vector sheets_() const { std::vector result; @@ -1156,6 +1193,30 @@ class ElementAdapter final : public AdapterBase { return row_entry->node; } + /// Whether a span covers (@p column, @p row). The index holds no covered + /// cell, so the row's dom is asked ([ODF 1.2] 9.1.5). + [[nodiscard]] bool is_covered(const ElementIdentifier sheet_id, + const std::uint32_t column, + const std::uint32_t row) const { + const ElementRegistry::Sheet::Row *row_entry = + m_registry->sheet_element_at(sheet_id).row(row); + if (row_entry == nullptr) { + return false; + } + std::uint32_t begin = 0; + for (const pugi::xml_node cell : row_entry->node.children()) { + const std::string_view name = cell.name(); + if (name != "table:table-cell" && name != "table:covered-table-cell") { + continue; + } + begin += cell.attribute("table:number-columns-repeated").as_uint(1); + if (column < begin) { + return name == "table:covered-table-cell"; + } + } + return false; + } + /// Gives (@p column, @p row) an element of its own: cuts the row and the /// cell run it is one position of, and states the `text:p` an empty cell /// has none of. Reindexes: every pointer read before is stale. @@ -1165,13 +1226,12 @@ class ElementAdapter final : public AdapterBase { const ElementRegistry::Sheet &sheet = m_registry->sheet_element_at(sheet_id); - const std::span cells = - sheet.row_cells(*sheet.row(row)); const ElementRegistry::Sheet::Cell *cell_entry = sheet.cell(column, row); - const std::size_t cell_index = cell_entry - cells.data(); - const std::uint32_t cell_begin = - cell_index == 0 ? 0 : cells[cell_index - 1].end; pugi::xml_node cell_node = cell_entry->node; + // not the end of the cell before: a span leaves a gap after that one + const std::uint32_t cell_begin = + cell_entry->end - + cell_node.attribute("table:number-columns-repeated").as_uint(1); // the row first: the cell keeps its node, so its own run is unmoved split_row_at(sheet, row); @@ -1562,6 +1622,22 @@ class ElementAdapter final : public AdapterBase { get_partial_cell_style(const ElementIdentifier sheet_id, const ElementIdentifier cell_id, const TablePosition &position) const { + if (const char *style_name = cell_style_name(sheet_id, cell_id, position); + style_name != nullptr) { + if (const Style *style = m_document->style_registry().style(style_name); + style != nullptr) { + return style->resolved(); + } + } + return {}; + } + + /// The style the cell shows: its own, else its row's or its column's + /// default. Null where none of them names one. + [[nodiscard]] const char * + cell_style_name(const ElementIdentifier sheet_id, + const ElementIdentifier cell_id, + const TablePosition &position) const { const char *style_name = nullptr; if (cell_id != null_element_id) { @@ -1602,14 +1678,7 @@ class ElementAdapter final : public AdapterBase { } } - if (style_name != nullptr) { - if (const Style *style = m_document->style_registry().style(style_name); - style != nullptr) { - return style->resolved(); - } - } - - return {}; + return style_name; } }; diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index cb584995f..8b437f3f3 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -3,10 +3,13 @@ #include #include +#include +#include #include #include #include #include +#include #include #include #include @@ -792,8 +795,125 @@ void write_text_properties(pugi::xml_node properties, const TextStyle &style) { } } +/// The properties child @p name of @p style, made in the order [ODF 1.2] +/// 16.2 gives them where it is missing. +pugi::xml_node properties_of(pugi::xml_node style, const char *name) { + static constexpr std::array order{ + "style:table-cell-properties", "style:paragraph-properties", + "style:text-properties"}; + if (pugi::xml_node existing = style.child(name)) { + return existing; + } + const auto rank = [](const std::string_view child) { + return std::ranges::find(order, child) - std::begin(order); + }; + for (pugi::xml_node child : style.children()) { + if (rank(child.name()) > rank(name)) { + return style.insert_child_before(name, child); + } + } + return style.append_child(name); +} + +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); +} + +const char *text_align_value(const HorizontalAlign align) { + switch (align) { + case HorizontalAlign::left: + return "left"; + case HorizontalAlign::center: + return "center"; + case HorizontalAlign::right: + return "right"; + } + return "left"; +} + } // namespace +std::string StyleRegistry::create_cell_style(pugi::xml_node automatic_styles, + const char *base_name, + const TableCellStyle &cell, + const TextStyle &text) { + const std::string base = base_name != nullptr ? base_name : ""; + const auto optional = [](const auto &value) { + return value.has_value() ? std::to_string(static_cast(*value)) + : std::string("-"); + }; + const std::string key = fmt::format( + "{}|{}|{}|{}|{}|{}|{}|{}|{}", base, + cell.background_color + ? fmt::format("{:08x}", cell.background_color->argb()) + : "-", + optional(cell.horizontal_align), optional(text.font_weight), + optional(text.font_style), optional(text.font_underline), + optional(text.font_line_through), + text.font_color ? fmt::format("{:08x}", text.font_color->argb()) : "-", + text.font_size ? text.font_size->to_string() : "-"); + if (const auto it = m_created_cell_styles.find(key); + it != std::end(m_created_cell_styles)) { + return it->second; + } + + std::string name; + for (;; ++m_next_cell_style) { + name = "ce" + std::to_string(m_next_cell_style); + if (!m_index_style.contains(name)) { + break; + } + } + + const auto base_it = m_index_style.find(base); + const pugi::xml_node base_node = + base_it != std::end(m_index_style) ? base_it->second : pugi::xml_node(); + pugi::xml_node node; + // an automatic style may be shared, so it is copied; a named one is + // inherited from + 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()); + } else { + node = automatic_styles.append_child("style:style"); + node.append_attribute("style:name").set_value(name.c_str()); + node.append_attribute("style:family").set_value("table-cell"); + if (!base.empty()) { + node.append_attribute("style:parent-style-name").set_value(base.c_str()); + } + } + + 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()); + } + 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)); + } + TextStyle text_properties = text; + text_properties.background_color.reset(); + if (text_properties.font_size || text_properties.font_weight || + text_properties.font_style || text_properties.font_underline || + text_properties.font_line_through || text_properties.font_color) { + write_text_properties(properties_of(node, "style:text-properties"), + text_properties); + } + + m_index_style[name] = node; + generate_style_(name, node); + m_created_cell_styles.emplace(key, name); + return name; +} + std::string StyleRegistry::create_text_style(pugi::xml_node automatic_styles, const char *base_name, const TextStyle &style) { diff --git a/src/odr/internal/odf/odf_style.hpp b/src/odr/internal/odf/odf_style.hpp index d4e51328e..88b3e6065 100644 --- a/src/odr/internal/odf/odf_style.hpp +++ b/src/odr/internal/odf/odf_style.hpp @@ -88,10 +88,18 @@ class StyleRegistry final { /// child of a named one, the delta alone where it is null. std::string create_text_style(pugi::xml_node automatic_styles, const char *base_name, const TextStyle &style); + /// A table-cell style carrying @p cell and @p text, made as a text style is. + /// Two calls with one base and one delta answer one style. + std::string create_cell_style(pugi::xml_node automatic_styles, + const char *base_name, + const TableCellStyle &cell, + const TextStyle &text); private: /// Where the search for a free `T` name starts. std::uint32_t m_next_text_style{1}; + std::uint32_t m_next_cell_style{1}; + std::unordered_map m_created_cell_styles; std::unordered_map m_index_font_face; std::unordered_map m_index_default_style; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index c01fcc1cb..f2c243510 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -88,6 +88,7 @@ add_executable(odr_test "src/internal/odf/odf_sheet_repeat_test.cpp" "src/internal/odf/odf_sheet_stale_test.cpp" "src/internal/odf/odf_sheet_value_test.cpp" + "src/internal/odf/odf_sheet_style_test.cpp" "src/internal/odf/odf_sheet_write_test.cpp" "src/internal/odf/odf_table_test.cpp" diff --git a/test/src/internal/odf/odf_sheet_style_test.cpp b/test/src/internal/odf/odf_sheet_style_test.cpp new file mode 100644 index 000000000..d2849ca0a --- /dev/null +++ b/test/src/internal/odf/odf_sheet_style_test.cpp @@ -0,0 +1,280 @@ +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +#include + +#include +#include +#include +#include +#include +#include + +using namespace odr; +using namespace odr::internal; + +namespace { + +/// A flat sheet whose single row holds @p cells, under @p automatic_styles +/// and @p styles. +std::string flat_sheet(const std::string &cells, + const std::string &automatic_styles = "", + const std::string &styles = "") { + return R"()" + R"()" + R"()" + + styles + R"()" + + automatic_styles + + R"()" + R"()" + + cells + + R"()" + R"()"; +} + +std::string string_cell(const std::string &text, + const std::string &style_name = "") { + return R"()" + text + R"()"; +} + +constexpr const char *red_cell_style = + R"()" + R"()" + R"()"; + +Document document_of(const std::string &source) { + return DecodedFile( + open_strategy::open_file(std::make_shared(source), {}, + Logger::null())) + .as_document_file() + .document(); +} + +Sheet first_sheet(const Document &document) { + return (*document.root_element().children().begin()).as_sheet(); +} + +TableCellStyle fill(const Color color) { + TableCellStyle style; + style.background_color = color; + return style; +} + +TextStyle bold() { + TextStyle style; + style.font_weight = FontWeight::bold; + return style; +} + +/// The style the text of the cell's one run shows. +TextStyle text_style_of(const Sheet &sheet, const std::uint32_t column) { + return sheet.cell(column, 0).first_child().first_child().as_text().style(); +} + +/// The fill of the cell as `0xRRGGBB`, none where it states none. +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; +} + +std::string saved(const Document &document) { + std::ostringstream out; + document.save(out); + return out.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(OdfSheetStyle, a_fill_lands_on_the_cell) { + const Document document = document_of(flat_sheet(string_cell("a"))); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, fill(0xffff00_rgb), {}); + + EXPECT_EQ(fill_at(sheet, 0, 0), 0xffff00u); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); +} + +TEST(OdfSheetStyle, a_style_shared_with_another_cell_is_copied) { + const Document document = document_of(flat_sheet( + string_cell("a", "ce1") + string_cell("b", "ce1"), red_cell_style)); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, {}, bold()); + + EXPECT_EQ(text_style_of(sheet, 0).font_weight, FontWeight::bold); + EXPECT_EQ(fill_at(sheet, 0, 0), 0xff0000u); + EXPECT_NE(text_style_of(sheet, 1).font_weight, FontWeight::bold); + EXPECT_EQ(fill_at(sheet, 1, 0), 0xff0000u); +} + +TEST(OdfSheetStyle, a_named_style_is_inherited_from) { + const Document document = document_of(flat_sheet( + string_cell("a", "Heading"), "", + R"()" + R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, fill(0x00ff00_rgb), {}); + + EXPECT_EQ(text_style_of(sheet, 0).font_weight, FontWeight::bold); + EXPECT_EQ(fill_at(sheet, 0, 0), 0x00ff00u); + EXPECT_NE(saved(document).find(R"(style:parent-style-name="Heading")"), + std::string::npos); +} + +TEST(OdfSheetStyle, one_cell_of_a_repeated_run_takes_the_style_alone) { + const Document document = document_of( + flat_sheet(R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(1, 0, fill(0x0000ff_rgb), {}); + + EXPECT_EQ(fill_at(sheet, 0, 0), std::nullopt); + EXPECT_EQ(fill_at(sheet, 1, 0), 0x0000ffu); + EXPECT_EQ(fill_at(sheet, 2, 0), std::nullopt); +} + +TEST(OdfSheetStyle, an_empty_cell_after_a_span_is_claimed_where_it_sits) { + const Document document = document_of(flat_sheet( + R"(a)" + R"()" + R"()")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(3, 0, fill(0x0000ff_rgb), {}); + sheet.set_cell(2, 0, CellValue("b")); + + EXPECT_EQ(fill_at(sheet, 2, 0), std::nullopt); + EXPECT_EQ(fill_at(sheet, 3, 0), 0x0000ffu); + EXPECT_EQ(sheet.cell(2, 0).value().text(), "b"); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); +} + +TEST(OdfSheetStyle, a_cell_past_the_sheet_is_made) { + const Document document = document_of(flat_sheet(string_cell("a"))); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(3, 2, fill(0x0000ff_rgb), {}); + + EXPECT_EQ(fill_at(sheet, 3, 2), 0x0000ffu); + const Document reopened = document_of(saved(document)); + EXPECT_EQ(fill_at(first_sheet(reopened), 3, 2), 0x0000ffu); +} + +TEST(OdfSheetStyle, one_delta_on_one_base_is_one_style) { + const Document document = document_of(flat_sheet( + string_cell("a", "ce1") + string_cell("b", "ce1") + string_cell("c"), + red_cell_style)); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, {}, bold()); + sheet.set_cell_style(1, 0, {}, bold()); + sheet.set_cell_style(2, 0, {}, bold()); + + // `ce1`, one copy of it, and one style over no base + EXPECT_EQ(count(saved(document), R"(style:family="table-cell")"), 3); +} + +TEST(OdfSheetStyle, a_fill_taken_away_is_written_transparent) { + const Document document = + document_of(flat_sheet(string_cell("a", "ce1"), red_cell_style)); + 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_NE(saved(document).find(R"(fo:background-color="transparent")"), + std::string::npos); +} + +TEST(OdfSheetStyle, an_alignment_is_fixed_on_the_cell) { + const Document document = document_of(flat_sheet(string_cell("a"))); + const Sheet sheet = first_sheet(document); + + TableCellStyle right; + right.horizontal_align = HorizontalAlign::right; + sheet.set_cell_style(0, 0, right, {}); + + EXPECT_EQ(sheet.cell(0, 0).first_child().as_paragraph().style().text_align, + TextAlign::right); + const std::string xml = saved(document); + EXPECT_NE(xml.find(R"(style:text-align-source="fix")"), std::string::npos); + EXPECT_NE(xml.find(R"(fo:text-align="right")"), std::string::npos); +} + +TEST(OdfSheetStyle, a_formula_cell_takes_a_style) { + const Document document = document_of(flat_sheet( + R"(2)")); + const Sheet sheet = first_sheet(document); + + sheet.set_cell_style(0, 0, fill(0xffff00_rgb), {}); + + EXPECT_EQ(fill_at(sheet, 0, 0), 0xffff00u); + EXPECT_EQ(sheet.cell(0, 0).value().formula(), "of:=1+1"); +} + +TEST(OdfSheetStyle, a_covered_cell_or_a_property_no_engine_writes_refuses) { + const Document document = document_of(flat_sheet(string_cell("a"))); + const Sheet sheet = first_sheet(document); + + TableCellStyle wrap; + wrap.wrap_text = true; + EXPECT_THROW(sheet.set_cell_style(0, 0, wrap, {}), UnsupportedOperation); + + const Document merged = document_of(flat_sheet( + R"(a)" + R"()")); + EXPECT_THROW(first_sheet(merged).set_cell_style(1, 0, fill(0xffff00_rgb), {}), + UnsupportedOperation); + EXPECT_THROW(first_sheet(merged).set_cell(1, 0, CellValue("x")), + UnsupportedOperation); + + TextStyle highlight; + highlight.background_color = 0xffff00_rgb; + EXPECT_THROW(sheet.set_cell_style(0, 0, {}, highlight), UnsupportedOperation); +} + +TEST(OdfSheetStyle, the_op_carries_the_fill_and_the_text_keys) { + const Document document = document_of(flat_sheet(string_cell("a"))); + + 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_of(sheet, 0).font_weight, FontWeight::bold); + + EXPECT_THROW( + document.edit( + R"({"version": 2, "ops": [{"op": "setCellStyle", "sheet": 0,)" + R"( "column": 0, "row": 0, "style": {"highlight": "#ffff00"}}]})"), + std::invalid_argument); +} From 4595df3b1d90916ee36072b172fc1e804597f0a7 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 20:58:56 +0200 Subject: [PATCH 2/2] refactor(odf): write a text property through set_attribute `write_text_properties` had its own copy of `set_attribute`, so it now calls that. The lambda that builds the key of a cell style is `key_of`, because `optional` reads like `std::optional`. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Tigcf9he3w1aHfNPJoEGsd --- src/odr/internal/odf/odf_style.cpp | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index 8b437f3f3..b9e3b62c9 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -749,15 +749,19 @@ 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) { - pugi::xml_attribute attribute = properties.attribute(name); - if (!attribute) { - attribute = properties.append_attribute(name); - } - attribute.set_value(value.c_str()); + set_attribute(properties, name, value.c_str()); }; if (style.font_size.has_value()) { @@ -815,14 +819,6 @@ pugi::xml_node properties_of(pugi::xml_node style, const char *name) { return style.append_child(name); } -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); -} - const char *text_align_value(const HorizontalAlign align) { switch (align) { case HorizontalAlign::left: @@ -842,7 +838,7 @@ std::string StyleRegistry::create_cell_style(pugi::xml_node automatic_styles, const TableCellStyle &cell, const TextStyle &text) { const std::string base = base_name != nullptr ? base_name : ""; - const auto optional = [](const auto &value) { + const auto key_of = [](const auto &value) { return value.has_value() ? std::to_string(static_cast(*value)) : std::string("-"); }; @@ -851,9 +847,9 @@ std::string StyleRegistry::create_cell_style(pugi::xml_node automatic_styles, cell.background_color ? fmt::format("{:08x}", cell.background_color->argb()) : "-", - optional(cell.horizontal_align), optional(text.font_weight), - optional(text.font_style), optional(text.font_underline), - optional(text.font_line_through), + key_of(cell.horizontal_align), key_of(text.font_weight), + key_of(text.font_style), key_of(text.font_underline), + key_of(text.font_line_through), text.font_color ? fmt::format("{:08x}", text.font_color->argb()) : "-", text.font_size ? text.font_size->to_string() : "-"); if (const auto it = m_created_cell_styles.find(key);