Abstract duplicate code in performing OCR

[ZiadAbdElFatah]

Summary

Abstract duplicate code for performing OCR to Ocrutils

AI usage

NA

Checklist

• I own the copyright of the code submitted and I license it under the MIT license
• If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
• I manually tested my changes in running JabRef (always required)
• [/] I added JUnit tests for changes (if applicable)
• [/] I added screenshots in the PR description (if change is visible to the user)
• [/] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
• [/] I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
• [/] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Centralize OCR process execution and deduplicate engine logic

✨ Enhancement 🕐 20-40 Minutes

AI Description

• Extract shared OCR process execution (logging, stream handling, timeout) into OcrUtils.
• Update OCR engines to delegate process running and only handle engine-specific outputs.
• Standardize failure handling via explicit error codes on OcrFailureReason.

Diagram

graph TD
  A["OCR Engines"] --> B["OcrUtils.performOcr"] --> C{{"OCR CLI process"}} --> D["Exit code"] --> E["OcrFailureReason"] --> F["OcrResult"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Return a typed outcome instead of int codes

• ➕ Eliminates sentinel integers and makes success/failure explicit (e.g., sealed type or Result-style object).
• ➕ Avoids any coupling between numeric codes and enum ordering.
• ➖ Requires introducing a new type and updating engine code accordingly.

2. Add OcrFailureReason.fromErrorCode(int) and stop using values()[code-1]

• ➕ Keeps the helper method simple while removing dependence on enum declaration order.
• ➕ Allows future changes to error codes without breaking decoding.
• ➖ Still uses numeric error codes internally, just with safer mapping.

Recommendation: Centralizing the ProcessBuilder/timeout/stream-gobbling logic in OcrUtils is the right direction and reduces duplicated, error-prone code. The main follow-up improvement would be to replace OcrFailureReason.values()[exitCode - 1] with an explicit fromErrorCode (or a typed outcome) to prevent subtle bugs if enum ordering/codes change.

Files changed (5) +62 / -71

Enhancement (1) +35 / -0

OcrUtils.javaAdd shared performOcr helper for process execution and error mapping +35/-0

Add shared performOcr helper for process execution and error mapping

• Introduces performOcr to start the external OCR process, consume output asynchronously, enforce a timeout, and translate common failures into OcrFailureReason error codes with engine-specific log messages.

jablib/src/main/java/org/jabref/logic/ocr/OcrUtils.java

Refactor (4) +27 / -71

OcrEngine.javaUpdate OCR engine contract to allow IOException propagation +2/-1

Update OCR engine contract to allow IOException propagation

• Adds IOException to the performOcrAndEmbedText signature, allowing engines to surface I/O errors from engine-specific post-processing instead of always converting them into OcrResult failures.

jablib/src/main/java/org/jabref/logic/ocr/OcrEngine.java

OcrFailureReason.javaIntroduce stable integer error codes for OCR failure reasons +11/-1

Introduce stable integer error codes for OCR failure reasons

• Associates each failure reason with an explicit integer errorCode and exposes a getter, enabling a common runner to return standardized failure codes.

jablib/src/main/java/org/jabref/logic/ocr/OcrFailureReason.java

OcrMyPdfEngine.javaReplace local process handling with shared OCR runner +5/-33

Replace local process handling with shared OCR runner

• Removes duplicated ProcessBuilder/timeout/stream handling and delegates execution to OcrUtils.performOcr. Maps the returned status code back into an OcrResult.

jablib/src/main/java/org/jabref/logic/ocr/OcrMyPdfEngine.java

DoclingEngine.javaDelegate Docling CLI invocation to OcrUtils.performOcr +9/-36

Delegate Docling CLI invocation to OcrUtils.performOcr

• Removes duplicated external-process handling and uses the shared runner for executing the Docling command. On success, continues with JSON output discovery and PDF text embedding; method now declares IOException.

jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Brittle values() error mapping ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The code converts exitCode to OcrFailureReason using OcrFailureReason.values()[exitCode - 1],
which silently depends on enum declaration order and assumes contiguous 1-based codes. This can
mis-report OCR failures or throw ArrayIndexOutOfBoundsException for unexpected codes, making
failure handling fragile and harder to maintain safely.

Code

jablib/src/main/java/org/jabref/logic/ocr/OcrMyPdfEngine.java[57]

+            return OcrResult.failure(OcrFailureReason.values()[exitCode - 1]);

Evidence

The cited changes introduce explicit numeric errorCode values on OcrFailureReason and have
OcrUtils.performOcr return an integer derived from getErrorCode(), but both OcrMyPdfEngine and
DoclingEngine decode that integer using ordinal-based array indexing (values()[exitCode - 1])
instead of mapping by errorCode. This creates hidden coupling to enum ordering (a maintainability
drift risk noted by rule 4) and introduces an unchecked runtime hazard (rule 9) because any
unexpected or non-contiguous code can lead to incorrect mapping or an
ArrayIndexOutOfBoundsException.

AGENTS.md: Keep code maintainable: small focused methods, SRP, avoid duplication and premature abstractions, follow documented JabRef code style: AGENTS.md: Keep code maintainable: small focused methods, SRP, avoid duplication and premature abstractions, follow documented JabRef code style: AGENTS.md: Keep code maintainable: small focused methods, SRP, avoid duplication and premature abstractions, follow documented JabRef code style: AGENTS.md: Keep code maintainable: small focused methods, SRP, avoid duplication and premature abstractions, follow documented JabRef code style
AGENTS.md: Exceptions and logging: avoid unchecked exceptions, catch specific exceptions only, log exceptions correctly: AGENTS.md: Exceptions and logging: avoid unchecked exceptions, catch specific exceptions only, log exceptions correctly: AGENTS.md: Exceptions and logging: avoid unchecked exceptions, catch specific exceptions only, log exceptions correctly: AGENTS.md: Exceptions and logging: avoid unchecked exceptions, catch specific exceptions only, log exceptions correctly
jablib/src/main/java/org/jabref/logic/ocr/OcrMyPdfEngine.java[53-58]
jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java[66-74]
jablib/src/main/java/org/jabref/logic/ocr/OcrFailureReason.java[4-15]
jablib/src/main/java/org/jabref/logic/ocr/OcrFailureReason.java[4-16]
jablib/src/main/java/org/jabref/logic/ocr/OcrUtils.java[50-80]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`OcrUtils.performOcr` returns an integer derived from `OcrFailureReason.getErrorCode()`, but callers convert it back to an enum using `OcrFailureReason.values()[exitCode - 1]`. This is brittle because it relies on enum declaration order matching `errorCode`, assumes contiguous 1-based codes, and can throw an unchecked exception if an unexpected code is returned.
## Issue Context
`OcrFailureReason` now defines explicit `errorCode` values and `performOcr` returns those codes, but the engines still use ordinal-based indexing rather than reverse-mapping by `errorCode`. Engines should reverse-map via `getErrorCode()` (or return `OcrFailureReason` directly) and handle unknown/unsupported codes defensively.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ocr/OcrMyPdfEngine.java[53-58]
- jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java[66-74]
- jablib/src/main/java/org/jabref/logic/ocr/OcrFailureReason.java[4-16]
- jablib/src/main/java/org/jabref/logic/ocr/OcrUtils.java[50-81]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Exception bypasses OCR result 🐞 Bug ≡ Correctness

Description

OcrEngine.performOcrAndEmbedText now declares throws IOException even though failures are modeled as
OcrResult, and DoclingEngine lets embedText IOExceptions propagate. In the GUI this routes to
BackgroundTask.onFailure (generic message) instead of the localized OcrResult.Failure path.

Code

jablib/src/main/java/org/jabref/logic/ocr/OcrEngine.java[15]

+    OcrResult performOcrAndEmbedText(Path pdfPath) throws IOException;

Evidence

The interface now allows throwing IOException, and DoclingEngine’s main method is declared throws
IOException while calling embedText which throws IOException. The GUI distinguishes
OcrResult.Failure (localized) from exceptions (generic onFailure), so these IOExceptions bypass
localized failure handling.

jablib/src/main/java/org/jabref/logic/ocr/OcrEngine.java[11-16]
jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java[50-75]
jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java[77-121]
jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[88-110]
jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[92-106]
jablib/src/main/java/org/jabref/logic/ocr/OcrResult.java[5-39]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The OCR API now mixes two error channels: `OcrResult.Failure` and checked `IOException`. `DoclingEngine` can throw from `embedText(...)`, which is then handled by the GUI as an “unexpected error” instead of a localizable OCR failure reason.
### Issue Context
`OcrResult` is explicitly designed to carry a localizable `OcrFailureReason`. Keeping OCR failures inside `OcrResult` ensures the GUI can consistently display user-friendly messages.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ocr/OcrEngine.java[11-20]
- jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java[50-75]
- jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[88-110]
- jablib/src/main/java/org/jabref/logic/ocr/OcrResult.java[5-39]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

2. Exception bypasses ocr result 🐞 Bug ≡ Correctness

OcrEngine.performOcrAndEmbedText now declares throws IOException even though failures are modeled as
OcrResult, and DoclingEngine lets embedText IOExceptions propagate. In the GUI this routes to
BackgroundTask.onFailure (generic message) instead of the localized OcrResult.Failure path.

Agent Prompt

### Issue description
The OCR API now mixes two error channels: `OcrResult.Failure` and checked `IOException`. `DoclingEngine` can throw from `embedText(...)`, which is then handled by the GUI as an “unexpected error” instead of a localizable OCR failure reason.

### Issue Context
`OcrResult` is explicitly designed to carry a localizable `OcrFailureReason`. Keeping OCR failures inside `OcrResult` ensures the GUI can consistently display user-friendly messages.

### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ocr/OcrEngine.java[11-20]
- jablib/src/main/java/org/jabref/logic/ocr/docling/DoclingEngine.java[50-75]
- jabgui/src/main/java/org/jabref/gui/linkedfile/OcrLinkedFileAction.java[88-110]
- jablib/src/main/java/org/jabref/logic/ocr/OcrResult.java[5-39]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[koppor]

If this is too complex, you can play around with https://github.com/itsallcode/simple-process