Fix for application dialogs opening in wrong displays (#7273)

* [WIP] Fix for application dialogs opening in wrong displays * [WIP] Fix for application dialogs opening in wrong displays * [WIP] Fix for application dialogs opening in wrong displays * [WIP] Fix for application dialogs opening in wrong displays * Fix for application dialogs opening in wrong displays - Rename DialogService.show() method to showCustomDialog() - Rename showCustomDialog()'s argument - Add documentation to showCustomDialog method - Make createDialog and createDialogWithOptOut non static * Add change description to CHANGELOG.md * Drop unneeded test AboutActionTest * Document guidelines for creating a new dialog * Document guidelines for creating a new dialog
JabRef · Jan 2, 2021 · 0b4be8a · 0b4be8a
1 parent afdb194
commit 0b4be8a
Show file tree

Hide file tree

Showing 53 changed files with 227 additions and 96 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 - We fixed an issue with the style of highlighted check boxes while searching in preferences. [#7226](https://github.com/JabRef/jabref/issues/7226)
 - We fixed an issue where the option "Move file to file directory" was disabled in the entry editor for all files [#7194](https://github.com/JabRef/jabref/issues/7194)
+- We fixed an issue where application dialogs were opening in the wrong display when using multiple screens [#7273](https://github.com/JabRef/jabref/pull/7273)
 
 ### Removed
 

diff --git a/docs/getting-into-the-code/code-howtos.md b/docs/getting-into-the-code/code-howtos.md
@@ -398,3 +398,20 @@ All radio buttons that should be grouped together need to have a ToggleGroup def
 </VBox>
 ```
 
+### JavaFX Dialogs
+
+All dialogs should be displayed to the user via `DialogService` interface methods.
+`DialogService` provides methods to display various dialogs (including custom ones) to the user. 
+It also ensures the displayed dialog opens on the correct window via `initOwner()` (for cases where the user has multiple screens).
+The following code snippet demonstrates how a custom dialog is displayed to the user:
+
+```java
+dialogService.showCustomDialog(new DocumentViewerView());
+```
+
+If an instance of `DialogService` is unavailable within current class/scope in which the dialog needs to be displayed, 
+`DialogService` can be instantiated via the code snippet shown as follows:
+
+```java
+DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+```
diff --git a/src/main/java/org/jabref/gui/DialogService.java b/src/main/java/org/jabref/gui/DialogService.java
@@ -15,6 +15,7 @@
 import javafx.scene.control.DialogPane;
 import javafx.scene.control.TextInputDialog;
 
+import org.jabref.gui.util.BaseDialog;
 import org.jabref.gui.util.DirectoryDialogConfiguration;
 import org.jabref.gui.util.FileDialogConfiguration;
 import org.jabref.logic.l10n.Localization;
@@ -160,6 +161,13 @@ boolean showConfirmationDialogWithOptOutAndWait(String title, String content,
                                                     String okButtonLabel, String cancelButtonLabel,
                                                     String optOutMessage, Consumer<Boolean> optOutAction);
 
+    /**
+     * Shows a custom dialog without returning any results.
+     *
+     * @param dialog dialog to show
+     */
+    void showCustomDialog(BaseDialog<?> dialog);
+
     /**
      * This will create and display a new dialog of the specified
      * {@link Alert.AlertType} but with user defined buttons as optional
@@ -184,7 +192,7 @@ Optional<ButtonType> showCustomButtonDialogAndWait(Alert.AlertType type, String
      * @param dialog dialog to show
      * @param <R>    type of result
      */
-    <R> Optional<R> showCustomDialogAndWait(Dialog<R> dialog);
+    <R> Optional<R> showCustomDialogAndWait(javafx.scene.control.Dialog<R> dialog);
 
     /**
      * Constructs and shows a canceable {@link ProgressDialog}. Clicking cancel will cancel the underlying service and close the dialog

diff --git a/src/main/java/org/jabref/gui/EntryTypeViewModel.java b/src/main/java/org/jabref/gui/EntryTypeViewModel.java
@@ -155,7 +155,7 @@ public void runFetcherWorker() {
                 Optional<BibEntry> duplicate = new DuplicateCheck(Globals.entryTypesManager).containsDuplicate(libraryTab.getDatabase(), entry, libraryTab.getBibDatabaseContext().getMode());
                 if ((duplicate.isPresent())) {
                     DuplicateResolverDialog dialog = new DuplicateResolverDialog(entry, duplicate.get(), DuplicateResolverDialog.DuplicateResolverType.IMPORT_CHECK, libraryTab.getBibDatabaseContext(), stateManager);
-                    switch (dialog.showAndWait().orElse(DuplicateResolverDialog.DuplicateResolverResult.BREAK)) {
+                    switch (dialogService.showCustomDialogAndWait(dialog).orElse(DuplicateResolverDialog.DuplicateResolverResult.BREAK)) {
                         case KEEP_LEFT:
                             libraryTab.getDatabase().removeEntry(duplicate.get());
                             libraryTab.getDatabase().insertEntry(entry);

diff --git a/src/main/java/org/jabref/gui/JabRefDialogService.java b/src/main/java/org/jabref/gui/JabRefDialogService.java
@@ -36,6 +36,7 @@
 
 import org.jabref.gui.icon.IconTheme;
 import org.jabref.gui.util.BackgroundTask;
+import org.jabref.gui.util.BaseDialog;
 import org.jabref.gui.util.DirectoryDialogConfiguration;
 import org.jabref.gui.util.FileDialogConfiguration;
 import org.jabref.gui.util.ZipFileChooser;
@@ -78,17 +79,18 @@ public JabRefDialogService(Window mainWindow, Pane mainPane, PreferencesService
         JabRefDialogService.preferences = preferences;
     }
 
-    private static FXDialog createDialog(AlertType type, String title, String content) {
+    private FXDialog createDialog(AlertType type, String title, String content) {
         FXDialog alert = new FXDialog(type, title, true);
         preferences.getTheme().installCss(alert.getDialogPane().getScene());
         alert.setHeaderText(null);
         alert.setContentText(content);
         alert.getDialogPane().setMinHeight(Region.USE_PREF_SIZE);
+        alert.initOwner(mainWindow);
         return alert;
     }
 
-    private static FXDialog createDialogWithOptOut(AlertType type, String title, String content,
-                                                   String optOutMessage, Consumer<Boolean> optOutAction) {
+    private FXDialog createDialogWithOptOut(AlertType type, String title, String content,
+                                            String optOutMessage, Consumer<Boolean> optOutAction) {
         FXDialog alert = new FXDialog(type, title, true);
         // Need to force the alert to layout in order to grab the graphic as we are replacing the dialog pane with a custom pane
         alert.getDialogPane().applyCss();
@@ -117,6 +119,7 @@ protected Node createDetailsButton() {
         alert.setHeaderText(null);
         alert.setContentText(content);
         alert.getDialogPane().setMinHeight(Region.USE_PREF_SIZE);
+        alert.initOwner(mainWindow);
         return alert;
     }
 
@@ -136,6 +139,7 @@ public <T> Optional<T> showChoiceDialogAndWait(String title, String content, Str
         choiceDialog.setHeaderText(title);
         choiceDialog.setTitle(title);
         choiceDialog.setContentText(content);
+        choiceDialog.initOwner(mainWindow);
         preferences.getTheme().installCss(choiceDialog.getDialogPane().getScene());
         return choiceDialog.showAndWait();
     }
@@ -145,6 +149,7 @@ public Optional<String> showInputDialogAndWait(String title, String content) {
         TextInputDialog inputDialog = new TextInputDialog();
         inputDialog.setHeaderText(title);
         inputDialog.setContentText(content);
+        inputDialog.initOwner(mainWindow);
         preferences.getTheme().installCss(inputDialog.getDialogPane().getScene());
         return inputDialog.showAndWait();
     }
@@ -154,6 +159,7 @@ public Optional<String> showInputDialogWithDefaultAndWait(String title, String c
         TextInputDialog inputDialog = new TextInputDialog(defaultValue);
         inputDialog.setHeaderText(title);
         inputDialog.setContentText(content);
+        inputDialog.initOwner(mainWindow);
         preferences.getTheme().installCss(inputDialog.getDialogPane().getScene());
         return inputDialog.showAndWait();
     }
@@ -181,6 +187,7 @@ public void showErrorDialogAndWait(String message, Throwable exception) {
         ExceptionDialog exceptionDialog = new ExceptionDialog(exception);
         exceptionDialog.getDialogPane().setMaxWidth(mainWindow.getWidth() / 2);
         exceptionDialog.setHeaderText(message);
+        exceptionDialog.initOwner(mainWindow);
         preferences.getTheme().installCss(exceptionDialog.getDialogPane().getScene());
         exceptionDialog.showAndWait();
     }
@@ -190,6 +197,7 @@ public void showErrorDialogAndWait(String title, String content, Throwable excep
         ExceptionDialog exceptionDialog = new ExceptionDialog(exception);
         exceptionDialog.setHeaderText(title);
         exceptionDialog.setContentText(content);
+        exceptionDialog.initOwner(mainWindow);
         preferences.getTheme().installCss(exceptionDialog.getDialogPane().getScene());
         exceptionDialog.showAndWait();
     }
@@ -259,12 +267,14 @@ public Optional<ButtonType> showCustomDialogAndWait(String title, DialogPane con
         alert.getButtonTypes().setAll(buttonTypes);
         alert.getDialogPane().setMinHeight(Region.USE_PREF_SIZE);
         alert.setResizable(true);
+        alert.initOwner(mainWindow);
         preferences.getTheme().installCss(alert.getDialogPane().getScene());
         return alert.showAndWait();
     }
 
     @Override
-    public <R> Optional<R> showCustomDialogAndWait(Dialog<R> dialog) {
+    public <R> Optional<R> showCustomDialogAndWait(javafx.scene.control.Dialog<R> dialog) {
+        dialog.initOwner(mainWindow);
         return dialog.showAndWait();
     }
 
@@ -285,6 +295,7 @@ public <V> void showProgressDialog(String title, String content, Task<V> task) {
             progressDialog.close();
         });
         preferences.getTheme().installCss(progressDialog.getDialogPane().getScene());
+        progressDialog.initOwner(mainWindow);
         progressDialog.show();
     }
 
@@ -307,6 +318,7 @@ public <V> Optional<ButtonType> showBackgroundProgressDialogAndWait(String title
         alert.getButtonTypes().setAll(ButtonType.YES, ButtonType.CANCEL);
         alert.getDialogPane().setMinHeight(Region.USE_PREF_SIZE);
         alert.setResizable(true);
+        alert.initOwner(mainWindow);
         preferences.getTheme().installCss(alert.getDialogPane().getScene());
 
         stateManager.getAnyTaskRunning().addListener((observable, oldValue, newValue) -> {
@@ -316,9 +328,7 @@ public <V> Optional<ButtonType> showBackgroundProgressDialogAndWait(String title
             }
         });
 
-        Dialog<ButtonType> dialog = alert::showAndWait;
-
-        return showCustomDialogAndWait(dialog);
+        return alert.showAndWait();
     }
 
     @Override
@@ -385,4 +395,10 @@ public Optional<Path> showFileOpenFromArchiveDialog(Path archivePath) throws IOE
             throw new IOException("Could not instantiate ZIP-archive reader.", exc);
         }
     }
+
+    @Override
+    public void showCustomDialog(BaseDialog<?> aboutDialogView) {
+        aboutDialogView.initOwner(mainWindow);
+        aboutDialogView.show();
+    }
 }
diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java
@@ -1089,7 +1089,7 @@ private void addImportedEntries(final LibraryTab panel, final ParserResult parse
         cleanup.doPostCleanup(parserResult.getDatabase().getEntries());
         ImportEntriesDialog dialog = new ImportEntriesDialog(panel.getBibDatabaseContext(), task);
         dialog.setTitle(Localization.lang("Import"));
-        dialog.showAndWait();
+        dialogService.showCustomDialogAndWait(dialog);
     }
 
     public FileHistoryMenu getFileHistory() {

diff --git a/src/main/java/org/jabref/gui/auximport/NewSubLibraryAction.java b/src/main/java/org/jabref/gui/auximport/NewSubLibraryAction.java
@@ -1,9 +1,12 @@
 package org.jabref.gui.auximport;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.JabRefFrame;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 
+import com.airhacks.afterburner.injection.Injector;
+
 import static org.jabref.gui.actions.ActionHelper.needsDatabase;
 
 /**
@@ -21,7 +24,7 @@ public NewSubLibraryAction(JabRefFrame jabRefFrame, StateManager stateManager) {
 
     @Override
     public void execute() {
-        FromAuxDialog dialog = new FromAuxDialog(jabRefFrame);
-        dialog.showAndWait();
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+        dialogService.showCustomDialogAndWait(new FromAuxDialog(jabRefFrame));
     }
 }
diff --git a/src/main/java/org/jabref/gui/bibtexextractor/ExtractBibtexAction.java b/src/main/java/org/jabref/gui/bibtexextractor/ExtractBibtexAction.java
@@ -1,8 +1,11 @@
 package org.jabref.gui.bibtexextractor;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 
+import com.airhacks.afterburner.injection.Injector;
+
 import static org.jabref.gui.actions.ActionHelper.needsDatabase;
 
 public class ExtractBibtexAction extends SimpleCommand {
@@ -13,7 +16,7 @@ public ExtractBibtexAction(StateManager stateManager) {
 
     @Override
     public void execute() {
-        ExtractBibtexDialog dlg = new ExtractBibtexDialog();
-        dlg.showAndWait();
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+        dialogService.showCustomDialogAndWait(new ExtractBibtexDialog());
     }
 }
diff --git a/src/main/java/org/jabref/gui/citationkeypattern/CitationKeyPatternAction.java b/src/main/java/org/jabref/gui/citationkeypattern/CitationKeyPatternAction.java
@@ -1,9 +1,12 @@
 package org.jabref.gui.citationkeypattern;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.JabRefFrame;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 
+import com.airhacks.afterburner.injection.Injector;
+
 import static org.jabref.gui.actions.ActionHelper.needsDatabase;
 
 public class CitationKeyPatternAction extends SimpleCommand {
@@ -18,6 +21,7 @@ public CitationKeyPatternAction(JabRefFrame frame, StateManager stateManager) {
 
     @Override
     public void execute() {
-        new CitationKeyPatternDialog(frame.getCurrentLibraryTab()).showAndWait();
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+        dialogService.showCustomDialogAndWait(new CitationKeyPatternDialog(frame.getCurrentLibraryTab()));
     }
 }
diff --git a/src/main/java/org/jabref/gui/cleanup/CleanupAction.java b/src/main/java/org/jabref/gui/cleanup/CleanupAction.java
@@ -56,10 +56,13 @@ public void execute() {
         isCanceled = false;
         modifiedEntriesCount = 0;
 
-        Optional<CleanupPreset> chosenPreset = new CleanupDialog(
+        CleanupDialog cleanupDialog = new CleanupDialog(
                 stateManager.getActiveDatabase().get(),
                 preferences.getCleanupPreset(),
-                preferences.getFilePreferences()).showAndWait();
+                preferences.getFilePreferences()
+        );
+
+        Optional<CleanupPreset> chosenPreset = dialogService.showCustomDialogAndWait(cleanupDialog);
 
         chosenPreset.ifPresent(preset -> {
             if (preset.isRenamePDFActive() && preferences.getAutoLinkPreferences().shouldAskAutoNamingPdfs()) {

diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangePane.java b/src/main/java/org/jabref/gui/collab/DatabaseChangePane.java
@@ -4,10 +4,12 @@
 
 import javafx.scene.Node;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.icon.IconTheme;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.model.database.BibDatabaseContext;
 
+import com.airhacks.afterburner.injection.Injector;
 import org.controlsfx.control.NotificationPane;
 import org.controlsfx.control.action.Action;
 
@@ -33,9 +35,8 @@ private void onDatabaseChanged(List<DatabaseChangeViewModel> changes) {
                     this.hide();
                 }),
                 new Action(Localization.lang("Review changes"), event -> {
-                    ChangeDisplayDialog changeDialog = new ChangeDisplayDialog(database, changes);
-                    changeDialog.showAndWait();
-
+                    DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+                    dialogService.showCustomDialogAndWait(new ChangeDisplayDialog(database, changes));
                     this.hide();
                 }));
         this.show();

diff --git a/src/main/java/org/jabref/gui/contentselector/ManageContentSelectorAction.java b/src/main/java/org/jabref/gui/contentselector/ManageContentSelectorAction.java
@@ -1,10 +1,13 @@
 package org.jabref.gui.contentselector;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.JabRefFrame;
 import org.jabref.gui.LibraryTab;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 
+import com.airhacks.afterburner.injection.Injector;
+
 import static org.jabref.gui.actions.ActionHelper.needsDatabase;
 
 public class ManageContentSelectorAction extends SimpleCommand {
@@ -19,7 +22,8 @@ public ManageContentSelectorAction(JabRefFrame jabRefFrame, StateManager stateMa
 
     @Override
     public void execute() {
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
         LibraryTab libraryTab = jabRefFrame.getCurrentLibraryTab();
-        new ContentSelectorDialogView(libraryTab).showAndWait();
+        dialogService.showCustomDialogAndWait(new ContentSelectorDialogView(libraryTab));
     }
 }
diff --git a/src/main/java/org/jabref/gui/copyfiles/CopyFilesAction.java b/src/main/java/org/jabref/gui/copyfiles/CopyFilesAction.java
@@ -35,8 +35,7 @@ private void showDialog(List<CopyFilesResultItemViewModel> data) {
             dialogService.showInformationDialogAndWait(Localization.lang("Copy linked files to folder..."), Localization.lang("No linked files found for export."));
             return;
         }
-        CopyFilesDialogView dialog = new CopyFilesDialogView(new CopyFilesResultListDependency(data));
-        dialog.showAndWait();
+        dialogService.showCustomDialogAndWait(new CopyFilesDialogView(new CopyFilesResultListDependency(data)));
     }
 
     @Override

diff --git a/src/main/java/org/jabref/gui/customentrytypes/CustomizeEntryAction.java b/src/main/java/org/jabref/gui/customentrytypes/CustomizeEntryAction.java
@@ -1,10 +1,13 @@
 package org.jabref.gui.customentrytypes;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.SimpleCommand;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntryTypesManager;
 
+import com.airhacks.afterburner.injection.Injector;
+
 import static org.jabref.gui.actions.ActionHelper.needsDatabase;
 
 public class CustomizeEntryAction extends SimpleCommand {
@@ -21,7 +24,7 @@ public CustomizeEntryAction(StateManager stateManager, BibEntryTypesManager entr
     @Override
     public void execute() {
         BibDatabaseContext database = stateManager.getActiveDatabase().orElseThrow(() -> new NullPointerException("Database null"));
-        CustomizeEntryTypeDialogView dialog = new CustomizeEntryTypeDialogView(database, entryTypesManager);
-        dialog.showAndWait();
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+        dialogService.showCustomDialogAndWait(new CustomizeEntryTypeDialogView(database, entryTypesManager));
     }
 }
diff --git a/src/main/java/org/jabref/gui/customizefields/SetupGeneralFieldsAction.java b/src/main/java/org/jabref/gui/customizefields/SetupGeneralFieldsAction.java
@@ -1,11 +1,15 @@
 package org.jabref.gui.customizefields;
 
+import org.jabref.gui.DialogService;
 import org.jabref.gui.actions.SimpleCommand;
 
+import com.airhacks.afterburner.injection.Injector;
+
 public class SetupGeneralFieldsAction extends SimpleCommand {
 
     @Override
     public void execute() {
-        new CustomizeGeneralFieldsDialogView().showAndWait();
+        DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
+        dialogService.showCustomDialogAndWait(new CustomizeGeneralFieldsDialogView());
     }
 }