Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enhanced message log #4815

Merged
merged 13 commits into from
Apr 3, 2019
Merged

Enhanced message log #4815

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3ab1bd7
refactored JabRefFrame.output and FXDialogService.notify to only use …
r0light Mar 27, 2019
577de6a
added logging to notifications and fixed problem with logger in JabRe…
r0light Mar 27, 2019
7e20650
moved copying of logevents to GuiAppender
r0light Mar 27, 2019
23362fa
directly use dialogService in BasePanel
r0light Mar 28, 2019
7995593
Merge branch 'master' into enhancedMessageLog
r0light Mar 28, 2019
deff466
changed constructor to avoid NPE
r0light Mar 28, 2019
e5cb625
Merge branch 'enhancedMessageLog' of github.com:r0light/jabref into e…
r0light Mar 28, 2019
7ccf914
Merge branch 'master' into enhancedMessageLog
r0light Mar 29, 2019
67e33e1
Merge branch 'master' into enhancedMessageLog
r0light Apr 2, 2019
0444b6a
refactored code so that DialogService.notify does not need DefaultTas…
r0light Apr 2, 2019
f2a31d0
replaced old swing dialog
r0light Apr 2, 2019
d87c7f6
replaced another swing dialog
r0light Apr 2, 2019
ea3e786
Merge branch 'master' into enhancedMessageLog
r0light Apr 3, 2019
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
12 changes: 6 additions & 6 deletions src/main/java/org/jabref/gui/BasePanel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ public JabRefFrame frame() {
}

public void output(String s) {
frame.output(s);
dialogService.notify(s);
}

private void setupActions() {
Expand Down Expand Up @@ -442,7 +442,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();
Expand Down Expand Up @@ -1326,10 +1326,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();
Expand Down Expand Up @@ -1397,9 +1397,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();
Expand Down
24 changes: 21 additions & 3 deletions src/main/java/org/jabref/gui/FXDialogService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,28 @@
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.DefaultTaskExecutor;
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
Expand All @@ -51,18 +58,28 @@
*/
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}.
*/
@Deprecated
public FXDialogService() {
this(null);
this(null, null);
Copy link
Member

Choose a reason for hiding this comment

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

The null argument will lead to a NPE below in notify. Leaving it as this(null) should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will fix it in the next commit.

}

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) {
Expand Down Expand Up @@ -253,7 +270,8 @@ public <V> void showProgressDialogAndWait(String title, String content, Task<V>

@Override
public void notify(String message) {
JabRefGUI.getMainFrame().output(message);
LOGGER.info(message);
DefaultTaskExecutor.runInJavaFXThread(() -> statusLine.fireEvent(new SnackbarEvent(new JFXSnackbarLayout(message), TOAST_MESSAGE_DISPLAY_TIME, null)));
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the runInJavaFXThread. This is a leftover when we had a lot of interaction between Swing and JavaFX code.

Copy link
Member

@Siedlerchr Siedlerchr Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this method is still sometimes called from background threads therefore we risk exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an alternative to this method? I also think it is necessary to run it on the JavaFX thread, for example in SendAsEmailAction.action notify is called from a background thread.

Copy link
Member

Choose a reason for hiding this comment

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

Usually background tasks are not allowed to communicate things directly. The general principle is that only the hard work is moved to a different task (by using BackgroundTask.wrap(doHardWork)) and the only user interaction should be in the onSuccess and onError handlers that automatically run on the JavaFX thread).

Personally, I would prefer exceptions instead of facing potentially hard to debug threading issues. But for now you can simply leave the runInJavaFXThread call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I understand and will keep that in mind.

}

@Override
Expand Down
25 changes: 5 additions & 20 deletions src/main/java/org/jabref/gui/JabRefFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -135,9 +134,6 @@
import org.jabref.preferences.SearchPreferences;

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;
Expand All @@ -154,12 +150,11 @@ public class JabRefFrame extends BorderPane implements OutputPrinter {
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);

Expand All @@ -185,7 +180,7 @@ public class JabRefFrame extends BorderPane implements OutputPrinter {

public JabRefFrame(Stage mainStage) {
this.mainStage = mainStage;
this.dialogService = new FXDialogService(mainStage);
this.dialogService = new FXDialogService(mainStage, this);
init();
}

Expand Down Expand Up @@ -994,16 +989,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();
Expand Down Expand Up @@ -1284,7 +1269,7 @@ public void showMessage(String message, String title, int msgType) {

@Override
public void setStatus(String s) {
output(s);
dialogService.notify(s);
}

@Override
Expand Down Expand Up @@ -1322,7 +1307,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);
Expand Down Expand Up @@ -1364,7 +1349,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();
});
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/actions/AutoLinkFilesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ public void execute() {
JabRefGUI.getMainFrame().getCurrentBasePanel().getUndoManager().addEdit(nc);
JabRefGUI.getMainFrame().getCurrentBasePanel().markBaseChanged();
}
JabRefGUI.getMainFrame().output(Localization.lang("Finished automatically setting external links."));
JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("Finished automatically setting external links."));
} else {
JabRefGUI.getMainFrame().output(Localization.lang("Finished automatically setting external links.") + " "
JabRefGUI.getMainFrame().getDialogService().notify(Localization.lang("Finished automatically setting external links.") + " "
+ Localization.lang("No files found."));
}
} , diag);
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/actions/CopyBibTeXKeyAndLinkAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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)));
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/actions/CopyDoiUrlAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ public void actionPerformed(ActionEvent e) {
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));
}
}
}
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/actions/LookupIdentifierAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,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);
Expand Down Expand Up @@ -84,7 +84,7 @@ 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",
frame.getDialogService().notify(Localization.lang("Looking up %0... - entry %1 out of %2 - found %3",
fetcher.getIdentifierName(), Integer.toString(count), totalCount, Integer.toString(foundCount)));
Optional<T> identifier = Optional.empty();
try {
Expand All @@ -97,7 +97,7 @@ 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",
frame.getDialogService().notify(Localization.lang("Looking up %0... - entry %1 out of %2 - found %3",
Integer.toString(count), totalCount, Integer.toString(foundCount)));
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/actions/NewDatabaseAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
}
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/desktop/JabRefDesktop.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ 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);
JabRefGUI.getMainFrame().getDialogService().notify(couldNotOpenBrowser);
JOptionPane.showMessageDialog(null,
couldNotOpenBrowser + "\n" + openManually + "\n" +
copiedToClipboard,
Expand Down Expand Up @@ -239,7 +239,7 @@ 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 {
Expand All @@ -251,7 +251,7 @@ public static void openConsole(File file) throws IOException {
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().notify(null);
}
Copy link
Member

Choose a reason for hiding this comment

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

I just saw that we seemed to forgot to convert that old swing dialog here. Could you please fix this as well?
dialogService.showErrorDialog or so should work
Thanks!

Copy link
Contributor Author

@r0light r0light Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

did it in f2a31d0, there was also another one in the same class

}
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/exporter/ExportCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,14 @@ private void export(Path file, FileChooser.ExtensionFilter selectedExtensionFilt
finEntries);
return null; // can not use BackgroundTask.wrap(Runnable) because Runnable.run() can't throw Exceptions
})
.onSuccess(x -> frame.output(Localization.lang("%0 export successful", format.getName())))
.onSuccess(x -> frame.getDialogService().notify(Localization.lang("%0 export successful", format.getName())))
.onFailure(this::handleError)
.executeWith(Globals.TASK_EXECUTOR);
}

private void handleError(Exception ex) {
LOGGER.warn("Problem exporting", ex);
frame.output(Localization.lang("Could not save file."));
frame.getDialogService().notify(Localization.lang("Could not save file."));
// Need to warn the user that saving failed!
frame.getDialogService().showErrorDialogAndWait(Localization.lang("Save library"), Localization.lang("Could not save file."), ex);
}
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ private boolean doSave() {
// Reset title of tab
frame.setTabTitle(panel, panel.getTabTitle(),
panel.getBibDatabaseContext().getDatabaseFile().get().getAbsolutePath());
frame.output(Localization.lang("Saved library") + " '"
frame.getDialogService().notify(Localization.lang("Saved library") + " '"
+ panel.getBibDatabaseContext().getDatabaseFile().get().getPath() + "'.");
frame.setWindowTitle();
frame.updateAllTabTitles();
Expand All @@ -158,7 +158,7 @@ private boolean doSave() {

public boolean save() {
if (panel.getBibDatabaseContext().getDatabasePath().isPresent()) {
panel.frame().output(Localization.lang("Saving library") + "...");
panel.frame().getDialogService().notify(Localization.lang("Saving library") + "...");
panel.setSaving(true);
return doSave();
} else {
Expand Down Expand Up @@ -240,7 +240,7 @@ public void saveSelectedAsPlain() {
try {
saveDatabase(path, true, prefs.getDefaultEncoding(), SavePreferences.DatabaseSaveType.PLAIN_BIBTEX);
frame.getFileHistory().newFile(path);
frame.output(Localization.lang("Saved selected to '%0'.", path.toString()));
frame.getDialogService().notify(Localization.lang("Saved selected to '%0'.", path.toString()));
} catch (SaveException ex) {
LOGGER.error("A problem occurred when trying to save the file", ex);
frame.getDialogService().showErrorDialogAndWait(Localization.lang("Save library"), Localization.lang("Could not save file."), ex);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/externalfiletype/ExternalFileMenuItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@ public void actionPerformed(ActionEvent e) {
boolean success = openLink();
if (!success) {
List<Path> searchedDirs = databaseContext.getFileDirectoriesAsPaths(Globals.prefs.getFilePreferences());
frame.output(Localization.lang("Unable to open %0", link) + " " + Arrays.toString(searchedDirs.toArray()));
frame.getDialogService().notify(Localization.lang("Unable to open %0", link) + " " + Arrays.toString(searchedDirs.toArray()));
}
}

private boolean openLink() {
frame.output(Localization.lang("External viewer called") + ".");
frame.getDialogService().notify(Localization.lang("External viewer called") + ".");
try {
Optional<ExternalFileType> type = fileType;
if (!this.fileType.isPresent()) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/importer/ImportAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,11 @@ private List<ImportFormatReader.UnknownFormatImport> doImport(List<Path> files)
try {
if (!importer.isPresent()) {
// Unknown format:
frame.output(Localization.lang("Importing in unknown format") + "...");
frame.getDialogService().notify(Localization.lang("Importing in unknown format") + "...");
// This import method never throws an IOException:
imports.add(Globals.IMPORT_FORMAT_READER.importUnknownFormat(filename, Globals.getFileUpdateMonitor()));
} else {
frame.output(Localization.lang("Importing in %0 format", importer.get().getName()) + "...");
frame.getDialogService().notify(Localization.lang("Importing in %0 format", importer.get().getName()) + "...");
// Specific importer:
ParserResult pr = importer.get().importDatabase(filename, Globals.prefs.getDefaultEncoding());
imports.add(new ImportFormatReader.UnknownFormatImport(importer.get().getName(), pr));
Expand Down
Loading