OCR: Add Docling engine - #16231
Conversation
| exports org.jabref.logic.ai.summarization.util; | ||
| exports org.jabref.logic.msc; | ||
| exports org.jabref.logic.ai.models; | ||
| exports org.jabref.logic.ocr.Docling; |
InAnYan
left a comment
There was a problem hiding this comment.
Cool! This is a good start and I think you are in right direction!
|
The requested changes were not addressed for 3 days. Please follow-up in the next 7 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
PR Summary by QodoOCR: Add Docling engine and shared OCR utilities
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1. Unchecked prov/page index
|
There was a problem hiding this comment.
Okay, for now to move forward, the most critical points are:
- Move some classes to model layer
- There should not be commented out code
- Look what happens here - it reads JSON, but what if there will be a parsing error? Maybe add try/catch block. Or the error is
IOException? - Look at Qodo suggestions
|
I edited the comment because I hit Enter too quickly |
| this.preferences = preferences; | ||
| this.taskExecutor = taskExecutor; | ||
| this.ocrEngine = new OcrMyPdfEngine(preferences.getOcrPreferences()); | ||
| // this.ocrEngine = new OcrMyPdfEngine(preferences.getOcrPreferences()); |
subhramit
left a comment
There was a problem hiding this comment.
Your learning curve is really visible - I didn't have to repeat any comment I had in your older PRs :)
| } | ||
|
|
||
| private OcrResult embedText(Path jsonOutputPath, Path originalPdf) throws IOException { | ||
| JsonMapper jsonMapper = new JsonMapper(); |
There was a problem hiding this comment.
Even if this class is used once, we should store this initialization as a private static final field for subsequent uses and not initialize every time on this method call.
| PDFont font = new PDType1Font(Standard14Fonts.FontName.HELVETICA); | ||
| float fontSize = 12F; |
There was a problem hiding this comment.
Same for the font and fontSize - it is fixed for every time this method is called in this class.
Move to class constants, don't re-initialize.
It throws |
any XYZIOException should be a subset of IOException unless they just named it that way. But work with specific exceptions as much as you can. Whatever is true for IOException should be true for this as well. Example - #16452 (comment) (not a very good pattern but just for reference) |
InAnYan
left a comment
There was a problem hiding this comment.
Okay, good! For now we can move on
| /// @return true if the engine is available, false otherwise. | ||
| public static boolean isAvailable(OcrPreferences ocrPreferences) { | ||
| ArrayList<String> command = StringUtil.splitRespectingEscapedWhitespace(ocrPreferences.getOcrEnginePath()); | ||
| command.add("--version"); |
There was a problem hiding this comment.
I would probably leave it in the OcrEngine interface, because each specific engine might have their own arguments to check (--help, --version, --v, etc.). And what if there won't be a long version for "version"?
I would leave the logic to run the process in ocr utils
Related issues and pull requests
Closes #13267
PR Description
Add Docling as the second engine for OCR
Steps to test
AI usage
Claude (Sonnet 4.6 through web), used for discussion and to double-check the solution.
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)