Enhanced message log (#4815)

* refactored JabRefFrame.output and FXDialogService.notify to only use FXDialogService.notify * added logging to notifications and fixed problem with logger in JabRefGUI * moved copying of logevents to GuiAppender * directly use dialogService in BasePanel * changed constructor to avoid NPE * refactored code so that DialogService.notify does not need DefaultTaskExecutor.runInJavaFXThread * replaced old swing dialog * replaced another swing dialog
JabRef · Apr 3, 2019 · a273eb1 · a273eb1
1 parent 47772fd
commit a273eb1
Show file tree

Hide file tree

Showing 28 changed files with 126 additions and 144 deletions.
diff --git a/src/main/java/org/jabref/gui/BasePanel.java b/src/main/java/org/jabref/gui/BasePanel.java
@@ -250,7 +250,7 @@ public JabRefFrame frame() {
     }
 
     public void output(String s) {
-        frame.output(s);
+        dialogService.notify(s);
     }
 
     private void setupActions() {
@@ -441,7 +441,7 @@ private void delete(boolean cut, List<BibEntry> entries) {
         getUndoManager().addEdit(compound);
 
         markBaseChanged();
-        frame.output(formatOutputMessage(cut ? Localization.lang("Cut") : Localization.lang("Deleted"), entries.size()));
+        this.output(formatOutputMessage(cut ? Localization.lang("Cut") : Localization.lang("Deleted"), entries.size()));
 
         // prevent the main table from loosing focus
         mainTable.requestFocus();
@@ -569,13 +569,12 @@ private void copyKeyAndTitle() {
     }
 
     private void openExternalFile() {
+        final List<BibEntry> selectedEntries = mainTable.getSelectedEntries();
+        if (selectedEntries.size() != 1) {
+            output(Localization.lang("This operation requires exactly one item to be selected."));
+            return;
+        }
         JabRefExecutorService.INSTANCE.execute(() -> {
-            final List<BibEntry> selectedEntries = mainTable.getSelectedEntries();
-            if (selectedEntries.size() != 1) {
-                output(Localization.lang("This operation requires exactly one item to be selected."));
-                return;
-            }
-
             final BibEntry entry = selectedEntries.get(0);
             if (!entry.hasField(FieldName.FILE)) {
                 // no bibtex field
@@ -1292,10 +1291,10 @@ public void action() {
             try {
                 getUndoManager().undo();
                 markBaseChanged();
-                frame.output(Localization.lang("Undo"));
+                output(Localization.lang("Undo"));
             } catch (CannotUndoException ex) {
                 LOGGER.warn("Nothing to undo", ex);
-                frame.output(Localization.lang("Nothing to undo") + '.');
+                output(Localization.lang("Nothing to undo") + '.');
             }
 
             markChangedOrUnChanged();
@@ -1363,9 +1362,9 @@ public void action() {
             try {
                 getUndoManager().redo();
                 markBaseChanged();
-                frame.output(Localization.lang("Redo"));
+                output(Localization.lang("Redo"));
             } catch (CannotRedoException ex) {
-                frame.output(Localization.lang("Nothing to redo") + '.');
+                output(Localization.lang("Nothing to redo") + '.');
             }
 
             markChangedOrUnChanged();

diff --git a/src/main/java/org/jabref/gui/FXDialogService.java b/src/main/java/org/jabref/gui/FXDialogService.java
@@ -24,21 +24,27 @@
 import javafx.scene.control.ChoiceDialog;
 import javafx.scene.control.DialogPane;
 import javafx.scene.control.TextInputDialog;
+import javafx.scene.layout.Pane;
 import javafx.scene.layout.Region;
 import javafx.stage.DirectoryChooser;
 import javafx.stage.FileChooser;
 import javafx.stage.Stage;
 import javafx.stage.Window;
+import javafx.util.Duration;
 
-import org.jabref.JabRefGUI;
 import org.jabref.gui.icon.IconTheme;
 import org.jabref.gui.util.DirectoryDialogConfiguration;
 import org.jabref.gui.util.FileDialogConfiguration;
 import org.jabref.gui.util.ZipFileChooser;
 import org.jabref.logic.l10n.Localization;
 
+import com.jfoenix.controls.JFXSnackbar;
+import com.jfoenix.controls.JFXSnackbar.SnackbarEvent;
+import com.jfoenix.controls.JFXSnackbarLayout;
 import org.controlsfx.dialog.ExceptionDialog;
 import org.controlsfx.dialog.ProgressDialog;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * This class provides methods to create default
@@ -51,7 +57,11 @@
  */
 public class FXDialogService implements DialogService {
 
+    private static final Duration TOAST_MESSAGE_DISPLAY_TIME = Duration.millis(3000);
+    private static final Logger LOGGER = LoggerFactory.getLogger(FXDialogService.class);
+
     private final Window mainWindow;
+    private final JFXSnackbar statusLine;
 
     /**
      * @deprecated try not to initialize a new dialog service but reuse the one constructed in {@link org.jabref.gui.JabRefFrame}.
@@ -63,6 +73,12 @@ public FXDialogService() {
 
     public FXDialogService(Window mainWindow) {
         this.mainWindow = mainWindow;
+        this.statusLine = new JFXSnackbar();
+    }
+
+    public FXDialogService(Window mainWindow, Pane mainPane) {
+        this.mainWindow = mainWindow;
+        this.statusLine = new JFXSnackbar(mainPane);
     }
 
     private static FXDialog createDialog(AlertType type, String title, String content) {
@@ -253,7 +269,8 @@ public <V> void showProgressDialogAndWait(String title, String content, Task<V>
 
     @Override
     public void notify(String message) {
-        JabRefGUI.getMainFrame().output(message);
+        LOGGER.info(message);
+        statusLine.fireEvent(new SnackbarEvent(new JFXSnackbarLayout(message), TOAST_MESSAGE_DISPLAY_TIME, null));
     }
 
     @Override

diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java
@@ -50,7 +50,6 @@
 import javafx.scene.layout.Pane;
 import javafx.scene.layout.Priority;
 import javafx.stage.Stage;
-import javafx.util.Duration;
 
 import org.jabref.Globals;
 import org.jabref.JabRefExecutorService;
@@ -132,9 +131,6 @@
 import org.jabref.preferences.LastFocusedTabPreferences;
 
 import com.google.common.eventbus.Subscribe;
-import com.jfoenix.controls.JFXSnackbar;
-import com.jfoenix.controls.JFXSnackbar.SnackbarEvent;
-import com.jfoenix.controls.JFXSnackbarLayout;
 import org.eclipse.fx.ui.controls.tabpane.DndTabPane;
 import org.eclipse.fx.ui.controls.tabpane.DndTabPaneFactory;
 import org.fxmisc.easybind.EasyBind;
@@ -151,12 +147,11 @@ public class JabRefFrame extends BorderPane {
     public static final String FRAME_TITLE = "JabRef";
 
     private static final Logger LOGGER = LoggerFactory.getLogger(JabRefFrame.class);
-    private static final Duration TOAST_MESSAGE_DISPLAY_TIME = Duration.millis(3000);
 
     private final SplitPane splitPane = new SplitPane();
     private final JabRefPreferences prefs = Globals.prefs;
     private final GlobalSearchBar globalSearchBar = new GlobalSearchBar(this);
-    private final JFXSnackbar statusLine = new JFXSnackbar(this);
+
     private final ProgressBar progressBar = new ProgressBar();
     private final FileHistoryMenu fileHistory = new FileHistoryMenu(prefs, this);
 
@@ -182,7 +177,7 @@ public class JabRefFrame extends BorderPane {
 
     public JabRefFrame(Stage mainStage) {
         this.mainStage = mainStage;
-        this.dialogService = new FXDialogService(mainStage);
+        this.dialogService = new FXDialogService(mainStage, this);
         init();
     }
 
@@ -962,16 +957,6 @@ public void addParserResult(ParserResult pr, boolean focusPanel) {
         }
     }
 
-    /**
-     * Displays the given message at the bottom of the main frame
-     *
-     * @deprecated use {@link DialogService#notify(String)} instead. However, do not remove this method, it's called from the dialogService
-     */
-    @Deprecated
-    public void output(final String message) {
-        DefaultTaskExecutor.runInJavaFXThread(() -> statusLine.fireEvent(new SnackbarEvent(new JFXSnackbarLayout(message), TOAST_MESSAGE_DISPLAY_TIME, null)));
-    }
-
     private void initActions() {
         /*
         openDatabaseOnlyActions.clear();
@@ -1279,7 +1264,7 @@ private boolean confirmClose(BasePanel panel) {
                     return true;
                 }
                 // The action was either canceled or unsuccessful.
-                output(Localization.lang("Unable to save library"));
+                dialogService.notify(Localization.lang("Unable to save library"));
             } catch (Throwable ex) {
                 LOGGER.error("A problem occurred when trying to save the file", ex);
                 dialogService.showErrorDialogAndWait(Localization.lang("Save library"), Localization.lang("Could not save file."), ex);
@@ -1321,7 +1306,7 @@ private void removeTab(BasePanel panel) {
             panel.cleanUp();
             tabbedPane.getTabs().remove(getTab(panel));
             setWindowTitle();
-            output(Localization.lang("Closed library") + '.');
+            dialogService.notify(Localization.lang("Closed library") + '.');
             // update tab titles
             updateAllTabTitles();
         });

diff --git a/src/main/java/org/jabref/gui/actions/CleanupAction.java b/src/main/java/org/jabref/gui/actions/CleanupAction.java
@@ -71,7 +71,7 @@ public void init() {
             isCanceled = true;
             return;
         }
-        panel.output(Localization.lang("Doing a cleanup for %0 entries...",
+        dialogService.notify(Localization.lang("Doing a cleanup for %0 entries...",
                 Integer.toString(panel.getSelectedEntries().size())));
     }
 
@@ -115,7 +115,7 @@ private void showResults() {
                 message = Localization.lang("%0 entries needed a clean up", Integer.toString(modifiedEntriesCount));
                 break;
         }
-        panel.output(message);
+        dialogService.notify(message);
     }
 
     private void cleanup(CleanupPreset cleanupPreset) {

diff --git a/src/main/java/org/jabref/gui/actions/CopyBibTeXKeyAndLinkAction.java b/src/main/java/org/jabref/gui/actions/CopyBibTeXKeyAndLinkAction.java
@@ -36,7 +36,7 @@ public void action() throws Exception {
             List<BibEntry> entriesWithKey = entries.stream().filter(BibEntry::hasCiteKey).collect(Collectors.toList());
 
             if (entriesWithKey.isEmpty()) {
-                JabRefGUI.getMainFrame().output(Localization.lang("None of the selected entries have BibTeX keys."));
+                JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("None of the selected entries have BibTeX keys."));
                 return;
             }
 
@@ -53,9 +53,9 @@ public void action() throws Exception {
             int toCopy = entries.size();
             if (copied == toCopy) {
                 // All entries had keys.
-                JabRefGUI.getMainFrame().output((entries.size() > 1 ? Localization.lang("Copied keys") : Localization.lang("Copied key")) + '.');
+                JabRefGUI.getMainFrame().getDialogService().notify((entries.size() > 1 ? Localization.lang("Copied keys") : Localization.lang("Copied key")) + '.');
             } else {
-                JabRefGUI.getMainFrame().output(Localization.lang("Warning: %0 out of %1 entries have undefined BibTeX key.",
+                JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("Warning: %0 out of %1 entries have undefined BibTeX key.",
                         Long.toString(toCopy - copied), Integer.toString(toCopy)));
             }
         }

diff --git a/src/main/java/org/jabref/gui/actions/CopyDoiUrlAction.java b/src/main/java/org/jabref/gui/actions/CopyDoiUrlAction.java
@@ -27,9 +27,9 @@ public void execute() {
         Optional<String> urlOptional = DOI.parse(identifier).map(DOI::getURIAsASCIIString);
         if (urlOptional.isPresent()) {
             Globals.clipboardManager.setContent(urlOptional.get());
-            JabRefGUI.getMainFrame().output(Localization.lang("The link has been copied to the clipboard."));
+            JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("The link has been copied to the clipboard."));
         } else {
-            JabRefGUI.getMainFrame().output(Localization.lang("Invalid DOI: '%0'.", identifier));
+            JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("Invalid DOI: '%0'.", identifier));
         }
     }
 }
diff --git a/src/main/java/org/jabref/gui/actions/GenerateBibtexKeyAction.java b/src/main/java/org/jabref/gui/actions/GenerateBibtexKeyAction.java
@@ -33,7 +33,7 @@ public void init() {
                                                    Localization.lang("First select the entries you want keys to be generated for."));
             return;
         }
-        basePanel.output(formatOutputMessage(Localization.lang("Generating BibTeX key for"), entries.size()));
+        dialogService.notify(formatOutputMessage(Localization.lang("Generating BibTeX key for"), entries.size()));
     }
 
     public static boolean confirmOverwriteKeys(DialogService dialogService) {
@@ -87,7 +87,7 @@ private void generateKeys() {
         }
 
         basePanel.markBaseChanged();
-        basePanel.output(formatOutputMessage(Localization.lang("Generated BibTeX key for"), entries.size()));
+        dialogService.notify(formatOutputMessage(Localization.lang("Generated BibTeX key for"), entries.size()));
     }
 
     private String formatOutputMessage(String start, int count) {

diff --git a/src/main/java/org/jabref/gui/actions/LookupIdentifierAction.java b/src/main/java/org/jabref/gui/actions/LookupIdentifierAction.java
@@ -12,6 +12,7 @@
 import org.jabref.gui.undo.NamedCompound;
 import org.jabref.gui.undo.UndoableFieldChange;
 import org.jabref.gui.util.BackgroundTask;
+import org.jabref.gui.util.DefaultTaskExecutor;
 import org.jabref.logic.importer.FetcherException;
 import org.jabref.logic.importer.IdFetcher;
 import org.jabref.logic.l10n.Localization;
@@ -39,7 +40,7 @@ public LookupIdentifierAction(JabRefFrame frame, IdFetcher<T> fetcher) {
     public void execute() {
         try {
             BackgroundTask.wrap(this::lookupIdentifiers)
-                          .onSuccess(frame::output)
+                          .onSuccess(frame.getDialogService()::notify)
                           .executeWith(Globals.TASK_EXECUTOR);
         } catch (Exception e) {
             LOGGER.error("Problem running ID Worker", e);
@@ -84,8 +85,9 @@ private String lookupIdentifiers() {
         int foundCount = 0;
         for (BibEntry bibEntry : bibEntries) {
             count++;
-            frame.output(Localization.lang("Looking up %0... - entry %1 out of %2 - found %3",
-                    fetcher.getIdentifierName(), Integer.toString(count), totalCount, Integer.toString(foundCount)));
+            final String statusMessage = Localization.lang("Looking up %0... - entry %1 out of %2 - found %3",
+                    fetcher.getIdentifierName(), Integer.toString(count), totalCount, Integer.toString(foundCount));
+            DefaultTaskExecutor.runInJavaFXThread(() -> frame.getDialogService().notify(statusMessage));
             Optional<T> identifier = Optional.empty();
             try {
                 identifier = fetcher.findIdentifier(bibEntry);
@@ -97,8 +99,9 @@ private String lookupIdentifiers() {
                 if (fieldChange.isPresent()) {
                     namedCompound.addEdit(new UndoableFieldChange(fieldChange.get()));
                     foundCount++;
-                    frame.output(Localization.lang("Looking up %0... - entry %1 out of %2 - found %3",
-                            Integer.toString(count), totalCount, Integer.toString(foundCount)));
+                    final String nextStatusMessage = Localization.lang("Looking up %0... - entry %1 out of %2 - found %3",
+                            fetcher.getIdentifierName(), Integer.toString(count), totalCount, Integer.toString(foundCount));
+                    DefaultTaskExecutor.runInJavaFXThread(() -> frame.getDialogService().notify(nextStatusMessage));
                 }
             }
         }

diff --git a/src/main/java/org/jabref/gui/actions/NewDatabaseAction.java b/src/main/java/org/jabref/gui/actions/NewDatabaseAction.java
@@ -24,6 +24,6 @@ public void execute() {
         BibDatabaseContext bibDatabaseContext = new BibDatabaseContext(new Defaults(BibDatabaseMode.BIBTEX));
         bibDatabaseContext.setMode(mode);
         jabRefFrame.addTab(bibDatabaseContext, true);
-        jabRefFrame.output(Localization.lang("New %0 library created.", mode.getFormattedName()));
+        jabRefFrame.getDialogService().notify(Localization.lang("New %0 library created.", mode.getFormattedName()));
     }
 }
diff --git a/src/main/java/org/jabref/gui/actions/WriteXMPAction.java b/src/main/java/org/jabref/gui/actions/WriteXMPAction.java
@@ -92,7 +92,7 @@ public void init() {
         }
         optionsDialog.open();
 
-        basePanel.output(Localization.lang("Writing XMP-metadata..."));
+        dialogService.notify(Localization.lang("Writing XMP-metadata..."));
     }
 
     private void writeXMP() {
@@ -162,7 +162,7 @@ private void writeXMP() {
             return;
         }
 
-        basePanel.output(Localization.lang("Finished writing XMP for %0 file (%1 skipped, %2 errors).",
+        dialogService.notify(Localization.lang("Finished writing XMP for %0 file (%1 skipped, %2 errors).",
                 String.valueOf(entriesChanged), String.valueOf(skipped), String.valueOf(errors)));
     }
 

diff --git a/src/main/java/org/jabref/gui/desktop/JabRefDesktop.java b/src/main/java/org/jabref/gui/desktop/JabRefDesktop.java
@@ -10,8 +10,6 @@
 import java.util.Optional;
 import java.util.regex.Pattern;
 
-import javax.swing.JOptionPane;
-
 import org.jabref.Globals;
 import org.jabref.JabRefGUI;
 import org.jabref.gui.desktop.os.DefaultDesktop;
@@ -201,12 +199,8 @@ public static void openBrowserShowPopup(String url) {
             String couldNotOpenBrowser = Localization.lang("Could not open browser.");
             String openManually = Localization.lang("Please open %0 manually.", url);
             String copiedToClipboard = Localization.lang("The link has been copied to the clipboard.");
-            JabRefGUI.getMainFrame().output(couldNotOpenBrowser);
-            JOptionPane.showMessageDialog(null,
-                                          couldNotOpenBrowser + "\n" + openManually + "\n" +
-                                                copiedToClipboard,
-                                          couldNotOpenBrowser,
-                                          JOptionPane.ERROR_MESSAGE);
+            JabRefGUI.getMainFrame().getDialogService().notify(couldNotOpenBrowser);
+            JabRefGUI.getMainFrame().getDialogService().showErrorDialogAndWait(couldNotOpenBrowser, couldNotOpenBrowser + "\n" + openManually + "\n" + copiedToClipboard);
         }
     }
 
@@ -239,19 +233,17 @@ public static void openConsole(File file) throws IOException {
                 // replace the placeholder if used
                 String commandLoggingText = command.replace("%DIR", absolutePath);
 
-                JabRefGUI.getMainFrame().output(Localization.lang("Executing command \"%0\"...", commandLoggingText));
+                JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("Executing command \"%0\"...", commandLoggingText));
                 LOGGER.info("Executing command \"" + commandLoggingText + "\"...");
 
                 try {
                     new ProcessBuilder(subcommands).start();
                 } catch (IOException exception) {
                     LOGGER.error("Open console", exception);
 
-                    JOptionPane.showMessageDialog(null,
-                                                  Localization.lang("Error occured while executing the command \"%0\".", commandLoggingText),
-                                                  Localization.lang("Open console") + " - " + Localization.lang("Error"),
-                                                  JOptionPane.ERROR_MESSAGE);
-                    JabRefGUI.getMainFrame().output(null);
+                    JabRefGUI.getMainFrame().getDialogService().showErrorDialogAndWait(
+                            Localization.lang("Open console") + " - " + Localization.lang("Error",
+                            Localization.lang("Error occured while executing the command \"%0\".", commandLoggingText)));
                 }
             }
         }