Improve Document Viewer error handling and state management #13298

FlyJoanne · 2025-06-10T06:38:14Z

Add user-friendly messages when no PDF files are available
Fix state management to properly clear content when switching entries
Replace technical exceptions with helpful placeholder text
Improve user experience for entries with non-PDF files only

Improved the Document Viewer's handling of entries that contain only non-PDF files by replacing technical error messages with user-friendly placeholders and fixing state management issues that caused stale content to be displayed when switching between entries.

Steps to test

Test entries with non-PDF files only:
- Select a bibliography entry that has only .docx, .md, or other non-PDF files attached
- Open Document Viewer
- Expected: Should display "No PDF available for preview" message instead of technical exceptions

Test normal PDF functionality:
- Select an entry with PDF files attached
- Open Document Viewer
- Expected: PDF should display normally with dropdown showing only PDF files

Test state management:
- First open Document Viewer with an entry containing PDF files
- Then switch to an entry with only non-PDF files
- Expected: Should show placeholder message, not the previous PDF content

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
[/] Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (if change is visible to the user)
[/] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
[/] Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

- Add user-friendly messages when no PDF files are available - Fix state management to properly clear content when switching entries - Replace technical exceptions with helpful placeholder text - Improve user experience for entries with non-PDF files only Closes JabRef#13198

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerView.java

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

Siedlerchr · 2025-06-10T06:45:07Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

+
+            // Filter to include only PDF files
+            Set<LinkedFile> pdfFiles = linkedFiles.stream()
+                                                  .filter(this::isPdfFile)


Move idPdfFile filter in the stream above so you can directlry filter

Siedlerchr · 2025-06-10T06:46:56Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

+     * @param file the LinkedFile to check
+     * @return true if the file is a PDF, false otherwise
+     */
+    private boolean isPdfFile(LinkedFile file) {


You can use the existing method already

jabref/jablib/src/main/java/org/jabref/logic/util/io/FileUtil.java

Line 494 in faed0c2

public static boolean isPDFFile(Path file) {

- Remove warning symbol from error message text for better formatting - Optimize PDF filtering to use single stream operation - Simplify isPdfFile method to use existing FileUtil directly - Fix checkstyle issues

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java

FlyJoanne · 2025-06-10T18:35:16Z

@Siedlerchr Thanks for the feedback! I've addressed all the review comments:

Removed the warning symbol from error messages
Optimized the PDF filtering to use a single stream operation as suggested
Simplified the isPdfFile method to directly use existing FileUtil
Fixed all checkstyle issues

The functionality has been tested and works as expected.

Siedlerchr · 2025-06-10T19:14:13Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

+            if (pdfFiles.isEmpty()) {
+                // No PDF files found - clear the list and current document
+                files.clear();
+                currentDocument.set(null);


I htink it would be good to display a notification: dialogService.notify(Localization.lang("No pdf files available"))

Siedlerchr · 2025-06-10T19:31:12Z

Just one little addition and I think it's ready

calixtus · 2025-06-11T07:20:25Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

@@ -35,6 +38,7 @@ public class DocumentViewerViewModel extends AbstractViewModel {
    private final BooleanProperty liveMode = new SimpleBooleanProperty(true);
    private final IntegerProperty currentPage = new SimpleIntegerProperty();
    private final StringProperty highlightText = new SimpleStringProperty();
+    private final Logger LOGGER = LoggerFactory.getLogger(DocumentViewerViewModel.class);


for the sake of consistency with other classes, please put LOGGER to the top of field definitions above stateManager.

calixtus · 2025-06-11T07:20:57Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

+            Set<LinkedFile> pdfFiles = entries.stream()
+                                              .map(BibEntry::getFiles)
+                                              .flatMap(List::stream)
+                                              .filter(this::isPdfFile)  // 直接在这里过滤


Please only make english comments

calixtus · 2025-06-11T07:24:20Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java

+        try {
+            Path filePath = Path.of(file.getLink());
+            return FileUtil.isPDFFile(filePath);
+        } catch (Exception e) {


do not catch generic exceptions, be very specific. You don't know then why sthg failed? 1000% sure it is because it is not a pdf file?

calixtus · 2025-06-11T07:25:11Z

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java

+
+        // Create placeholder label
+        placeholderLabel = new Label("No PDF available for preview");
+        placeholderLabel.setStyle("-fx-text-fill: #666666; -fx-font-size: 16px;");


no random style definitions in code. Put everything in base.css !

calixtus · 2025-06-11T07:25:18Z

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java

-        getChildren().add(pdfView);
+
+        // Create placeholder label
+        placeholderLabel = new Label("No PDF available for preview");


Localization

calixtus · 2025-06-11T07:26:43Z

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java

+        // Initially show placeholder
+        pdfView.setVisible(false);
+        placeholderLabel.setVisible(true);
+
        EasyBind.subscribe(currentPage, current -> pdfView.setPage(current.intValue()));
        // We can only set the search query at the moment not the results or mark them in the text
        EasyBind.subscribe(highlightText, pdfView::setSearchText);
+        // Initially hide PDFView until a document is loaded
+        pdfView.setVisible(false);


duplication

calixtus · 2025-06-11T07:27:36Z

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java

+                // Show PDF and hide placeholder
+                pdfView.setVisible(true);
+                placeholderLabel.setVisible(false);


meaningless comment. superfluous. But i know. AI loves this stupid comments.
What LLM do you use?

calixtus · 2025-06-11T07:28:36Z

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java

+                // Show error message
+                pdfView.setVisible(false);
+                placeholderLabel.setText("Could not load PDF: " + document.getFileName());
+                placeholderLabel.setVisible(true);


Localization

calixtus · 2025-06-11T07:29:03Z

jablib/src/main/abbrv.jabref.org

calixtus · 2025-06-11T07:29:12Z

jablib/src/main/resources/csl-styles

calixtus · 2025-06-11T07:29:57Z

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerView.java

+            // Always call show(), even when newDocument is null
+            viewer.show(newDocument);


meaningless comment

calixtus · 2025-06-11T07:40:43Z

https://devdocs.jabref.org/code-howtos/faq.html#submodules

https://github.com/JabRef/jabref/blob/main/CONTRIBUTING.md#notes-on-ai-usage

trag-bot bot reviewed Jun 10, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerView.java Show resolved Hide resolved

trag-bot bot reviewed Jun 10, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java Show resolved Hide resolved

Siedlerchr reviewed Jun 10, 2025

View reviewed changes

Address review feedback

74f1001

- Remove warning symbol from error message text for better formatting - Optimize PDF filtering to use single stream operation - Simplify isPdfFile method to use existing FileUtil directly - Fix checkstyle issues

trag-bot bot reviewed Jun 10, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java Show resolved Hide resolved

trag-bot bot reviewed Jun 10, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java Show resolved Hide resolved

trag-bot bot reviewed Jun 10, 2025

View reviewed changes

jabgui/src/main/java/org/jabref/gui/documentviewer/PdfDocumentViewer.java Show resolved Hide resolved

Siedlerchr reviewed Jun 10, 2025

View reviewed changes

calixtus reviewed Jun 11, 2025

View reviewed changes

jablib/src/main/abbrv.jabref.org

Copy link

Member

calixtus Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo

calixtus reviewed Jun 11, 2025

View reviewed changes

jablib/src/main/resources/csl-styles

Copy link

Member

calixtus Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo

calixtus reviewed Jun 11, 2025

View reviewed changes

		// Always call show(), even when newDocument is null
		viewer.show(newDocument);

Uh oh!

Improve Document Viewer error handling and state management #13298

Are you sure you want to change the base?

Improve Document Viewer error handling and state management #13298

Uh oh!

Conversation

FlyJoanne commented Jun 10, 2025

Steps to test

Mandatory checks

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FlyJoanne commented Jun 10, 2025

Uh oh!

Siedlerchr Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calixtus Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calixtus commented Jun 11, 2025

Uh oh!

Uh oh!

Siedlerchr Jun 10, 2025 •

edited

Loading

calixtus Jun 11, 2025 •

edited

Loading