From 44b28dbbaba6839ffa046180dc731a38bcbc9e81 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 21:56:57 +0200 Subject: [PATCH 1/2] refactor(xml): move insert_in_sequence into xml_util The odf style writer ordered the properties of a style with its own copy of `insert_in_sequence`, which only the ooxml writers could reach. The helper now sits in `xml_util` beside `set_attribute`, so the odf and the ooxml writers share one. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Tigcf9he3w1aHfNPJoEGsd --- src/odr/internal/odf/odf_style.cpp | 11 +---------- src/odr/internal/ooxml/AGENTS.md | 2 +- src/odr/internal/ooxml/ooxml_util.cpp | 18 ------------------ src/odr/internal/ooxml/ooxml_util.hpp | 5 ----- .../ooxml_presentation_document.cpp | 2 +- .../spreadsheet/ooxml_spreadsheet_style.cpp | 2 +- .../ooxml/text/ooxml_text_document.cpp | 2 +- src/odr/internal/xml/AGENTS.md | 5 +++-- src/odr/internal/xml/xml_util.cpp | 19 +++++++++++++++++++ src/odr/internal/xml/xml_util.hpp | 5 +++++ 10 files changed, 32 insertions(+), 39 deletions(-) diff --git a/src/odr/internal/odf/odf_style.cpp b/src/odr/internal/odf/odf_style.cpp index 272174506..25ef8ba73 100644 --- a/src/odr/internal/odf/odf_style.cpp +++ b/src/odr/internal/odf/odf_style.cpp @@ -4,7 +4,6 @@ #include #include -#include #include #include #include @@ -801,15 +800,7 @@ pugi::xml_node properties_of(pugi::xml_node style, const char *name) { 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); + return xml::insert_in_sequence(style, name, order); } const char *text_align_value(const HorizontalAlign align) { diff --git a/src/odr/internal/ooxml/AGENTS.md b/src/odr/internal/ooxml/AGENTS.md index dfa639ad7..d0caae63c 100644 --- a/src/odr/internal/ooxml/AGENTS.md +++ b/src/odr/internal/ooxml/AGENTS.md @@ -39,7 +39,7 @@ a per-part `ParseContext` (path, its relations, the part cache). |---|---| | `ooxml_file.{hpp,cpp}` | `OfficeOpenXmlFile`: meta, encryption state, `decrypt()`, dispatch to the per-format `Document` on `file_type()` | | `ooxml_meta.cpp` | `parse_file_meta`: type detection by sentinel path (`/word/document.xml`, `/ppt/presentation.xml`, `/xl/workbook.xml`); an encrypted package has `/EncryptionInfo` and `/EncryptedPackage` | -| `ooxml_util.{hpp,cpp}` | Stateless attribute readers: half-points, hundredth-points, EMUs, twips, percents, colours, borders, font weight and style; relationship parsing; `write_text_nodes`, `insert_in_sequence` for the writers | +| `ooxml_util.{hpp,cpp}` | Stateless attribute readers: half-points, hundredth-points, EMUs, twips, percents, colours, borders, font weight and style; relationship parsing; `write_text_nodes` for the writers | | `ooxml_crypto.{hpp,cpp}` | Decryption | ## Encryption diff --git a/src/odr/internal/ooxml/ooxml_util.cpp b/src/odr/internal/ooxml/ooxml_util.cpp index 6cbe111db..e5f7c3318 100644 --- a/src/odr/internal/ooxml/ooxml_util.cpp +++ b/src/odr/internal/ooxml/ooxml_util.cpp @@ -133,24 +133,6 @@ constexpr std::array, 16> } // namespace -pugi::xml_node -ooxml::insert_in_sequence(pugi::xml_node parent, const char *name, - const std::span order) { - const auto rank = [&](const std::string_view child_name) { - const auto it = std::ranges::find(order, child_name); - return it == std::end(order) - ? order.size() - : static_cast(it - std::begin(order)); - }; - const std::size_t own_rank = rank(name); - for (const pugi::xml_node child : parent.children()) { - if (rank(child.name()) > own_rank) { - return parent.insert_child_before(name, child); - } - } - return parent.append_child(name); -} - std::string ooxml::hex_color(const Color &color) { return fmt::format("{:06X}", color.rgb()); } diff --git a/src/odr/internal/ooxml/ooxml_util.hpp b/src/odr/internal/ooxml/ooxml_util.hpp index e8a47a552..98d7f263b 100644 --- a/src/odr/internal/ooxml/ooxml_util.hpp +++ b/src/odr/internal/ooxml/ooxml_util.hpp @@ -5,7 +5,6 @@ #include #include -#include #include #include #include @@ -37,10 +36,6 @@ xml::NodeSpan write_text_nodes(pugi::xml_node parent, pugi::xml_node before, const std::string &text, std::string_view prefix); -/// Inserts a child @p name into @p parent at its place in the schema -/// sequence @p order; a child the sequence does not name ranks last. -pugi::xml_node insert_in_sequence(pugi::xml_node parent, const char *name, - std::span order); /// `RRGGBB`, as `w:color/@w:val` and `a:srgbClr/@val` spell one. std::string hex_color(const Color &color); /// The `w:highlight` name of @p color, where it is one of the sixteen diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp index 151a5d4b8..a51326c4e 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp @@ -218,7 +218,7 @@ void write_run_properties(pugi::xml_node properties, const TextStyle &style) { attr.set_value(value.c_str()); }; const auto solid = [&](const char *name, const Color &color) { - insert_in_sequence(properties, name, run_property_order) + xml::insert_in_sequence(properties, name, run_property_order) .append_child("a:srgbClr") .append_attribute("val") .set_value(hex_color(color).c_str()); diff --git a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp index 8c6de19c5..441233c18 100644 --- a/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp +++ b/src/odr/internal/ooxml/spreadsheet/ooxml_spreadsheet_style.cpp @@ -81,7 +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; } - return insert_in_sequence(parent, name, order); + return xml::insert_in_sequence(parent, name, order); } /// The @p name child of `styleSheet`, made in the order [ECMA-376] 18.8.39 diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.cpp b/src/odr/internal/ooxml/text/ooxml_text_document.cpp index 08a8e4db6..978f6e0b1 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.cpp @@ -215,7 +215,7 @@ constexpr std::array run_property_order{ /// `w:themeColor` would win over a new `w:val`. pugi::xml_node set_run_property(pugi::xml_node properties, const char *name) { properties.remove_child(name); - return insert_in_sequence(properties, name, run_property_order); + return xml::insert_in_sequence(properties, name, run_property_order); } /// Writes the set fields of @p style into a `w:rPr`, the complex-script diff --git a/src/odr/internal/xml/AGENTS.md b/src/odr/internal/xml/AGENTS.md index b39a67210..31eabc157 100644 --- a/src/odr/internal/xml/AGENTS.md +++ b/src/odr/internal/xml/AGENTS.md @@ -6,8 +6,9 @@ xml does differently, and why. Open work is in [`PLAN.md`](PLAN.md). ## Three things live here - `xml_util` is the shared plumbing: `parse`, `escape_text`, - `escape_attribute`, `read_declared_encoding`, `tokenize_text`. odf, ooxml, - svg and the html writer go through it. It depends on no other engine. + `escape_attribute`, `read_declared_encoding`, `tokenize_text`, and for the + writers `set_attribute` and `insert_in_sequence`. odf, ooxml, svg and the + html writer go through it. It depends on no other engine. - `xml_tree_edit` is the node editing the odf and ooxml write sides share. - `xml_file` is the format: xml opened as a file of its own and rendered as a source view. The rest of this file is about it. diff --git a/src/odr/internal/xml/xml_util.cpp b/src/odr/internal/xml/xml_util.cpp index 434c5512f..603250b1e 100644 --- a/src/odr/internal/xml/xml_util.cpp +++ b/src/odr/internal/xml/xml_util.cpp @@ -8,6 +8,7 @@ #include +#include #include #include #include @@ -98,6 +99,24 @@ void xml::set_attribute(pugi::xml_node node, const char *name, attribute.set_value(value); } +pugi::xml_node +xml::insert_in_sequence(pugi::xml_node parent, const char *name, + const std::span order) { + const auto rank = [&](const std::string_view child_name) { + const auto it = std::ranges::find(order, child_name); + return it == std::end(order) + ? order.size() + : static_cast(it - std::begin(order)); + }; + const std::size_t own_rank = rank(name); + for (const pugi::xml_node child : parent.children()) { + if (rank(child.name()) > own_rank) { + return parent.insert_child_before(name, child); + } + } + return parent.append_child(name); +} + 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 7667a507c..cd1459502 100644 --- a/src/odr/internal/xml/xml_util.hpp +++ b/src/odr/internal/xml/xml_util.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include @@ -36,6 +37,10 @@ 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); +/// Inserts a child @p name into @p parent at its place in the schema +/// sequence @p order; a child the sequence does not name ranks last. +pugi::xml_node insert_in_sequence(pugi::xml_node parent, const char *name, + std::span order); /// Throws unless @p in holds a well formed xml document. void check_xml_file(std::istream &in); From 18546ba8c382bd87bd8a1ae87492bc9a84c33cd4 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 28 Sep 2026 21:56:57 +0200 Subject: [PATCH 2/2] refactor(ooxml): write a run property through xml::set_attribute `write_run_properties` in the pptx writer had its own copy of `set_attribute`, so its lambda now calls the shared one. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Tigcf9he3w1aHfNPJoEGsd --- .../ooxml/presentation/ooxml_presentation_document.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp index a51326c4e..8d84ece9b 100644 --- a/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp +++ b/src/odr/internal/ooxml/presentation/ooxml_presentation_document.cpp @@ -211,11 +211,7 @@ constexpr std::array fill_names{"a:noFill", "a:solidFill", /// size as attributes, the colour and the highlight as children. void write_run_properties(pugi::xml_node properties, const TextStyle &style) { const auto attribute = [&](const char *name, const std::string &value) { - pugi::xml_attribute attr = properties.attribute(name); - if (!attr) { - attr = properties.append_attribute(name); - } - attr.set_value(value.c_str()); + xml::set_attribute(properties, name, value.c_str()); }; const auto solid = [&](const char *name, const Color &color) { xml::insert_in_sequence(properties, name, run_property_order)