feat!: make Logger a value type with a public ILogger sink

cli/src/back_translate.cpp

@@ -11,7 +11,7 @@ using namespace odr;
 
 int main(int, char **argv) {
   try {
-    const std::shared_ptr logger =
+    const Logger logger =
         Logger::create_stdio("odr-back-translate", LogLevel::verbose);
 
     const std::string input{argv[1]};
@@ -21,7 +21,7 @@ int main(int, char **argv) {
     const DocumentFile document_file{input};
 
     if (document_file.password_encrypted()) {
-      ODR_FATAL(*logger, "encrypted documents are not supported");
+      ODR_FATAL(logger, "encrypted documents are not supported");
       return 1;
     }
 

cli/src/meta.cpp

@@ -11,8 +11,7 @@ using namespace odr;
 
 int main(const int argc, char **argv) {
   try {
-    const std::shared_ptr logger =
-        Logger::create_stdio("odr-meta", LogLevel::verbose);
+    const Logger logger = Logger::create_stdio("odr-meta", LogLevel::verbose);
 
     const std::string input{argv[1]};
 
@@ -25,13 +24,13 @@ int main(const int argc, char **argv) {
 
     if (document_file.password_encrypted()) {
       if (!password) {
-        ODR_FATAL(*logger, "document encrypted but no password given");
+        ODR_FATAL(logger, "document encrypted but no password given");
         return 2;
       }
       try {
         document_file = document_file.decrypt(*password);
       } catch (const WrongPasswordError &) {
-        ODR_FATAL(*logger, "wrong password");
+        ODR_FATAL(logger, "wrong password");
         return 1;
       }
     }

cli/src/server.cpp

@@ -14,8 +14,7 @@ using namespace odr;
 
 int main(const int argc, char **argv) {
   try {
-    const std::shared_ptr logger =
-        Logger::create_stdio("odr-server", LogLevel::verbose);
+    const Logger logger = Logger::create_stdio("odr-server", LogLevel::verbose);
 
     std::string input{argv[1]};
 
@@ -27,17 +26,17 @@ int main(const int argc, char **argv) {
     DecodePreference decode_preference;
     decode_preference.as_file_type = FileType::zip;
 
-    DecodedFile decoded_file{input, decode_preference, *logger};
+    DecodedFile decoded_file{input, decode_preference, logger};
 
     if (decoded_file.password_encrypted()) {
       if (!password) {
-        ODR_FATAL(*logger, "document encrypted but no password given");
+        ODR_FATAL(logger, "document encrypted but no password given");
         return 2;
       }
       try {
         decoded_file = decoded_file.decrypt(*password);
       } catch (const WrongPasswordError &) {
-        ODR_FATAL(*logger, "wrong password");
+        ODR_FATAL(logger, "wrong password");
         return 1;
       }
     }
@@ -71,9 +70,9 @@ int main(const int argc, char **argv) {
           html::translate(decoded_file, prefix_cache_path, html_config, logger);
       server.connect_service(service, prefix);
       const HtmlViews views = service.list_views();
-      ODR_INFO(*logger, "hosted decoded file with id: " << prefix);
+      ODR_INFO(logger, "hosted decoded file with id: " << prefix);
       for (const auto &view : views) {
-        ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path());
+        ODR_INFO(logger, base_url << "/file/" << prefix << "/" << view.path());
       }
     }
 
@@ -90,9 +89,9 @@ int main(const int argc, char **argv) {
       const HtmlService filesystem_service =
           html::translate(filesystem, prefix_cache_path, html_config, logger);
       server.connect_service(filesystem_service, prefix);
-      ODR_INFO(*logger, "hosted filesystem with id: " << prefix);
+      ODR_INFO(logger, "hosted filesystem with id: " << prefix);
       for (const auto &view : filesystem_service.list_views()) {
-        ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path());
+        ODR_INFO(logger, base_url << "/file/" << prefix << "/" << view.path());
       }
     }
 

cli/src/translate.cpp

@@ -10,7 +10,7 @@ using namespace odr;
 
 int main(const int argc, char **argv) {
   try {
-    const std::shared_ptr logger =
+    const Logger logger =
         Logger::create_stdio("odr-translate", LogLevel::verbose);
 
     const std::string input{argv[1]};
@@ -25,13 +25,13 @@ int main(const int argc, char **argv) {
 
     if (decoded_file.password_encrypted()) {
       if (!password) {
-        ODR_FATAL(*logger, "document encrypted but no password given");
+        ODR_FATAL(logger, "document encrypted but no password given");
         return 2;
       }
       try {
         decoded_file = decoded_file.decrypt(*password);
       } catch (const WrongPasswordError &) {
-        ODR_FATAL(*logger, "wrong password");
+        ODR_FATAL(logger, "wrong password");
         return 1;
       }
     }

docs/design/README.md

@@ -36,8 +36,6 @@
 
 ### Breaking API changes (next major)
 
-- `Logger` should be a value type, like the other interface types.
-  Eases lifetime management; consistent with value semantics for the user-facing API.
 - Collapse `FileMeta` into a single type. The nested `DocumentMeta`
   does not earn the extra indirection.
 

jni/AGENTS.md

@@ -37,8 +37,15 @@ package `app.opendocument.core`. Mirrors the surface of the python bindings
   never JNI's modified-UTF-8 `GetStringUTFChars`.
 - **Exceptions**: every native body runs inside `odr_jni::guarded`; C++
   exceptions map to `OdrException` subclasses (`odr_jni.cpp::throw_java`).
-- Mirror the C++ names; drop the `Logger` parameters (bindings use the default
-  null logger).
+- Mirror the C++ names. `Logger` is bound as a `NativeResource`; entry points
+  that take one get an overload (e.g. `Odr.open(path, logger)`).
+- `ILogger` is implementable in Java. `jni_logger.cpp`'s `JavaLogger` holds a
+  global ref to the sink and routes calls through the package-private
+  `LoggerBridge` statics, so only three method handles need caching. Log calls
+  arrive on whatever thread the library works on, so it attaches via `ScopedEnv`
+  and detaches again; an exception thrown by the sink is described and cleared
+  rather than left pending, since a logger must not derail the operation it
+  reports on.
 - Stream-based C++ APIs (`write`, `save`, `pipe`) are bound as natives
   returning `byte[]`/`String` via `std::ostringstream`.
 - **Not bound**: `HtmlConfig::resource_locator` (function pointer across JNI);

jni/CMakeLists.txt

@@ -30,6 +30,7 @@ add_library(odr_jni SHARED
         "src/jni_document.cpp"
         "src/jni_file.cpp"
         "src/jni_html.cpp"
+        "src/jni_logger.cpp"
         "src/jni_http_server.cpp"
         "src/jni_style.cpp"
 )
@@ -74,6 +75,11 @@ add_jar(odr_java
         "java/app/opendocument/core/FontPosition.java"
         "java/app/opendocument/core/FontStyle.java"
         "java/app/opendocument/core/FontWeight.java"
+        "java/app/opendocument/core/ILogger.java"
+        "java/app/opendocument/core/LogLevel.java"
+        "java/app/opendocument/core/Logger.java"
+        "java/app/opendocument/core/LoggerBridge.java"
+        "java/app/opendocument/core/SourceLocation.java"
         "java/app/opendocument/core/Frame.java"
         "java/app/opendocument/core/GlobalParams.java"
         "java/app/opendocument/core/GraphicStyle.java"
@@ -152,6 +158,7 @@ if (ODR_TEST)
             "tests/app/opendocument/core/FileTest.java"
             "tests/app/opendocument/core/HtmlTest.java"
             "tests/app/opendocument/core/HttpServerTest.java"
+            "tests/app/opendocument/core/LoggerTest.java"
             "tests/app/opendocument/core/MetaTest.java"
             "tests/app/opendocument/core/TestFiles.java"
             INCLUDE_JARS odr_java "${ODR_JNI_JUNIT_JAR}"

jni/java/app/opendocument/core/ILogger.java

@@ -0,0 +1,22 @@
+package app.opendocument.core;
+
+/**
+ * A log sink. Implement this and wrap it in a {@link Logger} to route odr's
+ * diagnostics. Mirrors {@code odr::ILogger}.
+ *
+ * <p>{@link Logger} gates on {@link #willLog} before calling {@link #log}, so
+ * an implementation may assume the level reaching {@code log} is enabled.
+ *
+ * <p>Methods may be invoked from native worker threads, so implementations must
+ * be thread-safe.
+ */
+public interface ILogger {
+  boolean willLog(LogLevel level);
+
+  /**
+   * @param epochMillis time of the message, in milliseconds since the epoch
+   */
+  void log(long epochMillis, LogLevel level, String message, SourceLocation location);
+
+  void flush();
+}

jni/java/app/opendocument/core/LogLevel.java

@@ -0,0 +1,14 @@
+package app.opendocument.core;
+
+/** Mirrors {@code odr::LogLevel}; constant order must match the C++ declaration. */
+public enum LogLevel {
+  VERBOSE, DEBUG, INFO, WARNING, ERROR, FATAL;
+
+  static LogLevel fromNative(int code) {
+    return code < 0 ? null : values()[code];
+  }
+
+  int toNative() {
+    return ordinal();
+  }
+}

jni/java/app/opendocument/core/Logger.java

@@ -0,0 +1,58 @@
+package app.opendocument.core;
+
+/**
+ * Handle to a log sink. Mirrors {@code odr::Logger}; copies share the sink.
+ *
+ * <p>Pass one to {@link Odr#open} (and friends) to receive the library's
+ * diagnostics. Wrap your own {@link ILogger} to route them anywhere.
+ */
+public final class Logger extends NativeResource {
+  static {
+    NativeLibrary.load();
+  }
+
+  Logger(long handle) {
+    super(handle, null, Logger::destroy);
+  }
+
+  /** A logger that discards everything. */
+  public static Logger nullLogger() {
+    return new Logger(createNull());
+  }
+
+  /** Writes to standard output. */
+  public static Logger stdio(String name, LogLevel level) {
+    return new Logger(createStdio(name, level.toNative()));
+  }
+
+  /** Routes to a sink implemented in Java. */
+  public Logger(ILogger sink) {
+    this(createFromSink(sink));
+  }
+
+  public boolean willLog(LogLevel level) {
+    return willLogNative(handle(), level.toNative());
+  }
+
+  public void log(LogLevel level, String message) {
+    logNative(handle(), level.toNative(), message);
+  }
+
+  public void flush() {
+    flushNative(handle());
+  }
+
+  private static native long createNull();
+
+  private static native long createStdio(String name, int level);
+
+  private static native long createFromSink(ILogger sink);
+
+  private static native boolean willLogNative(long handle, int level);
+
+  private static native void logNative(long handle, int level, String message);
+
+  private static native void flushNative(long handle);
+
+  private static native void destroy(long handle);
+}

jni/java/app/opendocument/core/LoggerBridge.java

@@ -0,0 +1,33 @@
+package app.opendocument.core;
+
+/**
+ * Entry points the native side calls to reach an {@link ILogger}. Keeping the
+ * enum and {@link SourceLocation} construction on this side means the native
+ * bridge only has to cache three static method handles.
+ */
+final class LoggerBridge {
+  private LoggerBridge() {}
+
+  static boolean willLog(ILogger sink, int level) {
+    return sink.willLog(LogLevel.fromNative(level));
+  }
+
+  static void log(
+      ILogger sink,
+      long epochMillis,
+      int level,
+      String message,
+      String fileName,
+      String functionName,
+      int line) {
+    sink.log(
+        epochMillis,
+        LogLevel.fromNative(level),
+        message,
+        new SourceLocation(fileName, functionName, line));
+  }
+
+  static void flush(ILogger sink) {
+    sink.flush();
+  }
+}

jni/java/app/opendocument/core/Odr.java

@@ -91,6 +91,11 @@ public static DecodedFile open(String path) {
     return new DecodedFile(openNative(path));
   }
 
+  /** Opens and decodes a file, reporting diagnostics to {@code logger}. */
+  public static DecodedFile open(String path, Logger logger) {
+    return new DecodedFile(openWithLoggerNative(path, logger.handle()));
+  }
+
   /** Opens and decodes a file as the given file type. */
   public static DecodedFile open(String path, FileType as) {
     return new DecodedFile(openAsNative(path, as.toNative()));
@@ -133,6 +138,8 @@ public static DecodedFile open(String path, DecodePreference preference) {
 
   private static native long openNative(String path);
 
+  private static native long openWithLoggerNative(String path, long logger);
+
   private static native long openAsNative(String path, int as);
 
   private static native long openWithPreferenceNative(

jni/java/app/opendocument/core/SourceLocation.java

@@ -0,0 +1,19 @@
+package app.opendocument.core;
+
+/** Call site a log message originated from. Mirrors {@code std::source_location}. */
+public final class SourceLocation {
+  public final String fileName;
+  public final String functionName;
+  public final int line;
+
+  SourceLocation(String fileName, String functionName, int line) {
+    this.fileName = fileName;
+    this.functionName = functionName;
+    this.line = line;
+  }
+
+  @Override
+  public String toString() {
+    return fileName + ":" + line;
+  }
+}

jni/src/jni_core.cpp

@@ -2,6 +2,7 @@
 
 #include <odr/file.hpp>
 #include <odr/global_params.hpp>
+#include <odr/logger.hpp>
 #include <odr/odr.hpp>
 #include <odr/table_position.hpp>
 
@@ -183,6 +184,16 @@ Java_app_opendocument_core_Odr_openNative(JNIEnv *env, jclass, jstring path) {
                  [&] { return make_handle(odr::open(to_string(env, path))); });
 }
 
+extern "C" JNIEXPORT jlong JNICALL
+Java_app_opendocument_core_Odr_openWithLoggerNative(JNIEnv *env, jclass,
+                                                    jstring path,
+                                                    jlong logger) {
+  return guarded(env, [&] {
+    return make_handle(
+        odr::open(to_string(env, path), *from_handle<odr::Logger>(logger)));
+  });
+}
+
 extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_Odr_openAsNative(
     JNIEnv *env, jclass, jstring path, jint as) {
   return guarded(env, [&] {