feat(api)!: TextStyle::font_name is a string of its own

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.

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.

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
 

apple/src/ODRStyle.mm

@@ -3,7 +3,6 @@
 #import "ODRInternal.h"
 #import "ODRPrivate.h"
 
-#include <odr/exceptions.hpp>
 #include <odr/style.hpp>
 
 #include <optional>
@@ -83,10 +82,7 @@
   return value.has_value() ? @(static_cast<NSInteger>(*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 <typename T>
-NSString *_Nullable box_string(const std::optional<T> &value) {
+NSString *_Nullable box_string(const std::optional<std::string> &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;

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;

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<std::string> &value);
-jstring make_string_opt(JNIEnv *env,
-                        const std::optional<std::string_view> &value);
 jobject make_integer_opt(JNIEnv *env, const std::optional<std::int32_t> &value);
 jobject make_measure(JNIEnv *env, const odr::Measure &value);
 jobject make_measure(JNIEnv *env, const std::optional<odr::Measure> &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.

jni/src/jni_style.cpp

@@ -1,8 +1,6 @@
 #include "jni_convert.hpp"
 #include "odr_jni.hpp"
 
-#include <odr/exceptions.hpp>
-
 #include <cstdarg>
 #include <vector>
 
@@ -137,11 +135,6 @@ jstring make_string_opt(JNIEnv *env, const std::optional<std::string> &value) {
   return value.has_value() ? to_jstring(env, *value) : nullptr;
 }
 
-jstring make_string_opt(JNIEnv *env,
-                        const std::optional<std::string_view> &value) {
-  return value.has_value() ? to_jstring(env, *value) : nullptr;
-}
-
 jobject make_integer_opt(JNIEnv *env,
                          const std::optional<std::int32_t> &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<std::string> {
+             if (value == nullptr) {
+               return std::nullopt;
+             }
+             return to_string(env, static_cast<jstring>(value));
+           });
   result.font_size =
       take(field("fontSize", "Lapp/opendocument/core/Measure;"),
            [&](const jobject value) { return measure_from_java(env, value); });

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<odr::LineBreak>(m, "LineBreak")
-      .def("style", &odr::LineBreak::style, keep_self_alive);
+      .def("style", &odr::LineBreak::style);
 
   bind_element<odr::Paragraph>(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<odr::Span>(m, "Span").def("style", &odr::Span::style,
-                                         keep_self_alive);
+  bind_element<odr::Span>(m, "Span").def("style", &odr::Span::style);
 
   bind_element<odr::Text>(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<odr::Link>(m, "Link").def("href", &odr::Link::href);
 
@@ -251,7 +249,7 @@ void odr_python::bind_document(py::module_ &m) {
   bind_element<odr::List>(m, "List").def("list_type", &odr::List::type);
 
   bind_element<odr::ListItem>(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);
 

python/src/bind_style.cpp

@@ -134,14 +134,7 @@ void odr_python::bind_style(py::module_ &m) {
 
   py::class_<odr::TextStyle>(m, "TextStyle")
       .def(py::init<>())
-      .def_property_readonly(
-          "font_name",
-          [](const odr::TextStyle &style) -> std::optional<std::string> {
-            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)

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<std::string_view> font_name = text_style.font_name;
+  if (const std::optional<std::string> &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<Measure> 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<std::string_view> font_name = text_style.font_name;
+  if (const std::optional<std::string> &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<Measure> font_size = text_style.font_size;

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 `<blockquote>` 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;

src/odr/internal/oldms/presentation/ppt_parser.cpp

@@ -751,8 +751,7 @@ collect_slides(std::istream &current_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 &registry,
     }
   }
 
-  // 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;
 }

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<std::string> font_names,
-                             std::vector<TextStyle> styles)
-    : m_font_names(std::move(font_names)), m_styles(std::move(styles)) {}
+StyleRegistry::StyleRegistry(std::vector<TextStyle> 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<std::uint32_t>(context.styles.size() - 1);

src/odr/internal/oldms/presentation/ppt_style.hpp

@@ -29,27 +29,19 @@ struct TextCFRun final {
 std::vector<TextCFRun> 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<std::string> font_names,
-                std::vector<TextStyle> styles);
+  /// `styles` are the resolved character styles, index 0 the default style.
+  explicit StyleRegistry(std::vector<TextStyle> 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<std::string> m_font_names;
   std::vector<TextStyle> 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<std::string> fonts;
   std::vector<TextStyle> styles{default_character_style()};

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<Font> m_fonts;
   std::vector<ResolvedStyle> m_cell_styles;
 };

src/odr/internal/oldms/text/doc_parser.cpp

@@ -170,17 +170,15 @@ ElementIdentifier text::parse_tree(ElementRegistry &registry,
   }
   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<std::string> font_names =
+  const std::vector<std::string> font_names =
       read_font_names(*table_stream, fib.fibRgFcLcb->sttbfFfn);
 
   // Direct character formatting only ([MS-DOC] 2.4.6.2).
   std::vector<TextStyle> styles{default_character_style()}; // index 0
   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);

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<std::string> font_names,
-                             std::vector<TextStyle> styles)
-    : m_font_names(std::move(font_names)), m_styles(std::move(styles)) {}
+StyleRegistry::StyleRegistry(std::vector<TextStyle> styles)
+    : m_styles(std::move(styles)) {}
 
 const TextStyle &StyleRegistry::text_style(const std::uint32_t index) const {
   return m_styles.at(index);