feat: add subtitle field to XMP metadata parser, refactor for modern Java#70
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (42)
📝 WalkthroughWalkthroughIntroduces version 1.2.0 with Windows 2025 runner support, enhanced host/platform detection in Gradle, C++23 shim compilation with Windows-specific headers, new QPDF-backed XMP/metadata extraction functions, systematic FFM binding refactoring from volatile+synchronized to StableValue caching, configurable XMP padding, improved metadata caching in PdfDocument, and refactored ScratchBuffer with ScopedValue-based lifecycle management. ChangesCI/Build & Platform Infrastructure
PDFium Shim XMP/Metadata Extraction APIs
FFM Binding Cache Modernization
Java Metadata APIs & PDF Save Flow
Internal Infrastructure Refactoring
Sequence Diagram(s)sequenceDiagram
participant Java as PdfDocument
participant NativeInit as PdfiumLibrary.initialize()
participant Shim as Native Shim
participant QPDF as QPDF Library
Java->>NativeInit: PdfiumLibrary.initialize()
NativeInit->>Shim: Resolve required symbols
Shim-->>NativeInit: Required symbols ready
NativeInit->>Shim: pdfium4jResolveOptionalSymbols()
Shim->>Shim: One-time resolve FPDF_GetXMPMetadata
Shim-->>NativeInit: Optional symbols resolved
NativeInit-->>Java: Initialization complete
Note over Java: PDF operations enabled
Java->>Java: bookMetadata() [synchronized]
Java->>Shim: pdfium4jGetXmpQpdf(path)
Shim->>QPDF: Extract /Metadata stream
QPDF-->>Shim: XMP bytes
Shim-->>Java: XMP document
Java->>Java: Parse XMP once, cache in cachedXmp
Java-->>Java: Return PdfBookMetadata with title/subtitle
sequenceDiagram
participant Client as PdfSaver.save()
participant Java as saveNative()
participant Shim as Native pdfium4j_save_*
participant File as File System
Client->>Java: save(SaveParams with source, dest, xmp, metadata)
Java->>Java: Determine source type (path/bytes/segment)
alt Source is file path
Java->>Shim: pdfium4j_save_with_metadata_native(srcPath, dstPath, xmp, metadata)
else Source is byte array
Java->>Shim: pdfium4j_save_with_metadata_mem_native(srcBytes, dstPath, xmp, metadata)
else Source is MemorySegment
Java->>Shim: pdfium4j_save_with_metadata_mem_to_file_native(srcSegment, dstPath, xmp, metadata)
end
Shim->>Shim: Inject XMP /Metadata
Shim->>Shim: Inject Info dictionary key/value pairs
Shim->>File: Write PDF file with updated metadata
File-->>Shim: Write complete
Shim-->>Java: Return status
Java-->>Client: Complete or throw IOException
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java (1)
363-406:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove
SegmentInputStreamand itsacquire()/release()calls.
SegmentInputStream(lines 363–406) is dead code. The public factorywrap(MemorySegment, long)that created instances was removed, and no remaining code references this class. TheSegmentInputStreaminstantiation found atPdfDocument.java:1024uses a different class (PdfDocument.SegmentInputStream) with a different constructor signature. Delete the unused inner class and its resource plumbing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java` around lines 363 - 406, Remove the unused inner class SegmentInputStream and its resource plumbing: delete the entire private static final class SegmentInputStream (including its constructor, read/available/close methods) and remove the calls to acquire() in the constructor and release() in close(); this class was orphaned after the public factory wrap(MemorySegment,long) was removed and is distinct from PdfDocument.SegmentInputStream, so simply deleting SegmentInputStream and its acquire()/release() invocations is the correct cleanup.src/main/java/org/grimmory/pdfium4j/PdfDocument.java (1)
922-957:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSubtitle is hard-coded to
Optional.empty()despite PR adding subtitle parsing.The PR adds subtitle parsing to
XmpMetadataParser(stored asbooklore:subtitlein custom fields), andPdfBookMetadatanow has asubtitlefield (position 2), but this constructor call passesOptional.empty()instead of extracting the parsed subtitle. The newly parsed subtitle is never surfaced to the caller, nullifying the user-visible feature.The subtitle is accessible via
XmpMetadata.findField("subtitle"), which searches custom namespaces and returnsOptional<String>:xmp.findField("subtitle")Alternatively, if a fallback to Info dictionary is desired (consistent with title/description handling):
xmp.findField("subtitle").or(() -> metadata(MetadataTag.SUBTITLE))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/grimmory/pdfium4j/PdfDocument.java` around lines 922 - 957, The constructor call creating PdfBookMetadata currently passes a hard-coded Optional.empty() for the subtitle (position 2) so the parsed subtitle from XmpMetadata is never returned; update that argument to use the parsed subtitle from XmpMetadata by replacing Optional.empty() with xmp.findField("subtitle") or, if you want the same fallback pattern used elsewhere, xmp.findField("subtitle").or(() -> metadata(MetadataTag.SUBTITLE)); locate the return in PdfDocument where PdfBookMetadata(...) is constructed and modify that second parameter accordingly (use XmpMetadata.findField and the metadata(MetadataTag.SUBTITLE) helper).
🧹 Nitpick comments (5)
src/main/java/org/grimmory/pdfium4j/internal/FfmHelper.java (1)
47-51: ⚡ Quick win
C_LONG_VStableValueis redundant — the fallback is evaluated eagerly.
Map.getOrDefault(key, default)always evaluates its second argument, soC_LONG_V.orElseSet(FfmHelper::detectCLongLayout)is invoked unconditionally on every class-init, regardless of whether"long"is present in the canonical layouts. Combined with the fact thatC_LONGis itself astatic finalinitialized exactly once, wrappingdetectCLongLayout()in aStableValueprovides no laziness or caching benefit here — it only adds indirection.If the intent was to call
detectCLongLayout()only when the canonical layouts don't supply"long", switch to a conditional lookup. Otherwise, just inline the detection call and dropC_LONG_Ventirely.♻️ Proposed simplification (truly lazy fallback)
- private static final StableValue<ValueLayout> C_LONG_V = StableValue.of(); - - public static final ValueLayout C_LONG = - (ValueLayout) - layouts().getOrDefault("long", C_LONG_V.orElseSet(FfmHelper::detectCLongLayout)); + public static final ValueLayout C_LONG = resolveCLong(); + + private static ValueLayout resolveCLong() { + MemoryLayout fromLinker = layouts().get("long"); + return fromLinker instanceof ValueLayout vl ? vl : detectCLongLayout(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/grimmory/pdfium4j/internal/FfmHelper.java` around lines 47 - 51, C_LONG_V is unnecessary because layouts().getOrDefault(...) eagerly evaluates the fallback; change the static init for C_LONG to avoid calling detectCLongLayout() unconditionally: perform a conditional lookup (e.g., check layouts().containsKey("long") and use layouts().get("long") if present, otherwise call FfmHelper::detectCLongLayout) and remove the C_LONG_V StableValue field; reference C_LONG, C_LONG_V, detectCLongLayout, and layouts() when making the change.src/test/java/org/grimmory/pdfium4j/internal/ScratchBufferTest.java (1)
209-218: ⚡ Quick winAdd coverage for
withScratch(Runnable)and exception propagation.The new
callWithScratchhappy path is covered, but the siblingwithScratch(Runnable)API is entirely untested, and there is no assertion that exceptions thrown inside the action propagate correctly (and that the State is still cleaned up). Both are cheap to add and exercise the most failure-prone parts of the new lifecycle.🧪 Suggested additions
`@Test` void callWithScratchReturnsResult() throws Exception { String result = ScratchBuffer.callWithScratch( () -> { ScratchBuffer.get(8); return "success"; }); assertEquals("success", result); } + + `@Test` + void withScratchRunsActionWithImplicitAcquire() { + AtomicLong captured = new AtomicLong(); + ScratchBuffer.withScratch(() -> captured.set(ScratchBuffer.get(8).byteSize())); + assertTrue(captured.get() >= 8); + } + + `@Test` + void callWithScratchPropagatesExceptions() { + assertThrows( + IllegalStateException.class, + () -> + ScratchBuffer.callWithScratch( + () -> { + throw new IllegalStateException("boom"); + })); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/grimmory/pdfium4j/internal/ScratchBufferTest.java` around lines 209 - 218, Add unit tests that cover the void API and exception propagation: add a test that calls ScratchBuffer.withScratch(() -> { ScratchBuffer.get(8); /* no return */ }) and asserts no exception and that State is cleaned up (e.g., ScratchBuffer.get(...) no longer returns a retained state or ScratchBuffer.isActive() false after call); and add a test that calls ScratchBuffer.withScratch(() -> { throw new RuntimeException("boom"); }) (or use callWithScratch variant) and assert the RuntimeException propagates out and that the ScratchBuffer State is still cleaned up afterwards (no lingering active state). Reference the methods withScratch(Runnable), callWithScratch(Callable), and ScratchBuffer.get(...) to locate where to add these assertions.src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java (3)
158-184: 💤 Low valueDocument the implicit acquire on
withScratch/callWithScratch.Both helpers pre-set
useCount = 1so the action can useget(...)immediately. That means an unbalancedrelease()(or extraScope.close()) inside the action will dropuseCountto 0 and surprise the nextget(...)withIllegalStateException. A short note in the JavaDoc clarifying that callers should not callrelease()/ useacquireScope()symmetrically would prevent surprises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java` around lines 158 - 184, Update the JavaDoc for withScratch and callWithScratch to document that they implicitly set State.useCount = 1 (i.e., they acquire a scratch for the duration of the action), and therefore callers must not call release(), acquireScope(), or close the surrounding Scope (e.g., Scope.close()) inside the provided action or expect to symmetrically release—doing so may drop useCount to 0 and cause get(...) to throw IllegalStateException; reference withScratch, callWithScratch, State.useCount, get(...), release(), acquireScope(), and Scope.close() in the comment so callers know the exact behavior to avoid.
12-23: ⚡ Quick winClass JavaDoc no longer matches the implementation.
The header still describes a "thread-local scratch buffer" with a "reusable thread-local slab", but the new design centers on
ScopedValuewithThreadLocalonly as a bridge fallback, and introduceswithScratch/callWithScratchas preferred entry points. Please refresh the doc to describe the scope-based lifecycle and the bridge semantics so callers can pick the right entry point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java` around lines 12 - 23, The ScratchBuffer JavaDoc is outdated: update it to explain the new scope-based lifecycle using ScopedValue with ThreadLocal only as a bridge fallback, and document the preferred entry points withScratch and callWithScratch (which establish the scratch scope) instead of implying a purely thread-local reusable slab; state that returned segments are short-lived temporaries valid only for the duration of the scope or until release(), describe that growth is non-destructive across nested scopes, and note ThreadLocal is used only when ScopedValue is unavailable so callers can choose the correct entry point.
176-184: 💤 Low valueMinor: simplify the Callable wrapping and import
Callabledirectly.
() -> action.call()is a redundant lambda wrapper around aCallable; passing the method reference is cleaner. Also addimport java.util.concurrent.Callable;instead of using the fully-qualified type in the signature.♻️ Suggested tweak
- public static <T> T callWithScratch(java.util.concurrent.Callable<T> action) throws Exception { + public static <T> T callWithScratch(Callable<T> action) throws Exception { State s = new State(); s.useCount = 1; try { - return ScopedValue.where(STATE, s).call(() -> action.call()); + return ScopedValue.where(STATE, s).call(action::call); } finally { s.close(); } }The method reference
action::callis fully type-safe withScopedValue.Carrier.call(CallableOp op)in Java 25, where theCallableOpthrows clause is inferred asException.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java` around lines 176 - 184, callWithScratch currently uses a redundant lambda (() -> action.call()) and a fully-qualified Callable type; simplify by importing java.util.concurrent.Callable and passing the method reference action::call to ScopedValue.where(STATE, s).call(...). Locate the callWithScratch method and replace the parameter type with Callable (after adding the import) and change the lambda to use action::call, leaving State s creation and s.close() in the finally unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shim/CMakeLists.txt`:
- Around line 2-5: Update the CMake minimum required version to match the C++23
standard: change the cmake_minimum_required(...) value from 3.15 to 3.20 so it
supports set(CMAKE_CXX_STANDARD 23) and the CMAKE_CXX_STANDARD_REQUIRED ON
behavior; locate the cmake_minimum_required call and bump its version to 3.20.
In `@src/main/java/org/grimmory/pdfium4j/internal/NativeLoader.java`:
- Around line 49-60: The lambda passed to loaded.orElseSet in NativeLoader
swallows original exceptions when rethrowing; update the two catch blocks inside
the lambda (the one catching NativeLoadException and the one catching Throwable
around performLoad()) to preserve the original exception as the cause when
throwing a new NativeLoadException (e.g., throw new NativeLoadException("...",
caughtException) or call initCause on the new exception). This ensures
performLoad() failures and detectPlatform() details remain available in the
thrown NativeLoadException's stack trace.
In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java`:
- Around line 186-199: The purge() implementation must not zombify a currently
bound scoped State: do not close or zero a State while STATE.isBound() is true.
Change purge() to check STATE.isBound() and refuse (throw IllegalStateException
with a clear message) instead of mutating the bound State; leave BRIDGE handling
for non-scoped States as-is. Reference symbols: purge(), STATE, BRIDGE, State,
useCount, close(), and the fields segment/steadySegment/metadataBuffer.
In `@src/main/java/org/grimmory/pdfium4j/internal/TrigramTokenizer.java`:
- Around line 20-27: The current TrigramTokenizer implementation builds a boxed
stream and uses Gatherers.windowSliding which causes heavy allocation; replace
that pipeline with a primitive, low-allocation loop inside the same method in
TrigramTokenizer: convert the input String to a char[] or use text.charAt(i) in
a for-loop, compute each trigram long via ((long) c0 << 32) | ((long) c1 << 16)
| c2, store into a primitive long[] (or an Int-to-long deduplicating routine
using primitive buffers), then sort and deduplicate the primitive array before
returning; ensure you eliminate Stream.boxed()/Gatherers and use only primitive
arrays and loops to minimize allocations while preserving the distinct/sorted
semantics.
In `@src/main/java/org/grimmory/pdfium4j/model/XmpMetadata.java`:
- Around line 100-109: The method calibreSeriesIndex() was changed from
returning OptionalDouble to Optional<Double>, which is an API-breaking change;
update the release notes and migration guide to document this change, mention
the exact method name calibreSeriesIndex() and the previous return type
OptionalDouble plus the new return type Optional<Double>, describe how callers
should adapt (unwrap Optional<Double> or use Double::isPresent and get()), and
include the version (1.2.0) and a short example migration snippet so integrators
can update their code.
In `@src/main/java/org/grimmory/pdfium4j/PdfiumLibrary.java`:
- Around line 74-76: Wrap the optional symbol invocation
ShimBindings.pdfium4jResolveOptionalSymbols().invokeExact() in a try-catch so
that exceptions from the optional resolution are caught, logged, and do not
abort library initialization; specifically, call
ShimBindings.pdfium4jResolveOptionalSymbols(), if non-null attempt invokeExact()
inside a catch (Throwable) that logs the error (using the existing logger used
by PdfiumLibrary or a suitable logger) and continues without rethrowing so the
rest of initialization proceeds.
In `@src/main/java/org/grimmory/pdfium4j/PdfPage.java`:
- Line 353: The calculation of requiredBytes in PdfPage may overflow long for
extreme w/h, bypassing the memory guard; replace the simple cast multiplication
with overflow-checked math using Math.multiplyExact: compute requiredBytes =
Math.multiplyExact(Math.multiplyExact((long) w, (long) h), (long)
BYTES_PER_PIXEL) and catch ArithmeticException (or otherwise handle overflow) to
treat it as an overflow condition (e.g., set requiredBytes to Long.MAX_VALUE or
fail the allocation) so the max-memory check cannot be bypassed.
In `@src/test/java/org/grimmory/pdfium4j/NativeSmokeTest.java`:
- Line 4: The main method in NativeSmokeTest is declared without public
visibility so the Java launcher can't use it; change the method signature of
NativeSmokeTest.main to be public static void main(String[] args) (i.e., restore
public visibility on the main method) so the class can be executed as a standard
Java application entrypoint.
---
Outside diff comments:
In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java`:
- Around line 363-406: Remove the unused inner class SegmentInputStream and its
resource plumbing: delete the entire private static final class
SegmentInputStream (including its constructor, read/available/close methods) and
remove the calls to acquire() in the constructor and release() in close(); this
class was orphaned after the public factory wrap(MemorySegment,long) was removed
and is distinct from PdfDocument.SegmentInputStream, so simply deleting
SegmentInputStream and its acquire()/release() invocations is the correct
cleanup.
In `@src/main/java/org/grimmory/pdfium4j/PdfDocument.java`:
- Around line 922-957: The constructor call creating PdfBookMetadata currently
passes a hard-coded Optional.empty() for the subtitle (position 2) so the parsed
subtitle from XmpMetadata is never returned; update that argument to use the
parsed subtitle from XmpMetadata by replacing Optional.empty() with
xmp.findField("subtitle") or, if you want the same fallback pattern used
elsewhere, xmp.findField("subtitle").or(() -> metadata(MetadataTag.SUBTITLE));
locate the return in PdfDocument where PdfBookMetadata(...) is constructed and
modify that second parameter accordingly (use XmpMetadata.findField and the
metadata(MetadataTag.SUBTITLE) helper).
---
Nitpick comments:
In `@src/main/java/org/grimmory/pdfium4j/internal/FfmHelper.java`:
- Around line 47-51: C_LONG_V is unnecessary because layouts().getOrDefault(...)
eagerly evaluates the fallback; change the static init for C_LONG to avoid
calling detectCLongLayout() unconditionally: perform a conditional lookup (e.g.,
check layouts().containsKey("long") and use layouts().get("long") if present,
otherwise call FfmHelper::detectCLongLayout) and remove the C_LONG_V StableValue
field; reference C_LONG, C_LONG_V, detectCLongLayout, and layouts() when making
the change.
In `@src/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.java`:
- Around line 158-184: Update the JavaDoc for withScratch and callWithScratch to
document that they implicitly set State.useCount = 1 (i.e., they acquire a
scratch for the duration of the action), and therefore callers must not call
release(), acquireScope(), or close the surrounding Scope (e.g., Scope.close())
inside the provided action or expect to symmetrically release—doing so may drop
useCount to 0 and cause get(...) to throw IllegalStateException; reference
withScratch, callWithScratch, State.useCount, get(...), release(),
acquireScope(), and Scope.close() in the comment so callers know the exact
behavior to avoid.
- Around line 12-23: The ScratchBuffer JavaDoc is outdated: update it to explain
the new scope-based lifecycle using ScopedValue with ThreadLocal only as a
bridge fallback, and document the preferred entry points withScratch and
callWithScratch (which establish the scratch scope) instead of implying a purely
thread-local reusable slab; state that returned segments are short-lived
temporaries valid only for the duration of the scope or until release(),
describe that growth is non-destructive across nested scopes, and note
ThreadLocal is used only when ScopedValue is unavailable so callers can choose
the correct entry point.
- Around line 176-184: callWithScratch currently uses a redundant lambda (() ->
action.call()) and a fully-qualified Callable type; simplify by importing
java.util.concurrent.Callable and passing the method reference action::call to
ScopedValue.where(STATE, s).call(...). Locate the callWithScratch method and
replace the parameter type with Callable (after adding the import) and change
the lambda to use action::call, leaving State s creation and s.close() in the
finally unchanged.
In `@src/test/java/org/grimmory/pdfium4j/internal/ScratchBufferTest.java`:
- Around line 209-218: Add unit tests that cover the void API and exception
propagation: add a test that calls ScratchBuffer.withScratch(() -> {
ScratchBuffer.get(8); /* no return */ }) and asserts no exception and that State
is cleaned up (e.g., ScratchBuffer.get(...) no longer returns a retained state
or ScratchBuffer.isActive() false after call); and add a test that calls
ScratchBuffer.withScratch(() -> { throw new RuntimeException("boom"); }) (or use
callWithScratch variant) and assert the RuntimeException propagates out and that
the ScratchBuffer State is still cleaned up afterwards (no lingering active
state). Reference the methods withScratch(Runnable), callWithScratch(Callable),
and ScratchBuffer.get(...) to locate where to add these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d755be9-d342-43fe-9216-b98feb746d45
⛔ Files ignored due to path filters (8)
pdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libbase_allocator_partition_allocator_src_partition_alloc_allocator_base.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libbase_allocator_partition_allocator_src_partition_alloc_allocator_core.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libbase_allocator_partition_allocator_src_partition_alloc_allocator_shim.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libbase_allocator_partition_allocator_src_partition_alloc_raw_ptr.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libchrome_zlib.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libicuuc.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libpdfium.sois excluded by!**/*.sopdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/libthird_party_abseil-cpp_absl.sois excluded by!**/*.so
📒 Files selected for processing (42)
.github/workflows/ci.yml.github/workflows/publish.ymlbuild.gradle.ktspdfium4j-natives-linux-x64/build.gradle.ktspdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/native-libs.txtshim/CMakeLists.txtshim/include/pdfium4j_shim.hshim/src/pdfium4j_read.cppshim/src/pdfium4j_utils.hshim/src/pdfium4j_write.cppsrc/main/java/org/grimmory/pdfium4j/PdfDocument.javasrc/main/java/org/grimmory/pdfium4j/PdfPage.javasrc/main/java/org/grimmory/pdfium4j/PdfSaver.javasrc/main/java/org/grimmory/pdfium4j/PdfiumLibrary.javasrc/main/java/org/grimmory/pdfium4j/XmpMetadataParser.javasrc/main/java/org/grimmory/pdfium4j/XmpMetadataWriter.javasrc/main/java/org/grimmory/pdfium4j/internal/AnnotBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/AttachmentBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/BitmapBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/DocBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/EditBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/FfmHelper.javasrc/main/java/org/grimmory/pdfium4j/internal/Generators.javasrc/main/java/org/grimmory/pdfium4j/internal/InternalLogger.javasrc/main/java/org/grimmory/pdfium4j/internal/NativeLoader.javasrc/main/java/org/grimmory/pdfium4j/internal/PdfDocumentFallbackMeta.javasrc/main/java/org/grimmory/pdfium4j/internal/ScratchBuffer.javasrc/main/java/org/grimmory/pdfium4j/internal/SegmentOutputStream.javasrc/main/java/org/grimmory/pdfium4j/internal/ShimBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/TextBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/ThumbnailBindings.javasrc/main/java/org/grimmory/pdfium4j/internal/TrigramTokenizer.javasrc/main/java/org/grimmory/pdfium4j/internal/ViewBindings.javasrc/main/java/org/grimmory/pdfium4j/model/BookMetadata.javasrc/main/java/org/grimmory/pdfium4j/model/PdfBookMetadata.javasrc/main/java/org/grimmory/pdfium4j/model/XmpMetadata.javasrc/test/java/org/grimmory/pdfium4j/CorpusMetadataRoundTripTest.javasrc/test/java/org/grimmory/pdfium4j/NativeSmokeTest.javasrc/test/java/org/grimmory/pdfium4j/PdfDocumentTest.javasrc/test/java/org/grimmory/pdfium4j/XmpMetadataParserTest.javasrc/test/java/org/grimmory/pdfium4j/internal/ScratchBufferTest.javasrc/test/java/org/grimmory/pdfium4j/internal/TrigramTokenizerAllocationTest.java
💤 Files with no reviewable changes (2)
- pdfium4j-natives-linux-x64/src/main/resources/natives/linux-x64/native-libs.txt
- pdfium4j-natives-linux-x64/build.gradle.kts
📜 Review details
🧰 Additional context used
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs [::grimmory-tools/grimmory-docs::]
-
docs/magic-shelf.md
- Line ~60: documents a "Subtitle" metadata field (
subtitle) described as "Book subtitle". - Line ~205: includes
subtitlein metadata presence dropdown list. - Lines ~1256–1263: mentions filtering/books that have subtitle metadata and example filter
{ "field": "subtitle", "operator": "is_not_empty", "value": null }.
- Line ~60: documents a "Subtitle" metadata field (
-
docs/metadata/file-naming-patterns.md
- Line ~32 / ~80 / ~87 / ~90 / ~129 / ~136: multiple examples and pattern entries referencing
{subtitle}and examples showing how subtitles appear/are omitted in filenames.
- Line ~32 / ~80 / ~87 / ~90 / ~129 / ~136: multiple examples and pattern entries referencing
-
docs/metadata/sidecar-files.md
- Line ~37: lists subtitle among sidecar metadata fields (Title, subtitle, ...).
-
docs/metadata/metadata-settings.md
- Line ~61: lists subtitle in "Standard fields".
-
docs/metadata/metadata-fetch-configuration.md
- Line ~63: documents "Subtitle | Book subtitle".
Relevance: the repo contains user-facing documentation that references a "subtitle" metadata field. The PR adds subtitle handling in the PDFium4j XMP metadata model, so these docs align with that field; no code-level consumers were found in this repository.
🔇 Additional comments (61)
src/main/java/org/grimmory/pdfium4j/internal/SegmentOutputStream.java (1)
27-31: LGTM!src/main/java/org/grimmory/pdfium4j/PdfPage.java (1)
62-63: LGTM!Also applies to: 151-151
src/main/java/org/grimmory/pdfium4j/internal/FfmHelper.java (1)
30-30: ⚡ Quick winThe
--enable-previewconfiguration is properly wired up for both compilation and runtime.The build configuration correctly enables preview features:
build.gradle.ktsincludes--enable-previewin theJavaCompiletask (lines 117) and across all JVM execution contexts (benchmarks, tests, Javadoc). Compiler warnings are suppressed globally via-Xlint:-preview(line 118).One note: unlike similar binding classes (
ShimBindings,ThumbnailBindings,Generators,AttachmentBindings),FfmHelperlacks the@SuppressWarnings("preview")annotation at the class level, though the build-level suppression covers this and prevents compilation warnings.src/test/java/org/grimmory/pdfium4j/internal/ScratchBufferTest.java (1)
87-96: LGTM!Also applies to: 100-104, 129-131
.github/workflows/publish.yml (1)
97-97: Verify that thewindows-2025runner image is available.Same verification needed as in
ci.ymlline 103.src/main/java/org/grimmory/pdfium4j/internal/AttachmentBindings.java (1)
15-15: LGTM!src/main/java/org/grimmory/pdfium4j/internal/ThumbnailBindings.java (1)
12-12: LGTM!src/main/java/org/grimmory/pdfium4j/internal/Generators.java (1)
10-10: LGTM!src/main/java/org/grimmory/pdfium4j/internal/PdfDocumentFallbackMeta.java (1)
181-181: LGTM!.github/workflows/ci.yml (1)
103-103: ⚡ Quick winNo action needed —
windows-2025is a documented, available GitHub Actions runner image provided and maintained by GitHub.> Likely an incorrect or invalid review comment.src/main/java/org/grimmory/pdfium4j/model/BookMetadata.java (1)
17-18: ⚡ Quick winNo action needed.
PdfBookMetadatais a Java record withsubtitleas a component, which automatically generates the requiredsubtitle()method to satisfy theBookMetadatainterface contract. All implementations are properly updated.src/main/java/org/grimmory/pdfium4j/model/PdfBookMetadata.java (1)
31-31: ⚡ Quick winThe
subtitleparameter is correctly provided in the onlyPdfBookMetadataconstruction site. All 13 record fields are properly accounted for inPdfDocument.javaline 922, with subtitle passed asOptional.empty()at the correct position. No other construction sites exist that need updating.src/main/java/org/grimmory/pdfium4j/internal/InternalLogger.java (1)
3-4: LGTM!Also applies to: 8-8, 13-13, 17-17
src/main/java/org/grimmory/pdfium4j/internal/AnnotBindings.java (1)
50-55: LGTM!Also applies to: 57-62, 64-69, 71-76, 78-87, 89-94
src/main/java/org/grimmory/pdfium4j/internal/EditBindings.java (1)
41-46: LGTM!Also applies to: 48-53, 55-60, 74-83, 85-94, 98-108, 110-119, 121-126, 128-133, 135-140, 144-153
src/main/java/org/grimmory/pdfium4j/internal/TextBindings.java (1)
42-47: StableValue class implementation is missing from the codebase.The review comment raises a valid concern about null-handling semantics, but cannot be verified because
StableValueis used throughout the code (in TextBindings, FfmHelper, Generators, and all binding files) yet the class definition does not exist in the repository.The concern itself is legitimate: if
orElseSet()caches null values returned by the supplier (whenfind()fails to locate a symbol), the binding would not retry the lookup on subsequent calls. However, without access to theStableValueimplementation, the actual behavior cannot be confirmed. The code may already handle this correctly (as suggested by patterns in AttachmentBindings where results are wrapped inOptional.ofNullable().orElse(null)), or it may indeed have the caching issue described.build.gradle.kts (5)
18-18: LGTM!
117-118: LGTM!
141-150: LGTM!
555-555: LGTM!
652-652: LGTM!Also applies to: 689-689
src/main/java/org/grimmory/pdfium4j/internal/DocBindings.java (1)
42-50: LGTM!Also applies to: 53-61, 64-69, 72-76, 79-87, 90-98, 101-109, 112-120, 123-127, 130-134, 137-145, 148-156
src/main/java/org/grimmory/pdfium4j/internal/BitmapBindings.java (1)
39-45: LGTM!Also applies to: 48-56, 59-67, 70-74, 77-81, 84-88, 91-95, 98-102
src/main/java/org/grimmory/pdfium4j/internal/NativeLoader.java (2)
9-9: LGTM!Also applies to: 203-206
34-34: LGTM!Also applies to: 36-36, 39-39, 45-45, 193-193
src/main/java/org/grimmory/pdfium4j/internal/ViewBindings.java (1)
71-75: LGTM!Also applies to: 78-82, 85-93, 96-104, 107-115, 118-122, 125-129, 132-141, 144-152, 155-159, 162-166, 169-173, 176-184, 187-195, 198-207, 210-214, 217-225
src/main/java/org/grimmory/pdfium4j/XmpMetadataWriter.java (4)
71-84: LGTM!
130-146: LGTM!Also applies to: 148-188
453-458: LGTM!
559-562: LGTM!src/main/java/org/grimmory/pdfium4j/model/XmpMetadata.java (4)
132-164: LGTM!
181-198: LGTM!
212-225: LGTM!
227-358: LGTM!src/main/java/org/grimmory/pdfium4j/XmpMetadataParser.java (1)
170-171: LGTM!src/test/java/org/grimmory/pdfium4j/XmpMetadataParserTest.java (4)
12-34: LGTM!
74-74: LGTM!Also applies to: 126-127, 208-208
211-278: LGTM!
281-387: LGTM!src/test/java/org/grimmory/pdfium4j/CorpusMetadataRoundTripTest.java (2)
158-160: LGTM!
171-176: LGTM!src/test/java/org/grimmory/pdfium4j/PdfDocumentTest.java (2)
575-600: LGTM!
1668-1700: LGTM!src/main/java/org/grimmory/pdfium4j/PdfSaver.java (3)
68-75: LGTM!
149-202: LGTM!
243-252: LGTM!src/main/java/org/grimmory/pdfium4j/internal/ShimBindings.java (1)
48-53: LGTM!Also applies to: 56-64, 66-75, 77-86, 88-97, 99-108, 110-119, 121-130, 132-141, 143-148, 150-159, 161-170, 172-181, 183-192, 194-199, 201-206, 208-218, 220-229, 231-241, 243-252, 254-263, 265-274, 276-285, 287-297, 299-309, 311-320, 322-332, 334-343, 345-354, 356-362, 364-373, 375-384, 386-396, 398-409, 411-422, 424-433, 435-444, 446-451
shim/src/pdfium4j_utils.h (2)
13-15: LGTM!
17-28: LGTM!shim/src/pdfium4j_write.cpp (4)
16-103: LGTM!
122-168: LGTM!
194-244: LGTM!
246-293: LGTM!src/main/java/org/grimmory/pdfium4j/PdfDocument.java (2)
914-919: LGTM!
126-126: ⚡ Quick winNo action required. The build configuration already correctly enables preview features for
StableValue. The Gradle build file (build.gradle.kts) setslanguageVersion = JavaLanguageVersion.of(25)and includes--enable-previewin both theJavaCompilecompiler arguments (line 117) and allJavaExecandTestruntime JVM arguments. This configuration is sufficient and correct; the code will compile and run without issues.shim/include/pdfium4j_shim.h (2)
140-148: LGTM!Also applies to: 168-181
25-32: ⚡ Quick winABI layout is already verified—static assertions and Java implementation are correct.
The struct
pdfium4j_char_info_t(24 bytes:int+ 5×float) already has explicitstatic_assertchecks for size and alignment inshim/include/pdfium4j_shim.h(lines 14–15). The Java-side memory layout inPdfPage.java(lines 482–488) correctly reads the struct fields at offsets 0, 4, 8, 12, 16, 20 with the expected 24-byte stride. Integration tests inPdfDocumentTest.javaandPdfPageTextAllocationTest.javaexercise this path and confirm correctness.> Likely an incorrect or invalid review comment.shim/src/pdfium4j_read.cpp (4)
30-66: LGTM!
216-293: LGTM!
295-364: LGTM!
385-387: LGTM!Also applies to: 419-421
balazs-szucs
force-pushed
the
modern-stuff
branch
from
0fb0fc4 to
d1d9752
Compare
|
1 participant
Mostly done via automated tools. Notable changes: volatilite -> StableValue, remove unused, Intellij's "cleanup", adding more fields to XMP, bump C++, minor modernization there.
Summary by CodeRabbit
Release Notes
New Features
Improvements