Improve Document Viewer error handling and state management #13298
Conversation
- 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
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerView.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java
Show resolved
Hide resolved
|
|
||
| // Filter to include only PDF files | ||
| Set<LinkedFile> pdfFiles = linkedFiles.stream() | ||
| .filter(this::isPdfFile) |
There was a problem hiding this comment.
Move idPdfFile filter in the stream above so you can directlry filter
| * @param file the LinkedFile to check | ||
| * @return true if the file is a PDF, false otherwise | ||
| */ | ||
| private boolean isPdfFile(LinkedFile file) { |
There was a problem hiding this comment.
You can use the existing method already
- 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
| @@ -89,11 +108,34 @@ private void setCurrentDocument(Path path) { | |||
| } | |||
| } | |||
|
|
|||
| private boolean isPdfFile(LinkedFile file) { | |||
| if (file == null || file.getLink() == null || file.getLink().trim().isEmpty()) { | |||
There was a problem hiding this comment.
The method isPdfFile should not use exceptions for control flow. Instead, it should handle null checks and invalid states more gracefully.
| LOGGER.debug("No document provided to viewer, showing placeholder"); | ||
| // Show placeholder and hide PDF | ||
| pdfView.setVisible(false); | ||
| placeholderLabel.setText("No PDF available for preview"); |
There was a problem hiding this comment.
The label text should be in sentence case and consistent with other strings. Consider using 'No PDF available for preview.' to maintain consistency and proper sentence structure.
| } | ||
| } else { | ||
| LOGGER.error("Could not load PDF document: no document found"); | ||
| LOGGER.debug("No document provided to viewer, showing placeholder"); |
There was a problem hiding this comment.
The log message should end with a period instead of a comma for proper sentence structure.
|
@Siedlerchr Thanks for the feedback! I've addressed all the review comments:
The functionality has been tested and works as expected. |
| if (pdfFiles.isEmpty()) { | ||
| // No PDF files found - clear the list and current document | ||
| files.clear(); | ||
| currentDocument.set(null); |
There was a problem hiding this comment.
I htink it would be good to display a notification: dialogService.notify(Localization.lang("No pdf files available"))
|
Just one little addition and I think it's ready |
Closes #13198
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
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)