Improve document viewer (#11432)

LoayGhreeb · koppor · web-flow · commit 1c0e4ec283a4 · 2024-06-26T15:36:49.000Z
* Improve document viewer

* Update CHANGELOG.md

* Update CHANGELOG.md

* rewriteRun

* Update CHANGELOG.md

---------

Co-authored-by: Oliver Kopp &lt;kopp.dev@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -53,6 +53,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - We fixed an issue where the Pubmed/Medline Plain importer would not respect the user defined keyword separator [#11413](https://github.com/JabRef/jabref/issues/11413)
 - We fixed an issue where the value of "Override default font settings" was not applied on startup [#11344](https://github.com/JabRef/jabref/issues/11344)
 - We fixed an issue where DatabaseChangeDetailsView was not scrollable when reviewing external metadata changes [#11220](https://github.com/JabRef/jabref/issues/11220)
+- We fixed an issue where clicking on a page number in the search results tab opens a wrong file in the document viewer. [#11432](https://github.com/JabRef/jabref/pull/11432)
 
 ### Removed
 
diff --git a/src/main/java/org/jabref/gui/documentviewer/DocumentViewModel.java b/src/main/java/org/jabref/gui/documentviewer/DocumentViewModel.java
@@ -5,14 +5,10 @@
 import javafx.collections.ObservableList;
 
 public abstract class DocumentViewModel {
-    private IntegerProperty maxPages = new SimpleIntegerProperty();
+    private final IntegerProperty maxPages = new SimpleIntegerProperty();
 
     public abstract ObservableList<DocumentPageViewModel> getPages();
 
-    public int getMaxPages() {
-        return maxPages.get();
-    }
-
     public IntegerProperty maxPagesProperty() {
         return maxPages;
     }
diff --git a/src/main/java/org/jabref/gui/documentviewer/DocumentViewer.fxml b/src/main/java/org/jabref/gui/documentviewer/DocumentViewer.fxml
@@ -46,7 +46,7 @@
             <bottom>
                 <ToolBar>
                     <HBox alignment="CENTER" spacing="5.0" HBox.hgrow="ALWAYS">
-                        <Button onAction="#previousPage" styleClass="icon-button">
+                        <Button fx:id="previousButton" onAction="#previousPage" styleClass="icon-button">
                             <graphic>
                                 <JabRefIconView glyph="PREVIOUS_LEFT"/>
                             </graphic>
@@ -57,7 +57,7 @@
                         <TextField fx:id="currentPage" minWidth="30.0" prefWidth="40.0"/>
                         <Label text="of"/>
                         <Label fx:id="maxPages"/>
-                        <Button onAction="#nextPage" styleClass="icon-button">
+                        <Button fx:id="nextButton" onAction="#nextPage" styleClass="icon-button">
                             <graphic>
                                 <JabRefIconView glyph="NEXT_RIGHT"/>
                             </graphic>
diff --git a/src/main/java/org/jabref/gui/documentviewer/DocumentViewerView.java b/src/main/java/org/jabref/gui/documentviewer/DocumentViewerView.java
@@ -2,15 +2,18 @@
 
 import javafx.event.ActionEvent;
 import javafx.fxml.FXML;
+import javafx.scene.control.Button;
 import javafx.scene.control.ButtonBar;
 import javafx.scene.control.ButtonType;
 import javafx.scene.control.ComboBox;
 import javafx.scene.control.Label;
 import javafx.scene.control.ScrollBar;
 import javafx.scene.control.TextField;
 import javafx.scene.control.ToggleButton;
+import javafx.scene.control.ToggleGroup;
 import javafx.scene.layout.BorderPane;
 import javafx.stage.Modality;
+import javafx.stage.Stage;
 
 import org.jabref.gui.StateManager;
 import org.jabref.gui.util.BaseDialog;
@@ -29,10 +32,13 @@ public class DocumentViewerView extends BaseDialog<Void> {
     @FXML private ScrollBar scrollBar;
     @FXML private ComboBox<LinkedFile> fileChoice;
     @FXML private BorderPane mainPane;
+    @FXML private ToggleGroup toggleGroupMode;
     @FXML private ToggleButton modeLive;
     @FXML private ToggleButton modeLock;
     @FXML private TextField currentPage;
     @FXML private Label maxPages;
+    @FXML private Button nextButton;
+    @FXML private Button previousButton;
 
     @Inject private StateManager stateManager;
     @Inject private TaskExecutor taskExecutor;
@@ -65,18 +71,22 @@ private void initialize() {
         setupModeButtons();
     }
 
-    private void setupModeButtons() throws RuntimeException {
-        viewModel.liveModeProperty().addListener((observable, oldValue, newValue) -> {
-            modeLive.setSelected(newValue);
+    private void setupModeButtons() {
+        // make sure that always one toggle is selected
+        toggleGroupMode.selectedToggleProperty().addListener((observable, oldToggle, newToggle) -> {
+            if (newToggle == null) {
+                oldToggle.setSelected(true);
+            }
         });
 
-        modeLive.setOnAction(event -> {
-            if (!viewModel.liveModeProperty().get()) {
+        modeLive.selectedProperty().addListener((observable, oldValue, newValue) -> {
+            if (newValue) {
                 viewModel.setLiveMode(true);
             }
         });
-        modeLock.setOnAction(event -> {
-            if (viewModel.liveModeProperty().get()) {
+
+        modeLock.selectedProperty().addListener((observable, oldValue, newValue) -> {
+            if (newValue) {
                 viewModel.setLiveMode(false);
             }
         });
@@ -92,6 +102,11 @@ private void setupPageControls() {
         viewModel.currentPageProperty().bindBidirectional(integerFormatter.valueProperty());
         currentPage.setTextFormatter(integerFormatter);
         maxPages.textProperty().bind(viewModel.maxPagesProperty().asString());
+        previousButton.setDisable(true);
+        viewModel.currentPageProperty().addListener((observable, oldValue, newValue) -> {
+            nextButton.setDisable(newValue == viewModel.maxPagesProperty().get());
+            previousButton.setDisable(newValue == 1);
+        });
     }
 
     private void setupFileChoice() {
@@ -103,8 +118,14 @@ private void setupFileChoice() {
                 (observable, oldValue, newValue) -> viewModel.switchToFile(newValue));
         // We always want that the first item is selected after a change
         // This also automatically selects the first file on the initial load
-        fileChoice.itemsProperty().addListener(
-                (observable, oldValue, newValue) -> fileChoice.getSelectionModel().selectFirst());
+        Stage stage = (Stage) getDialogPane().getScene().getWindow();
+        fileChoice.itemsProperty().addListener((observable, oldValue, newValue) -> {
+            if (newValue.isEmpty()) {
+                stage.close();
+            } else {
+                fileChoice.getSelectionModel().selectFirst();
+            }
+        });
         fileChoice.itemsProperty().bind(viewModel.filesProperty());
     }
 
@@ -119,8 +140,12 @@ private void setupViewer() {
         mainPane.setCenter(viewer);
     }
 
-    public void setLiveMode(boolean liveMode) {
-        modeLive.setSelected(liveMode);
+    public void disableLiveMode() {
+        modeLock.setSelected(true);
+    }
+
+    public void switchToFile(LinkedFile file) {
+        fileChoice.getSelectionModel().select(file);
     }
 
     public void gotoPage(int pageNumber) {
diff --git a/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java b/src/main/java/org/jabref/gui/documentviewer/DocumentViewerViewModel.java
@@ -4,8 +4,9 @@
 import java.nio.file.Path;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
 
-import javafx.application.Platform;
 import javafx.beans.property.BooleanProperty;
 import javafx.beans.property.IntegerProperty;
 import javafx.beans.property.ListProperty;
@@ -19,6 +20,7 @@
 
 import org.jabref.gui.AbstractViewModel;
 import org.jabref.gui.StateManager;
+import org.jabref.gui.util.UiTaskExecutor;
 import org.jabref.logic.util.io.FileUtil;
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.entry.LinkedFile;
@@ -38,7 +40,7 @@ public class DocumentViewerViewModel extends AbstractViewModel {
     private final PreferencesService preferencesService;
     private final ObjectProperty<DocumentViewModel> currentDocument = new SimpleObjectProperty<>();
     private final ListProperty<LinkedFile> files = new SimpleListProperty<>();
-    private final BooleanProperty liveMode = new SimpleBooleanProperty();
+    private final BooleanProperty liveMode = new SimpleBooleanProperty(true);
     private final ObjectProperty<Integer> currentPage = new SimpleObjectProperty<>();
     private final IntegerProperty maxPages = new SimpleIntegerProperty();
 
@@ -48,7 +50,7 @@ public DocumentViewerViewModel(StateManager stateManager, PreferencesService pre
 
         this.stateManager.getSelectedEntries().addListener((ListChangeListener<? super BibEntry>) c -> {
             // Switch to currently selected entry in live mode
-            if (isLiveMode()) {
+            if (liveMode.get()) {
                 setCurrentEntries(this.stateManager.getSelectedEntries());
             }
         });
@@ -61,7 +63,7 @@ public DocumentViewerViewModel(StateManager stateManager, PreferencesService pre
         });
 
         // we need to wrap this in run later so that the max pages number is correctly shown
-        Platform.runLater(() -> maxPages.bindBidirectional(
+        UiTaskExecutor.runInJavaFXThread(() -> maxPages.bind(
                 EasyBind.wrapNullable(currentDocument).selectProperty(DocumentViewModel::maxPagesProperty)));
         setCurrentEntries(this.stateManager.getSelectedEntries());
     }
@@ -78,10 +80,6 @@ public IntegerProperty maxPagesProperty() {
         return maxPages;
     }
 
-    private boolean isLiveMode() {
-        return liveMode.get();
-    }
-
     public ObjectProperty<DocumentViewModel> currentDocumentProperty() {
         return currentDocument;
     }
@@ -91,18 +89,13 @@ public ListProperty<LinkedFile> filesProperty() {
     }
 
     private void setCurrentEntries(List<BibEntry> entries) {
-        if (!entries.isEmpty()) {
-            BibEntry firstSelectedEntry = entries.getFirst();
-            setCurrentEntry(firstSelectedEntry);
-        }
-    }
-
-    private void setCurrentEntry(BibEntry entry) {
-        stateManager.getActiveDatabase().ifPresent(database -> {
-            List<LinkedFile> linkedFiles = entry.getFiles();
+        if (entries.isEmpty()) {
+            files.clear();
+        } 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));
-        });
+        }
     }
 
     private void setCurrentDocument(Path path) {
@@ -121,23 +114,28 @@ public void switchToFile(LinkedFile file) {
             stateManager.getActiveDatabase()
                         .flatMap(database -> file.findIn(database, preferencesService.getFilePreferences()))
                         .ifPresent(this::setCurrentDocument);
+            currentPage.set(1);
         }
     }
 
-    public BooleanProperty liveModeProperty() {
-        return liveMode;
-    }
-
     public void showPage(int pageNumber) {
-        currentPage.set(pageNumber);
+        if (pageNumber >= 1 && pageNumber <= maxPages.get()) {
+            currentPage.set(pageNumber);
+        } else {
+            currentPage.set(1);
+        }
     }
 
     public void showNextPage() {
-        currentPage.set(getCurrentPage() + 1);
+        if (getCurrentPage() < maxPages.get()) {
+            currentPage.set(getCurrentPage() + 1);
+        }
     }
 
     public void showPreviousPage() {
-        currentPage.set(getCurrentPage() - 1);
+        if (getCurrentPage() > 1) {
+            currentPage.set(getCurrentPage() - 1);
+        }
     }
 
     public void setLiveMode(boolean value) {
diff --git a/src/main/java/org/jabref/gui/documentviewer/ShowDocumentViewerAction.java b/src/main/java/org/jabref/gui/documentviewer/ShowDocumentViewerAction.java
@@ -11,14 +11,18 @@
 import static org.jabref.gui.actions.ActionHelper.needsEntriesSelected;
 
 public class ShowDocumentViewerAction extends SimpleCommand {
+    private final DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+    private DocumentViewerView documentViewerView;
 
     public ShowDocumentViewerAction(StateManager stateManager, PreferencesService preferences) {
         this.executable.bind(needsEntriesSelected(stateManager).and(ActionHelper.isFilePresentForSelectedEntry(stateManager, preferences)));
     }
 
     @Override
     public void execute() {
-        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
-        dialogService.showCustomDialog(new DocumentViewerView());
+        if (documentViewerView == null) {
+            documentViewerView = new DocumentViewerView();
+        }
+        dialogService.showCustomDialog(documentViewerView);
     }
 }
diff --git a/src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FulltextSearchResultsTab.java b/src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FulltextSearchResultsTab.java

Original file line number	Diff line number	Diff line change
`@@ -11,14 +11,18 @@`
`11`	`11`	`import static org.jabref.gui.actions.ActionHelper.needsEntriesSelected;`
`12`	`12`
`13`	`13`	`public class ShowDocumentViewerAction extends SimpleCommand {`
	`14`	`+ private final DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);`
	`15`	`+ private DocumentViewerView documentViewerView;`
`14`	`16`
`15`	`17`	`public ShowDocumentViewerAction(StateManager stateManager, PreferencesService preferences) {`
`16`	`18`	`this.executable.bind(needsEntriesSelected(stateManager).and(ActionHelper.isFilePresentForSelectedEntry(stateManager, preferences)));`
`17`	`19`	`}`
`18`	`20`
`19`	`21`	`@Override`
`20`	`22`	`public void execute() {`
`21`		`- DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);`
`22`		`- dialogService.showCustomDialog(new DocumentViewerView());`
	`23`	`+ if (documentViewerView == null) {`
	`24`	`+ documentViewerView = new DocumentViewerView();`
	`25`	`+ }`
	`26`	`+ dialogService.showCustomDialog(documentViewerView);`
`23`	`27`	`}`
`24`	`28`	`}`