From 7957aacfbf203a262c4989a195a1fcc33c1eed4e Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 13 Sep 2026 19:55:04 +0200 Subject: [PATCH] feat(api)!: TextStyle::font_name is a string of its own The `std::string_view` borrowed from the document that produced the style, so a style could not outlive its document and a caller could not build one with a font name, which `Text::set_style` made a real gap. It is a `std::string` now. The `.doc` and `.ppt` registries stop carrying the font names for lifetime alone, the python style getters drop the keep-alive they needed only for it, and the Java and Objective-C converters pass a font name through to C++, which refuses it as before. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0137vd79NGaB8nfLsdPoghM4 --- CHANGELOG.md | 4 +++ apple/include/OdrCoreObjC/ODRStyle.h | 2 +- apple/src/ODRPrivate.h | 3 +-- apple/src/ODRStyle.mm | 8 ++---- jni/java/app/opendocument/core/TextStyle.java | 2 +- jni/src/jni_convert.hpp | 5 +--- jni/src/jni_style.cpp | 21 ++++++---------- python/src/bind_document.cpp | 14 +++++------ python/src/bind_style.cpp | 9 +------ src/odr/internal/html/document_style.cpp | 8 +++--- src/odr/internal/markdown/markdown_style.cpp | 5 ++-- .../oldms/presentation/ppt_parser.cpp | 8 ++---- .../internal/oldms/presentation/ppt_style.cpp | 7 +++--- .../internal/oldms/presentation/ppt_style.hpp | 25 ++++++------------- .../internal/oldms/spreadsheet/xls_style.hpp | 4 +-- src/odr/internal/oldms/text/doc_parser.cpp | 6 ++--- src/odr/internal/oldms/text/doc_style.cpp | 5 ++-- src/odr/internal/oldms/text/doc_style.hpp | 18 ++++--------- src/odr/style.hpp | 7 ++---- test/src/document_edit_test.cpp | 5 ++++ 20 files changed, 61 insertions(+), 105 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4c143e60..21832dedf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ The release run heads these entries with the version and opens a fresh `Sheet::set_cell` writes a boolean and refuses the other three. The enum mirrors in every binding follow. +- **Breaking**: `TextStyle::font_name` is a `std::string` rather than a + `std::string_view` that borrowed from the document, so a style outlives the + document it was read from and a caller can build one. + - `Text::set_style` and the edit op `setTextStyle {id, style}` write bold, italic, underline, strikethrough, highlight, colour and size onto a run of an odf, docx or pptx document. The other engines refuse it. diff --git a/apple/include/OdrCoreObjC/ODRStyle.h b/apple/include/OdrCoreObjC/ODRStyle.h index 874a10862..7642f25c0 100644 --- a/apple/include/OdrCoreObjC/ODRStyle.h +++ b/apple/include/OdrCoreObjC/ODRStyle.h @@ -146,7 +146,7 @@ NS_SWIFT_NAME(DirectionalString) /// property `nil`, and a `nil` property is left alone on the run. NS_SWIFT_NAME(TextStyle) @interface ODRTextStyle : NSObject -/// Read only: it borrows from the document, and `setStyle:` refuses it. +/// Read only: `setStyle:` refuses it. @property(nonatomic, nullable, copy) NSString *fontName; @property(nonatomic, nullable) ODRMeasure *fontSize; /// `ODRFontWeight`, boxed. diff --git a/apple/src/ODRPrivate.h b/apple/src/ODRPrivate.h index 112b0c1ff..54aedf14d 100644 --- a/apple/src/ODRPrivate.h +++ b/apple/src/ODRPrivate.h @@ -103,8 +103,7 @@ NS_ASSUME_NONNULL_BEGIN @interface ODRTextStyle (Private) + (instancetype)styleWithHandle:(const odr::TextStyle &)handle; -/// The set properties as a `TextStyle`; throws `UnsupportedOperation` for a -/// `fontName`, which the C++ one borrows from a document. +/// The set properties as a `TextStyle`. - (odr::TextStyle)handle; @end diff --git a/apple/src/ODRStyle.mm b/apple/src/ODRStyle.mm index 8a519611c..5e2b7fb09 100644 --- a/apple/src/ODRStyle.mm +++ b/apple/src/ODRStyle.mm @@ -3,7 +3,6 @@ #import "ODRInternal.h" #import "ODRPrivate.h" -#include #include #include @@ -83,10 +82,7 @@ return value.has_value() ? @(static_cast(*value)) : nil; } -/// Also takes the `string_view` of `font_name`, which borrows from the document -/// that produced the style — copying it here is the point. -template -NSString *_Nullable box_string(const std::optional &value) { +NSString *_Nullable box_string(const std::optional &value) { return value.has_value() ? to_nsstring(*value) : nil; } @@ -224,7 +220,7 @@ + (instancetype)styleWithHandle:(const odr::TextStyle &)handle { - (odr::TextStyle)handle { odr::TextStyle result; if (_fontName != nil) { - throw odr::UnsupportedOperation(); + result.font_name = to_string(_fontName); } if (_fontSize != nil) { result.font_size = _fontSize.handle; diff --git a/jni/java/app/opendocument/core/TextStyle.java b/jni/java/app/opendocument/core/TextStyle.java index a10ff4230..1d5d54d69 100644 --- a/jni/java/app/opendocument/core/TextStyle.java +++ b/jni/java/app/opendocument/core/TextStyle.java @@ -6,7 +6,7 @@ * left alone on the run. */ public final class TextStyle { - /** Read only: it borrows from the document, and {@link Text#setStyle} refuses it. */ + /** Read only: {@link Text#setStyle} refuses it. */ public String fontName; public Measure fontSize; public FontWeight fontWeight; diff --git a/jni/src/jni_convert.hpp b/jni/src/jni_convert.hpp index aaa620dba..9ecb376b2 100644 --- a/jni/src/jni_convert.hpp +++ b/jni/src/jni_convert.hpp @@ -17,8 +17,6 @@ namespace odr_jni { // C++ to Java; optionals map to null. jstring make_string_opt(JNIEnv *env, const std::optional &value); -jstring make_string_opt(JNIEnv *env, - const std::optional &value); jobject make_integer_opt(JNIEnv *env, const std::optional &value); jobject make_measure(JNIEnv *env, const odr::Measure &value); jobject make_measure(JNIEnv *env, const std::optional &value); @@ -55,8 +53,7 @@ jobject make_file_type_capabilities(JNIEnv *env, jobject html_config_to_java(JNIEnv *env, const odr::HtmlConfig &config); odr::HtmlConfig html_config_from_java(JNIEnv *env, jobject config); -/// The fields a Java `TextStyle` states; a `fontName` is refused, since the -/// C++ one borrows from a document. +/// The fields a Java `TextStyle` states. odr::TextStyle text_style_from_java(JNIEnv *env, jobject style); /// Optional enum to a Java-side code; -1 encodes absent. diff --git a/jni/src/jni_style.cpp b/jni/src/jni_style.cpp index 2bc29f63f..226939193 100644 --- a/jni/src/jni_style.cpp +++ b/jni/src/jni_style.cpp @@ -1,8 +1,6 @@ #include "jni_convert.hpp" #include "odr_jni.hpp" -#include - #include #include @@ -137,11 +135,6 @@ jstring make_string_opt(JNIEnv *env, const std::optional &value) { return value.has_value() ? to_jstring(env, *value) : nullptr; } -jstring make_string_opt(JNIEnv *env, - const std::optional &value) { - return value.has_value() ? to_jstring(env, *value) : nullptr; -} - jobject make_integer_opt(JNIEnv *env, const std::optional &value) { if (!value.has_value()) { @@ -295,12 +288,14 @@ odr::TextStyle text_style_from_java(JNIEnv *env, const jobject style) { return converted; }; - if (const jobject font_name = field("fontName", "Ljava/lang/String;"); - font_name != nullptr) { - env->DeleteLocalRef(font_name); - env->DeleteLocalRef(cls); - throw odr::UnsupportedOperation(); - } + result.font_name = + take(field("fontName", "Ljava/lang/String;"), + [&](const jobject value) -> std::optional { + if (value == nullptr) { + return std::nullopt; + } + return to_string(env, static_cast(value)); + }); result.font_size = take(field("fontSize", "Lapp/opendocument/core/Measure;"), [&](const jobject value) { return measure_from_java(env, value); }); diff --git a/python/src/bind_document.cpp b/python/src/bind_document.cpp index de97e556a..d92d3a32b 100644 --- a/python/src/bind_document.cpp +++ b/python/src/bind_document.cpp @@ -19,8 +19,7 @@ namespace py = pybind11; namespace { // Ties the returned object to `self`: navigation handles keep the originating -// `Document` alive transitively, and so does a `TextStyle`, whose `font_name` -// borrows from the document. +// `Document` alive transitively. constexpr auto keep_self_alive = py::keep_alive<0, 1>(); py::object make_element_iterator(const odr::ElementRange &range) { @@ -229,20 +228,19 @@ void odr_python::bind_document(py::module_ &m) { .def("page_layout", &odr::MasterPage::page_layout); bind_element(m, "LineBreak") - .def("style", &odr::LineBreak::style, keep_self_alive); + .def("style", &odr::LineBreak::style); bind_element(m, "Paragraph") .def("style", &odr::Paragraph::style) - .def("text_style", &odr::Paragraph::text_style, keep_self_alive); + .def("text_style", &odr::Paragraph::text_style); - bind_element(m, "Span").def("style", &odr::Span::style, - keep_self_alive); + bind_element(m, "Span").def("style", &odr::Span::style); bind_element(m, "Text") .def("content", &odr::Text::content) .def("set_content", &odr::Text::set_content, py::arg("text")) .def("set_style", &odr::Text::set_style, py::arg("style")) - .def("style", &odr::Text::style, keep_self_alive); + .def("style", &odr::Text::style); bind_element(m, "Link").def("href", &odr::Link::href); @@ -251,7 +249,7 @@ void odr_python::bind_document(py::module_ &m) { bind_element(m, "List").def("list_type", &odr::List::type); bind_element(m, "ListItem") - .def("style", &odr::ListItem::style, keep_self_alive) + .def("style", &odr::ListItem::style) .def("marker", &odr::ListItem::marker) .def("number", &odr::ListItem::number); diff --git a/python/src/bind_style.cpp b/python/src/bind_style.cpp index 5b1605816..06c33461d 100644 --- a/python/src/bind_style.cpp +++ b/python/src/bind_style.cpp @@ -134,14 +134,7 @@ void odr_python::bind_style(py::module_ &m) { py::class_(m, "TextStyle") .def(py::init<>()) - .def_property_readonly( - "font_name", - [](const odr::TextStyle &style) -> std::optional { - if (!style.font_name.has_value()) { - return std::nullopt; - } - return std::string(*style.font_name); - }) + .def_readwrite("font_name", &odr::TextStyle::font_name) .def_readwrite("font_size", &odr::TextStyle::font_size) .def_readwrite("font_weight", &odr::TextStyle::font_weight) .def_readwrite("font_style", &odr::TextStyle::font_style) diff --git a/src/odr/internal/html/document_style.cpp b/src/odr/internal/html/document_style.cpp index 264fc7627..e39df83d9 100644 --- a/src/odr/internal/html/document_style.cpp +++ b/src/odr/internal/html/document_style.cpp @@ -177,10 +177,10 @@ std::string html::translate_inner_page_style(const PageLayout &page_layout) { std::string html::translate_text_style(const TextStyle &text_style) { std::string result; - if (const std::optional font_name = text_style.font_name; + if (const std::optional &font_name = text_style.font_name; font_name.has_value()) { result.append("font-family:") - .append(xml::escape_attribute(std::string(*font_name))) + .append(xml::escape_attribute(*font_name)) .append(";"); } if (const std::optional font_size = text_style.font_size; @@ -245,10 +245,10 @@ std::string html::translate_text_style(const TextStyle &text_style) { std::string html::translate_block_font_style(const TextStyle &text_style) { std::string result; - if (const std::optional font_name = text_style.font_name; + if (const std::optional &font_name = text_style.font_name; font_name.has_value()) { result.append("font-family:") - .append(xml::escape_attribute(std::string(*font_name))) + .append(xml::escape_attribute(*font_name)) .append(";"); } if (const std::optional font_size = text_style.font_size; diff --git a/src/odr/internal/markdown/markdown_style.cpp b/src/odr/internal/markdown/markdown_style.cpp index f1677f3b9..544e6eb7c 100644 --- a/src/odr/internal/markdown/markdown_style.cpp +++ b/src/odr/internal/markdown/markdown_style.cpp @@ -15,9 +15,8 @@ constexpr std::array heading_font_sizes{2.0, 1.5, 1.17, 1.0, 0.83, 0.67}; /// One `
` worth of indent, per nesting level. constexpr double quote_margin = 2.5; -/// Nothing in a markdown file names a face. Static storage, so -/// `TextStyle::font_name` may point at it. -constexpr std::string_view monospace_font_name = "monospace"; +/// Nothing in a markdown file names a face. +constexpr const char *monospace_font_name = "monospace"; constexpr std::uint32_t default_style_index = 0; constexpr std::uint32_t first_heading_style_index = 1; diff --git a/src/odr/internal/oldms/presentation/ppt_parser.cpp b/src/odr/internal/oldms/presentation/ppt_parser.cpp index c903f87b1..f6c6f1e3e 100644 --- a/src/odr/internal/oldms/presentation/ppt_parser.cpp +++ b/src/odr/internal/oldms/presentation/ppt_parser.cpp @@ -751,8 +751,7 @@ collect_slides(std::istream ¤t_user, std::istream &document, } const std::uint32_t doc_offset = doc_it->second; - // The font names must be complete before the first style is resolved — the - // styles' `font_name` point into them. + // the font names must be complete before the first style is resolved { document.clear(); document.seekg(doc_offset); @@ -911,10 +910,7 @@ presentation::parse_tree(ElementRegistry ®istry, } } - // The styles' `font_name` point into the font-name strings, which keep - // their buffers when the vectors are moved into the registry. - style_registry = - StyleRegistry(std::move(context.fonts), std::move(context.styles)); + style_registry = StyleRegistry(std::move(context.styles)); return root_id; } diff --git a/src/odr/internal/oldms/presentation/ppt_style.cpp b/src/odr/internal/oldms/presentation/ppt_style.cpp index eb83f4f1d..2bdf95266 100644 --- a/src/odr/internal/oldms/presentation/ppt_style.cpp +++ b/src/odr/internal/oldms/presentation/ppt_style.cpp @@ -181,9 +181,8 @@ void read_text_cf_exception(BodyCursor &cursor, TextCFRun &run) { } // namespace -StyleRegistry::StyleRegistry(std::vector font_names, - std::vector styles) - : m_font_names(std::move(font_names)), m_styles(std::move(styles)) {} +StyleRegistry::StyleRegistry(std::vector styles) + : m_styles(std::move(styles)) {} const TextStyle &StyleRegistry::text_style(const std::uint32_t index) const { return m_styles.at(index); @@ -249,7 +248,7 @@ std::uint32_t presentation::resolve_style(const TextCFRun &run, context.fonts[*run.font_ref].empty()) { throw std::runtime_error("ppt: font reference out of range"); } - style.font_name = context.fonts[*run.font_ref].c_str(); + style.font_name = context.fonts[*run.font_ref]; } context.styles.push_back(style); return static_cast(context.styles.size() - 1); diff --git a/src/odr/internal/oldms/presentation/ppt_style.hpp b/src/odr/internal/oldms/presentation/ppt_style.hpp index a39102629..0d76c34bb 100644 --- a/src/odr/internal/oldms/presentation/ppt_style.hpp +++ b/src/odr/internal/oldms/presentation/ppt_style.hpp @@ -29,27 +29,19 @@ struct TextCFRun final { std::vector parse_style_text_prop_atom(std::string_view body, std::size_t char_count); -/// Owns the document's resolved character styles — indexed by the style index -/// stored on paragraph/span elements, 0 being the default style — and the -/// font names `TextStyle::font_name` points into. Immutable after -/// construction. +/// Owns the document's resolved character styles, indexed by the style index +/// stored on paragraph/span elements, 0 being the default style. Immutable +/// after construction. class StyleRegistry final { public: StyleRegistry() = default; - /// `font_names` are the FontCollection names the styles' `font_name` point - /// into; `styles` are the resolved character styles, index 0 the default - /// style. - StyleRegistry(std::vector font_names, - std::vector styles); + /// `styles` are the resolved character styles, index 0 the default style. + explicit StyleRegistry(std::vector styles); /// Throws if the index has no style. [[nodiscard]] const TextStyle &text_style(std::uint32_t index) const; private: - /// Owns the font names: `TextStyle::font_name` (`const char *`) points into - /// them. Never modified after construction (moving the registry is fine — - /// the strings themselves do not move). - std::vector m_font_names; std::vector m_styles; }; @@ -62,10 +54,9 @@ class StyleRegistry final { constexpr std::uint32_t default_style_index = 0; /// Accumulates the font names and resolved character styles while parsing; -/// moved into the `StyleRegistry` once the tree is built. `fonts` is indexed -/// by FontEntityAtom recInstance (an empty string marks a gap) and must be -/// complete before the first style is resolved — the styles' `font_name` -/// point into its strings. +/// the styles move into the `StyleRegistry` once the tree is built. `fonts` +/// is indexed by FontEntityAtom recInstance (an empty string marks a gap) and +/// must be complete before the first style is resolved. struct StyleContext final { std::vector fonts; std::vector styles{default_character_style()}; diff --git a/src/odr/internal/oldms/spreadsheet/xls_style.hpp b/src/odr/internal/oldms/spreadsheet/xls_style.hpp index 107af4342..0e9114b4c 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_style.hpp +++ b/src/odr/internal/oldms/spreadsheet/xls_style.hpp @@ -32,9 +32,7 @@ class StyleRegistry final { [[nodiscard]] const ResolvedStyle &cell_style(std::uint16_t ixfe) const; private: - /// Owns the font names: `TextStyle::font_name` (a `std::string_view`) points - /// into them. Never modified after construction (moving the registry is fine - /// — the strings themselves do not move). + /// The fonts the XF records index. std::vector m_fonts; std::vector m_cell_styles; }; diff --git a/src/odr/internal/oldms/text/doc_parser.cpp b/src/odr/internal/oldms/text/doc_parser.cpp index 53baef488..83236c063 100644 --- a/src/odr/internal/oldms/text/doc_parser.cpp +++ b/src/odr/internal/oldms/text/doc_parser.cpp @@ -170,9 +170,7 @@ ElementIdentifier text::parse_tree(ElementRegistry ®istry, } const auto table_stream = table_file->stream(); - // TextStyle::font_name points into these strings, which keep their buffers - // when the vector is moved into the registry below. - std::vector font_names = + const std::vector font_names = read_font_names(*table_stream, fib.fibRgFcLcb->sttbfFfn); // Direct character formatting only ([MS-DOC] 2.4.6.2). @@ -180,7 +178,7 @@ ElementIdentifier text::parse_tree(ElementRegistry ®istry, const CharacterRuns character_runs = read_character_runs(*document_stream, *table_stream, fib.fibRgFcLcb->plcfBteChpx, styles, font_names); - style_registry = StyleRegistry(std::move(font_names), std::move(styles)); + style_registry = StyleRegistry(std::move(styles)); table_stream->seekg(fib.fibRgFcLcb->clx.fc); const CharacterIndex character_index = read_character_index(*table_stream); diff --git a/src/odr/internal/oldms/text/doc_style.cpp b/src/odr/internal/oldms/text/doc_style.cpp index 47fddd87e..673c23921 100644 --- a/src/odr/internal/oldms/text/doc_style.cpp +++ b/src/odr/internal/oldms/text/doc_style.cpp @@ -50,9 +50,8 @@ bool toggle_on(const std::uint8_t value) { } // namespace -StyleRegistry::StyleRegistry(std::vector font_names, - std::vector styles) - : m_font_names(std::move(font_names)), m_styles(std::move(styles)) {} +StyleRegistry::StyleRegistry(std::vector styles) + : m_styles(std::move(styles)) {} const TextStyle &StyleRegistry::text_style(const std::uint32_t index) const { return m_styles.at(index); diff --git a/src/odr/internal/oldms/text/doc_style.hpp b/src/odr/internal/oldms/text/doc_style.hpp index d289baddb..2ad8b0182 100644 --- a/src/odr/internal/oldms/text/doc_style.hpp +++ b/src/odr/internal/oldms/text/doc_style.hpp @@ -13,26 +13,19 @@ namespace odr::internal::oldms::text { -/// Owns the document's resolved character styles — indexed by the style index -/// stored on paragraph/span elements, 0 being the default style — and the -/// font names `TextStyle::font_name` points into. Immutable after -/// construction. +/// Owns the document's resolved character styles, indexed by the style index +/// stored on paragraph/span elements, 0 being the default style. Immutable +/// after construction. class StyleRegistry final { public: StyleRegistry() = default; - /// `font_names` are the SttbfFfn names the styles' `font_name` point into; /// `styles` are the resolved character styles, index 0 the default style. - StyleRegistry(std::vector font_names, - std::vector styles); + explicit StyleRegistry(std::vector styles); /// Throws if the index has no style. [[nodiscard]] const TextStyle &text_style(std::uint32_t index) const; private: - /// Owns the font names: `TextStyle::font_name` (a `std::string_view`) points - /// into them. Never modified after construction (moving the registry is fine - /// — the strings themselves do not move). - std::vector m_font_names; std::vector m_styles; }; @@ -42,8 +35,7 @@ class StyleRegistry final { /// Applies the character SPRMs of a Chpx grpprl ([MS-DOC] 2.6.1) on top of /// `style`; non-character SPRMs are skipped via their operand size. -/// `font_names` resolves sprmCRgFtc0 (the strings must outlive the style — -/// `TextStyle::font_name` points into them). +/// `font_names` resolves sprmCRgFtc0. TextStyle apply_character_sprms(TextStyle style, std::string_view grpprl, std::span font_names); diff --git a/src/odr/style.hpp b/src/odr/style.hpp index 21bfa639f..bffdd273f 100644 --- a/src/odr/style.hpp +++ b/src/odr/style.hpp @@ -156,12 +156,9 @@ template struct DirectionalStyle final { } }; -/// @brief Represents a style for text. -/// -/// @note `font_name` borrows from the document that produced the style and is -/// only valid for as long as that document is alive. +/// Represents a style for text. struct TextStyle final { - std::optional font_name; + std::optional font_name; std::optional font_size; std::optional font_weight; std::optional font_style; diff --git a/test/src/document_edit_test.cpp b/test/src/document_edit_test.cpp index 3c3541aeb..160cd6ac7 100644 --- a/test/src/document_edit_test.cpp +++ b/test/src/document_edit_test.cpp @@ -833,6 +833,11 @@ TEST(DocumentEdit, a_style_the_handle_does_not_write_refuses) { EXPECT_THROW(run_at(document, 0, 0).as_text().set_style(style), UnsupportedOperation); + + TextStyle named; + named.font_name = "Comic Sans"; + EXPECT_THROW(run_at(document, 0, 0).as_text().set_style(named), + UnsupportedOperation); } namespace {