Add support for displaying PDF annotations in entry preview#15493
Closed
hallekoyanagi wants to merge 12 commits intoJabRef:mainfrom
Closed
Add support for displaying PDF annotations in entry preview#15493hallekoyanagi wants to merge 12 commits intoJabRef:mainfrom
hallekoyanagi wants to merge 12 commits intoJabRef:mainfrom
Conversation
… change (built by Carl)
…did a quick fix in PreviewViewer removed change in PreviewPanel cleanup needed
…or clearer implementation, removed annotationCache global variable Added FileAnnotationHtmlRendererTest (Built by Carl)
Added support for linked PDF annotations and various CLI options.
Contributor
Review Summary by QodoAdd PDF annotations display in entry preview
WalkthroughsDescription• Display PDF annotations in entry preview with file names and page numbers • Render annotations as HTML appended to preview with proper escaping • Support linked annotations showing highlights with attached notes • Filter and sort annotations by page for organized presentation Diagramflowchart LR
A["PreviewViewer.update()"] -->|generates preview| B["layout.generatePreview()"]
B -->|HTML preview| C["injectAnnotations()"]
C -->|retrieves annotations| D["FileAnnotationCache"]
D -->|annotation data| E["FileAnnotationHtmlRenderer.render()"]
E -->|formatted HTML| F["Appended to preview"]
File Changes1. jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java
|
Contributor
Code Review by Qodo
1. Annotations bypass preview template
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+218
to
+221
| BackgroundTask.wrap(() -> { | ||
| String previewHtml = layout.generatePreview(currentEntry, databaseContext); | ||
| return injectAnnotations(previewHtml, currentEntry, databaseContext); | ||
| }) |
Contributor
There was a problem hiding this comment.
1. Annotations bypass preview template 📎 Requirement gap ≡ Correctness
The preview HTML is post-processed in PreviewViewer by appending rendered annotations, rather than exposing annotations via a dedicated entry field usable in Preview Entry templates. This prevents users from placing/formatting annotations through the template system as required.
Agent Prompt
## Issue description
PDF annotations are appended to the generated preview HTML instead of being exposed via a dedicated entry field that users can reference in Preview Entry templates.
## Issue Context
The requirement is to make annotations available through a field usable in preview templates (so users can customize layouts), rather than hard-wiring the annotations at the bottom of the preview.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java[218-221]
- jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java[403-410]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+26
to
+62
| StringBuilder html = new StringBuilder(); | ||
| html.append("<BR><BR><b>PDF Annotations</b><BR>"); | ||
|
|
||
| for (Map.Entry<Path, List<FileAnnotation>> fileEntry : annotations.entrySet()) { | ||
| List<FileAnnotation> fileAnnotations = fileEntry.getValue(); | ||
| if (fileAnnotations == null || fileAnnotations.isEmpty()) { | ||
| continue; | ||
| } | ||
|
|
||
| String fileName = fileEntry.getKey().getFileName().toString(); | ||
| html.append("<BR><i>").append(escapeHtml(fileName)).append("</i><BR>"); | ||
|
|
||
| fileAnnotations.stream() | ||
| .sorted(Comparator.comparingInt(FileAnnotation::getPage)) | ||
| .filter(annotation -> StringUtil.isNotBlank(annotation.getContent())) | ||
| .forEach(annotation -> renderAnnotation(html, annotation)); | ||
| } | ||
|
|
||
| return html.toString(); | ||
| } | ||
|
|
||
| private static void renderAnnotation(StringBuilder html, FileAnnotation annotation) { | ||
| String type = annotation.getAnnotationType().toString(); | ||
| int page = annotation.getPage(); | ||
| String content = annotation.getContent(); | ||
|
|
||
| html.append("<b>") | ||
| .append(escapeHtml(type)) | ||
| .append(" (p. ").append(page).append("):</b> "); | ||
|
|
||
| if (annotation.hasLinkedAnnotation()) { | ||
| // highlights/underlines with a sticky note attached | ||
| html.append(escapeHtml(content)); | ||
| String noteContent = annotation.getLinkedFileAnnotation().getContent(); | ||
| if (StringUtil.isNotBlank(noteContent)) { | ||
| html.append(" — <i>Note: ").append(escapeHtml(noteContent)).append("</i>"); | ||
| } |
Contributor
There was a problem hiding this comment.
2. Unlocalized annotation ui strings 📘 Rule violation ⚙ Maintainability
The new annotation rendering hardcodes user-facing strings like PDF Annotations and Note: in generated preview HTML. This breaks localization requirements for UI text.
Agent Prompt
## Issue description
User-facing strings in the preview annotation HTML output are hardcoded in English.
## Issue Context
Preview output is UI-visible; strings such as the section header and linked-note label must be localized using the project's localization mechanism.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/pdf/FileAnnotationHtmlRenderer.java[26-28]
- jablib/src/main/java/org/jabref/logic/pdf/FileAnnotationHtmlRenderer.java[60-62]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+40
to
+52
| void renderSingleHighlightAnnotation() { | ||
| FileAnnotation annotation = createAnnotation("important text", 3, FileAnnotationType.HIGHLIGHT); | ||
| Map<Path, List<FileAnnotation>> annotations = Map.of( | ||
| Path.of("paper.pdf"), List.of(annotation) | ||
| ); | ||
|
|
||
| String result = FileAnnotationHtmlRenderer.render(annotations); | ||
|
|
||
| assertTrue(result.contains("Highlight")); | ||
| assertTrue(result.contains("p. 3")); | ||
| assertTrue(result.contains("important text")); | ||
| assertTrue(result.contains("paper.pdf")); | ||
| } |
Contributor
There was a problem hiding this comment.
3. Tests assert via contains() 📘 Rule violation ☼ Reliability
New unit tests primarily use assertTrue(result.contains(...)) which is a weak predicate check and can pass with near-miss output. These tests should assert exact expected HTML (or structured expectations) when feasible.
Agent Prompt
## Issue description
The new renderer tests use weak substring/predicate assertions (`contains`) instead of asserting exact expected output.
## Issue Context
Exact assertions provide stronger regression protection and clearer diagnostics when output changes.
## Fix Focus Areas
- jablib/src/test/java/org/jabref/logic/pdf/FileAnnotationHtmlRendererTest.java[40-52]
- jablib/src/test/java/org/jabref/logic/pdf/FileAnnotationHtmlRendererTest.java[55-67]
- jablib/src/test/java/org/jabref/logic/pdf/FileAnnotationHtmlRendererTest.java[69-95]
- jablib/src/test/java/org/jabref/logic/pdf/FileAnnotationHtmlRendererTest.java[97-126]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+403
to
+406
| private String injectAnnotations(String previewHtml, BibEntry entry, BibDatabaseContext databaseContext) { | ||
| FileAnnotationCache annotationCache = new FileAnnotationCache(databaseContext, preferences.getFilePreferences()); | ||
| Map<Path, List<FileAnnotation>> annotations = annotationCache.getFromCache(entry); | ||
| if (annotations == null || annotations.isEmpty()) { |
Contributor
There was a problem hiding this comment.
4. Cache recreated every refresh 🐞 Bug ➹ Performance
PreviewViewer.injectAnnotations creates a new FileAnnotationCache on every update, defeating the Caffeine cache and repeatedly re-reading PDFs from disk. This can noticeably slow preview rendering because annotation extraction happens in the same background task as preview generation.
Agent Prompt
### Issue description
`PreviewViewer.injectAnnotations(...)` allocates a new `FileAnnotationCache` on every preview refresh. Because `FileAnnotationCache` is a Caffeine `LoadingCache` whose loader imports annotations by scanning PDFs, recreating it defeats caching and causes repeated PDF disk reads, delaying preview rendering.
### Issue Context
A long-lived `FileAnnotationCache` already exists per `LibraryTab` and is exposed via `getAnnotationCache()`, indicating reuse is expected.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java[205-225]
- jabgui/src/main/java/org/jabref/gui/preview/PreviewViewer.java[403-411]
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[226-231]
### Suggested fix
- Add a `@Nullable FileAnnotationCache annotationCache` field in `PreviewViewer`.
- Initialize/refresh it when `setDatabaseContext(...)` is called (and set to `null` when context is `null`).
- In `injectAnnotations(...)`, use the existing field instead of creating a new cache.
- (Optional, but good) If `annotationCache` is `null` (e.g., no DB context), return `previewHtml` without attempting annotation loading.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
github-actions
bot
added
the
status: changes-required
github-actions
bot
added
status: no-bot-comments
and removed
status: changes-required
calixtus
reviewed
Apr 4, 2026
Comment on lines
-96
to
+101
| public PreviewViewer(DialogService dialogService, | ||
| GuiPreferences preferences, | ||
| ThemeManager themeManager, | ||
| TaskExecutor taskExecutor) { | ||
| public PreviewViewer(DialogService dialogService, GuiPreferences preferences, ThemeManager themeManager, TaskExecutor taskExecutor) { |
Member
There was a problem hiding this comment.
Please avoid reformatting newlines
calixtus
reviewed
Apr 4, 2026
Comment on lines
-103
to
+105
| public PreviewViewer(DialogService dialogService, | ||
| GuiPreferences preferences, | ||
| ThemeManager themeManager, | ||
| TaskExecutor taskExecutor, | ||
| StringProperty searchQueryProperty) { | ||
| public PreviewViewer(DialogService dialogService, GuiPreferences preferences, ThemeManager themeManager, TaskExecutor taskExecutor, StringProperty searchQueryProperty) { |
calixtus
reviewed
Apr 4, 2026
Comment on lines
-202
to
+213
| LOGGER.debug("Missing components - Database: {}, Entry: {}, Layout: {}", | ||
| databaseContext == null ? "null" : databaseContext, | ||
| entry == null ? "null" : entry, | ||
| layout == null ? "null" : layout); | ||
| LOGGER.debug("Missing components - Database: {}, Entry: {}, Layout: {}", databaseContext == null ? "null" : databaseContext, entry == null ? "null" : entry, layout == null ? "null" : layout); |
calixtus
reviewed
Apr 4, 2026
| BackgroundTask.wrap(() -> { | ||
| String previewHtml = layout.generatePreview(currentEntry, databaseContext); | ||
| return injectAnnotations(previewHtml, currentEntry, databaseContext); | ||
| }).onSuccess(this::setPreviewText).onFailure(e -> setPreviewText(formatError(currentEntry, e))).executeWith(taskExecutor); |
calixtus
reviewed
Apr 4, 2026
| """.formatted( | ||
| Localization.lang("Error while generating citation style"), | ||
| exception.getLocalizedMessage() != null ? exception.getLocalizedMessage() : "Unknown error"); | ||
| """.formatted(Localization.lang("Error while generating citation style"), exception.getLocalizedMessage() != null ? exception.getLocalizedMessage() : "Unknown error"); |
calixtus
reviewed
Apr 4, 2026
Comment on lines
-261
to
-273
| private static String formatPreviewText(String baseUrl, String coverIfAny, String text) { | ||
| return """ | ||
| <html> | ||
| <head> | ||
| <base href="%s"> | ||
| </head> | ||
| <body id="previewBody"> | ||
| %s <div id="content"> %s </div> | ||
| </body> | ||
| </html> | ||
| """.formatted(baseUrl, coverIfAny, text); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Please avoid shifting code around
calixtus
reviewed
Apr 4, 2026
Comment on lines
293
to
298
| BackgroundTask.wrap(() -> { | ||
| job.getJobSettings().setJobName(entry.getCitationKey().orElse("NO CITATION KEY")); | ||
| previewView.getEngine().print(job); | ||
| job.endJob(); | ||
| }) | ||
| .onFailure(e -> dialogService.showErrorDialogAndWait(Localization.lang("Could not print preview"), e)) | ||
| .executeWith(taskExecutor); | ||
| job.getJobSettings().setJobName(entry.getCitationKey().orElse("NO CITATION KEY")); | ||
| previewView.getEngine().print(job); | ||
| job.endJob(); | ||
| }).onFailure(e -> dialogService.showErrorDialogAndWait(Localization.lang("Could not print preview"), e)).executeWith(taskExecutor); | ||
| } |
Member
There was a problem hiding this comment.
Please avoid reformatting without changes
calixtus
reviewed
Apr 4, 2026
Comment on lines
-345
to
+336
| ExportToClipboardAction exportToClipboardAction = new ExportToClipboardAction( | ||
| dialogService, | ||
| stateManager, | ||
| clipBoardManager, | ||
| taskExecutor, | ||
| preferences); | ||
| ExportToClipboardAction exportToClipboardAction = new ExportToClipboardAction(dialogService, stateManager, clipBoardManager, taskExecutor, preferences); |
calixtus
reviewed
Apr 4, 2026
Comment on lines
-366
to
+352
| Object result = previewView.getEngine().executeScript( | ||
| "var content = document.getElementById('content');" + | ||
| "content ? content.getBoundingClientRect().height : document.body.scrollHeight;" | ||
| ); | ||
| Object result = previewView.getEngine().executeScript("var content = document.getElementById('content');" + "content ? content.getBoundingClientRect().height : document.body.scrollHeight;"); |
calixtus
requested changes
Apr 4, 2026
Member
calixtus
left a comment
calixtus
left a comment
There was a problem hiding this comment.
Please undo all reformattings unrelated to your changes
Member
|
Please also do not forget to disclose AI usage in the creation of this PR. |
github-actions
bot
added
status: changes-required
Contributor
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
Member
|
Closing due to inactivity |
Contributor
|
This pull requests was closed without merging. You have been unassigned from the respective issue #4257. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues and pull requests
Closes #4257
PR Description
This change integrates PDF annotations into the entry preview by post-processing the generated preview HTML in PreviewViewer, appending rendered annotation content from cached PDF data. The goal was to make annotations visible directly within the preview without modifying the existing layout system, keeping the implementation lightweight and consistent with the current rendering pipeline.
This approach ensures annotations are displayed reliably regardless of user-defined preview layouts, resulting in a smaller and more maintainable change than initially expected while improving visibility of PDF metadata for users.
Steps to test
a. annotation type
b. page number
c. annotation content
Expected Result
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)