fix: whole-codebase correctness and conciseness review

android/src/androidTest/java/app/opendocument/core/DocumentTest.kt

@@ -94,8 +94,6 @@ class DocumentTest {
 
         val pages = html.pages()
         assertEquals(1, pages.size)
-        // the renderer reads the css/js the AAR ships, so this only passes with the
-        // extracted assets in place
         val content = read(Paths.get(pages[0].path))
         assertTrue(content.contains(TestFiles.ODT_WORD))
     }

apple/include/OdrCoreObjC/ODRTable.h

@@ -35,8 +35,11 @@ NS_INLINE ODRTablePosition ODRTablePositionMake(uint32_t column, uint32_t row) {
 NS_SWIFT_NAME(TableAddress)
 @interface ODRTableAddress : NSObject
 
+/// 0 for anything that is not a column, e.g. `"c"` or `""`; use
+/// `position:fromString:` to be told why instead.
 + (uint32_t)columnNumberFromString:(NSString *)string
     NS_SWIFT_NAME(columnNumber(from:));
+/// 0 for anything that is not a row number.
 + (uint32_t)rowNumberFromString:(NSString *)string
     NS_SWIFT_NAME(rowNumber(from:));
 + (NSString *)stringFromColumnNumber:(uint32_t)column

apple/src/ODRDocumentElement.mm

@@ -60,13 +60,24 @@ ODRTableDimensions to_dimensions(const odr::TableDimensions &value) {
   return ODRTableDimensionsMake(value.rows, value.columns);
 }
 
+/// Wraps a range through `-derive:`, so `source`'s owner is carried along.
+NSArray<ODRElement *> *to_nsarray(ODRElement *const source,
+                                  const odr::ElementRange &range) {
+  NSMutableArray<ODRElement *> *const result = [NSMutableArray array];
+  for (const odr::Element element : range) {
+    if (ODRElement *const wrapped = [source derive:element]; wrapped != nil) {
+      [result addObject:wrapped];
+    }
+  }
+  return result;
+}
+
 } // namespace
 
 @implementation ODRElement {
   odr::Element _handle;
-  // The document the adapter behind `_handle` belongs to. `odr::Element` holds
-  // a bare pointer into it, so without this the tree could outlive what it
-  // points into — the analogue of the JNI bindings' owner chain.
+  // `odr::Element` holds a bare pointer into the document's adapter, so the
+  // tree has to keep the document alive itself.
   id _owner;
 }
 
@@ -212,15 +223,7 @@ - (nullable ODRElement *)nextSibling {
 - (NSArray<ODRElement *> *)children {
   return guarded_value(
       [&]() -> NSArray<ODRElement *> * {
-        NSMutableArray<ODRElement *> *const result = [NSMutableArray array];
-        for (odr::Element child = _handle.first_child(); child;
-             child = child.next_sibling()) {
-          ODRElement *const wrapped = [self derive:child];
-          if (wrapped != nil) {
-            [result addObject:wrapped];
-          }
-        }
-        return result;
+        return to_nsarray(self, _handle.children());
       },
       @[]);
 }
@@ -333,14 +336,7 @@ - (nullable ODRElement *)cellAtColumn:(uint32_t)column row:(uint32_t)row {
 - (NSArray<ODRElement *> *)shapes {
   return guarded_value(
       [&]() -> NSArray<ODRElement *> * {
-        NSMutableArray<ODRElement *> *const result = [NSMutableArray array];
-        for (const odr::Element shape : self.handle.as_sheet().shapes()) {
-          ODRElement *const wrapped = [self derive:shape];
-          if (wrapped != nil) {
-            [result addObject:wrapped];
-          }
-        }
-        return result;
+        return to_nsarray(self, self.handle.as_sheet().shapes());
       },
       @[]);
 }
@@ -578,29 +574,15 @@ - (nullable ODRElement *)firstColumn {
 - (NSArray<ODRElement *> *)columns {
   return guarded_value(
       [&]() -> NSArray<ODRElement *> * {
-        NSMutableArray<ODRElement *> *const result = [NSMutableArray array];
-        for (const odr::Element column : self.handle.as_table().columns()) {
-          ODRElement *const wrapped = [self derive:column];
-          if (wrapped != nil) {
-            [result addObject:wrapped];
-          }
-        }
-        return result;
+        return to_nsarray(self, self.handle.as_table().columns());
       },
       @[]);
 }
 
 - (NSArray<ODRElement *> *)rows {
   return guarded_value(
       [&]() -> NSArray<ODRElement *> * {
-        NSMutableArray<ODRElement *> *const result = [NSMutableArray array];
-        for (const odr::Element row : self.handle.as_table().rows()) {
-          ODRElement *const wrapped = [self derive:row];
-          if (wrapped != nil) {
-            [result addObject:wrapped];
-          }
-        }
-        return result;
+        return to_nsarray(self, self.handle.as_table().rows());
       },
       @[]);
 }

apple/src/ODRFile.mm

@@ -271,8 +271,9 @@ @implementation ODRDecodedFile {
 
 + (instancetype)decodedFileWithHandle:(odr::DecodedFile)handle {
   // The most derived wrapper the file qualifies for, so a caller never has to
-  // downcast something it already knows the type of. PDFs are document files
-  // too, so they have to be tested first.
+  // downcast something it already knows the type of. The predicates are
+  // mutually exclusive — a PDF is not an `is_document_file`, despite reporting
+  // `FileCategory::document`.
   Class klass = [ODRDecodedFile class];
   if (handle.is_pdf_file()) {
     klass = [ODRPdfFile class];

apple/src/ODRHtml.mm

@@ -306,9 +306,8 @@ + (instancetype)htmlWithHandle:(const odr::Html &)handle {
 
 @implementation ODRHtmlView {
   std::optional<odr::HtmlView> _handle;
-  // The service the view belongs to. The view's impl holds a bare pointer to
-  // it, so without this a view handed out by `-views` could outlive what it
-  // points into — the same owner chain `ODRElement` keeps to its document.
+  // The view's impl holds a bare pointer to its service, so the view has to
+  // keep the service alive itself — as `ODRElement` does its document.
   id _owner;
 }
 

apple/src/ODRInternal.h

@@ -36,25 +36,13 @@ NSData *to_nsdata(std::istream &stream);
 /// inside a `catch`.
 void fill_error(NSError *_Nullable *_Nullable error);
 
-/// Fills `*error` with `ODRErrorUnsupportedOperation` for an API area that is
-/// declared but not bound yet, and returns nil. Every use is a placeholder to
-/// delete, never a permanent answer.
-id _Nullable not_yet_bound(NSError *_Nullable *_Nullable error,
-                           const char *what);
-
 /// Reports an exception that could not be handed to the caller. Call only from
 /// inside a `catch`.
 void report_swallowed(const char *what);
 
 /// Runs `body` where the caller has no way to receive an error — an ObjC
-/// property, or a `void` method — and returns `fallback` if it throws.
-///
-/// This is not politeness. An exception crossing into Objective-C++ unhandled
-/// calls `std::terminate`, so an unguarded getter turns a malformed argument
-/// into a crash of the *host app*; `odr::Filesystem::exists("")` throwing
-/// `std::invalid_argument` is exactly how this was found. Almost nothing in
-/// odrcore's public API is `noexcept`, so assume any call can throw and pick a
-/// fallback that keeps the caller sane — `YES` for a walker's `end`, so a
+/// property, or a `void` method — and returns `fallback` if it throws. Pick a
+/// fallback that keeps the caller sane: `YES` for a walker's `end`, so a
 /// `while (!end)` loop terminates rather than spins.
 template <typename Body>
 auto guarded_value(Body &&body, std::invoke_result_t<Body> fallback)
@@ -82,11 +70,8 @@ template <typename Body> void guarded_void(Body &&body) {
 }
 
 /// Runs `body`, mapping any C++ exception onto `*error`. A failed call returns
-/// a value-initialised `Result` — `nil` for an object, `NO` for a `BOOL`, `0`
-/// for a count — which is exactly the ObjC convention for "consult the error".
-///
-/// Every binding body goes through this: an exception crossing into ObjC++
-/// unhandled would terminate the process.
+/// a value-initialised `Result` — `nil`, `NO`, `0` — which is the ObjC
+/// convention for "consult the error".
 template <typename Body>
 auto guarded(NSError *_Nullable *_Nullable error,
              Body &&body) -> decltype(body()) {

apple/src/ODRLogger.mm

@@ -25,26 +25,33 @@
 
 namespace {
 
+void report_sink_exception(NSException *const exception) {
+  // A logger must not derail the operation it is reporting on.
+  NSLog(@"odr: log sink threw %@: %@", exception.name, exception.reason);
+}
+
 /// Routes `odr::ILogger` into an ObjC sink, the analogue of `jni_logger.cpp`'s
-/// `JavaLogger`.
-///
-/// Holds the sink strongly: the C++ logger can outlive every ObjC reference the
-/// caller kept, and a sink collected out from under it would be a use after
-/// free on a background thread.
+/// `JavaLogger`. Holds the sink strongly: the C++ logger can outlive every ObjC
+/// reference the caller kept.
 class SinkLogger final : public odr::ILogger {
 public:
   explicit SinkLogger(id<ODRLogSink> sink) : m_sink{sink} {}
 
   [[nodiscard]] bool will_log(const odr::LogLevel level) const final {
     @autoreleasepool {
-      return [m_sink willLog:static_cast<ODRLogLevel>(level)] == YES;
+      @try {
+        return [m_sink willLog:static_cast<ODRLogLevel>(level)] == YES;
+      } @catch (NSException *const exception) {
+        report_sink_exception(exception);
+        return false;
+      }
     }
   }
 
   void log(const Time, const odr::LogLevel level, const std::string &message,
            const std::source_location &location) final {
-    // Log calls arrive on whatever thread the library works on, so each one
-    // gets its own pool rather than leaking into the caller's.
+    // Calls arrive on whatever thread the library works on, so each one gets
+    // its own pool rather than leaking into the caller's.
     @autoreleasepool {
       @try {
         [m_sink logLevel:static_cast<ODRLogLevel>(level)
@@ -53,8 +60,7 @@ void log(const Time, const odr::LogLevel level, const std::string &message,
                              std::string_view(location.file_name()))
                     line:location.line()];
       } @catch (NSException *const exception) {
-        // A logger must not derail the operation it is reporting on.
-        NSLog(@"odr: log sink threw %@: %@", exception.name, exception.reason);
+        report_sink_exception(exception);
       }
     }
   }
@@ -64,7 +70,7 @@ void flush() final {
       @try {
         [m_sink flush];
       } @catch (NSException *const exception) {
-        NSLog(@"odr: log sink threw %@: %@", exception.name, exception.reason);
+        report_sink_exception(exception);
       }
     }
   }

apple/src/ODRPrivate.h

@@ -19,14 +19,10 @@
 
 /// Cross-translation-unit access to the C++ value each wrapper owns.
 ///
-/// The handle model is much lighter than the JNI one (`jni/AGENTS.md`): an
-/// ObjC++ `@implementation` can hold the C++ handle as an ivar directly, and
-/// ARC's `.cxx_construct`/`.cxx_destruct` run its constructor and destructor.
-/// No `long` handles, no `destroy` natives, no reaper thread.
-///
-/// Nor is there a keep-alive chain to maintain for these: the public C++
-/// handles hold a `shared_ptr` to the implementation, so a wrapper owning one
-/// by value already keeps it alive on its own.
+/// Each `@implementation` holds its handle as an ivar, destroyed by ARC's
+/// `.cxx_destruct` — no `long` handles as in `jni/`. Most handles own a
+/// `shared_ptr`, so a wrapper holding one needs no keep-alive; the exceptions
+/// are `ODRElement` and `ODRHtmlView` below.
 ///
 /// Categories cannot add ivars, so each class declares its own accessors here
 /// and implements them next to its `@implementation`.

apple/src/ODRStyle.mm

@@ -7,6 +7,7 @@
 
 #include <optional>
 
+using odr::apple::guarded_value;
 using odr::apple::to_nsstring;
 
 ODR_SAME_ENUM(ODRFontWeightNormal, odr::FontWeight::normal);
@@ -59,14 +60,11 @@
 
 namespace {
 
-/// The boxed forms of an absent `std::optional`. `nil` and not a sentinel: a
-/// style that does not set a property is different from one that sets it to a
-/// default, and only the caller knows what to fall back to.
-NSNumber *_Nullable box_bool(const std::optional<bool> &value) {
-  return value.has_value() ? @(*value) : nil;
-}
-
-NSNumber *_Nullable box_double(const std::optional<double> &value) {
+/// An absent `std::optional` boxes as `nil` and not as a sentinel: a style that
+/// does not set a property is different from one that sets it to a default, and
+/// only the caller knows what to fall back to.
+template <typename T>
+NSNumber *_Nullable box_number(const std::optional<T> &value) {
   return value.has_value() ? @(*value) : nil;
 }
 
@@ -75,15 +73,10 @@
   return value.has_value() ? @(static_cast<NSInteger>(*value)) : nil;
 }
 
-NSString *_Nullable box_string(const std::optional<std::string> &value) {
-  return value.has_value() ? to_nsstring(*value) : nil;
-}
-
-/// `font_name` is a `string_view` borrowing from the document that produced the
-/// style, so it must be copied here — an `NSString` outliving that document is
-/// the whole point of handing it to a caller.
-NSString *_Nullable box_string_view(
-    const std::optional<std::string_view> &value) {
+/// 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) {
   return value.has_value() ? to_nsstring(*value) : nil;
 }
 
@@ -102,15 +95,16 @@ + (instancetype)measureWithHandle:(const odr::Measure &)handle {
 }
 
 - (double)magnitude {
-  return _handle->magnitude();
+  return guarded_value([&] { return _handle->magnitude(); }, 0.0);
 }
 
 - (NSString *)unit {
-  return to_nsstring(_handle->unit().name());
+  return guarded_value([&] { return to_nsstring(_handle->unit().name()); },
+                       @"");
 }
 
 - (NSString *)stringValue {
-  return to_nsstring(_handle->to_string());
+  return guarded_value([&] { return to_nsstring(_handle->to_string()); }, @"");
 }
 
 - (NSString *)description {
@@ -155,12 +149,12 @@ @implementation ODRTextStyle
 
 + (instancetype)styleWithHandle:(const odr::TextStyle &)handle {
   ODRTextStyle *const result = [[ODRTextStyle alloc] init];
-  result->_fontName = box_string_view(handle.font_name);
+  result->_fontName = box_string(handle.font_name);
   result->_fontSize = box(handle.font_size);
   result->_fontWeight = box_enum(handle.font_weight);
   result->_fontStyle = box_enum(handle.font_style);
-  result->_fontUnderline = box_bool(handle.font_underline);
-  result->_fontLineThrough = box_bool(handle.font_line_through);
+  result->_fontUnderline = box_number(handle.font_underline);
+  result->_fontLineThrough = box_number(handle.font_line_through);
   result->_fontShadow = box_string(handle.font_shadow);
   result->_fontColor = box(handle.font_color);
   result->_backgroundColor = box(handle.background_color);
@@ -223,7 +217,7 @@ + (instancetype)styleWithHandle:(const odr::TableCellStyle &)handle {
   result->_padding =
       [ODRDirectionalMeasure directionalWithHandle:handle.padding];
   result->_border = [ODRDirectionalString directionalWithHandle:handle.border];
-  result->_textRotation = box_double(handle.text_rotation);
+  result->_textRotation = box_number(handle.text_rotation);
   return result;
 }