Skip to content

Improve Document Viewer error handling and state management #13298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
- We fixed an issue where the preview area in the "Select Style" dialog of the LibreOffice integration was too small to display full content. [#13051](https://github.com/JabRef/jabref/issues/13051)
- We fixed an issue where the tab showing the fulltext search results was not displayed. [#12865](https://github.com/JabRef/jabref/issues/12865)
- We fixed an issue showing an empty tooltip in maintable. [#11681](https://github.com/JabRef/jabref/issues/11681)
- We fixed an issue where Document Viewer showed technical exceptions when opening entries with non-PDF files. [#13198](https://github.com/JabRef/jabref/issues/13198)

### Removed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private void setupFileChoice() {
// This also automatically selects the first file on the initial load
Stage stage = (Stage) getDialogPane().getScene().getWindow();
fileChoice.itemsProperty().addListener((observable, oldValue, newValue) -> {
if (newValue.isEmpty()) {
if (newValue == null || newValue.isEmpty()) {
stage.close();
} else {
fileChoice.getSelectionModel().selectFirst();
Expand All @@ -99,9 +99,8 @@ private void setupFileChoice() {

private void setupViewer() {
viewModel.currentDocumentProperty().addListener((observable, oldDocument, newDocument) -> {
if (newDocument != null) {
viewer.show(newDocument);
}
// Always call show(), even when newDocument is null
viewer.show(newDocument);
Comment on lines +102 to +103
Copy link
Member

Choose a reason for hiding this comment

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

meaningless comment

});
viewModel.currentPageProperty().bindBidirectional(viewer.currentPageProperty());
viewModel.highlightTextProperty().bindBidirectional(viewer.highlightTextProperty());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.LinkedFile;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class DocumentViewerViewModel extends AbstractViewModel {

private final StateManager stateManager;
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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


public DocumentViewerViewModel(StateManager stateManager, CliPreferences preferences) {
this.stateManager = Objects.requireNonNull(stateManager);
Expand Down Expand Up @@ -76,10 +80,25 @@ public StringProperty highlightTextProperty() {
private void setCurrentEntries(List<BibEntry> entries) {
if (entries.isEmpty()) {
files.clear();
currentDocument.set(null); // Clear current document when no entries
} else {
Set<LinkedFile> linkedFiles = entries.stream().map(BibEntry::getFiles).flatMap(List::stream).collect(Collectors.toSet());
// We don't need to switch to the first file, this is done automatically in the UI part
files.setValue(FXCollections.observableArrayList(linkedFiles));
Set<LinkedFile> pdfFiles = entries.stream()
.map(BibEntry::getFiles)
.flatMap(List::stream)
.filter(this::isPdfFile) // 直接在这里过滤
Copy link
Member

Choose a reason for hiding this comment

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

Please only make english comments

.collect(Collectors.toSet());

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

@Siedlerchr Siedlerchr Jun 10, 2025

Choose a reason for hiding this comment

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

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

// The UI will automatically close the dialog when files list is empty
// This provides better UX than showing technical errors
} else {
// We have PDF files - display them in the dropdown
files.setValue(FXCollections.observableArrayList(pdfFiles));
// The first file will be automatically selected by the UI
}
}
}

Expand All @@ -89,11 +108,34 @@ private void setCurrentDocument(Path path) {
}
}

private boolean isPdfFile(LinkedFile file) {
Copy link
Member

Choose a reason for hiding this comment

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

You can use the existing method already

public static boolean isPDFFile(Path file) {

if (file == null || file.getLink() == null || file.getLink().trim().isEmpty()) {
return false;
}

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

Choose a reason for hiding this comment

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

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?

return false;
}
}

public void switchToFile(LinkedFile file) {
if (file != null) {
stateManager.getActiveDatabase()
.flatMap(database -> file.findIn(database, preferences.getFilePreferences()))
.ifPresent(this::setCurrentDocument);
.ifPresentOrElse(
this::setCurrentDocument,
() -> {
// File not found or cannot be accessed - clear current document
currentDocument.set(null);
LOGGER.warn("Could not find or access file: {}", file.getLink());
}
);
} else {
// File is null - clear current document to ensure UI consistency
currentDocument.set(null);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import javafx.beans.property.SimpleIntegerProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.geometry.Pos;
import javafx.scene.control.Label;
import javafx.scene.layout.StackPane;

import com.dlsc.pdfviewfx.PDFView;
Expand All @@ -25,13 +27,27 @@ public class PdfDocumentViewer extends StackPane {
private final PDFView pdfView;
private final IntegerProperty currentPage = new SimpleIntegerProperty(0);
private final StringProperty highlightText = new SimpleStringProperty("");
private final Label placeholderLabel;

public PdfDocumentViewer() {
pdfView = new PDFView();
getChildren().add(pdfView);

// Create placeholder label
placeholderLabel = new Label("No PDF available for preview");
Copy link
Member

Choose a reason for hiding this comment

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

Localization

placeholderLabel.setStyle("-fx-text-fill: #666666; -fx-font-size: 16px;");
Copy link
Member

Choose a reason for hiding this comment

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

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

placeholderLabel.setAlignment(Pos.CENTER);

getChildren().addAll(pdfView, placeholderLabel);

// 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);
Comment on lines +42 to +50
Copy link
Member

Choose a reason for hiding this comment

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

duplication

}

public IntegerProperty currentPageProperty() {
Expand All @@ -47,11 +63,23 @@ public void show(Path document) {
try {
pdfView.load(Files.newInputStream(document));
pdfView.setPage(currentPage.get());
// Show PDF and hide placeholder
pdfView.setVisible(true);
placeholderLabel.setVisible(false);
Comment on lines +66 to +68
Copy link
Member

@calixtus calixtus Jun 11, 2025

Choose a reason for hiding this comment

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

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

LOGGER.debug("Successfully loaded PDF document: {}", document);
} catch (IOException e) {
LOGGER.error("Could not load PDF document {}", document, e);
// Show error message
pdfView.setVisible(false);
placeholderLabel.setText("Could not load PDF: " + document.getFileName());
placeholderLabel.setVisible(true);
Comment on lines +72 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Localization

}
} else {
LOGGER.error("Could not load PDF document: no document found");
LOGGER.debug("No document provided to viewer, showing placeholder");
// Show placeholder and hide PDF
pdfView.setVisible(false);
placeholderLabel.setText("No PDF available for preview");
placeholderLabel.setVisible(true);
}
}
}
2 changes: 1 addition & 1 deletion jablib/src/main/abbrv.jabref.org
Copy link
Member

Choose a reason for hiding this comment

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

undo

2 changes: 1 addition & 1 deletion jablib/src/main/resources/csl-styles
Copy link
Member

Choose a reason for hiding this comment

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

undo

Loading