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.

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

Fix JavaFX thread exception when fetching new infos #4354

Merged
merged 5 commits into from
Sep 23, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/BasePanel.java
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ public BasePanel(JabRefFrame frame, BasePanelPreferences preferences, BibDatabas

this.getDatabase().registerListener(new UpdateTimestampListener(Globals.prefs));

this.entryEditor = new EntryEditor(this, preferences.getEntryEditorPreferences(), Globals.getFileUpdateMonitor(), dialogService, externalFileTypes);
this.entryEditor = new EntryEditor(this, preferences.getEntryEditorPreferences(), Globals.getFileUpdateMonitor(), dialogService, externalFileTypes, Globals.TASK_EXECUTOR);

this.preview = new PreviewPanel(this, getBibDatabaseContext(), preferences.getKeyBindings(), preferences.getPreviewPreferences(), dialogService, externalFileTypes);
frame().getGlobalSearchBar().getSearchQueryHighlightObservable().addSearchListener(preview);
Expand Down
29 changes: 26 additions & 3 deletions src/main/java/org/jabref/gui/entryeditor/EntryEditor.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,17 @@
import org.jabref.gui.help.HelpAction;
import org.jabref.gui.keyboard.KeyBinding;
import org.jabref.gui.menus.ChangeEntryTypeMenu;
import org.jabref.gui.mergeentries.EntryFetchAndMergeWorker;
import org.jabref.gui.mergeentries.MergeFetchedEntryDialog;
import org.jabref.gui.undo.CountingUndoManager;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.ColorUtil;
import org.jabref.gui.util.DefaultTaskExecutor;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.TypedBibEntry;
import org.jabref.logic.help.HelpFile;
import org.jabref.logic.importer.EntryBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.search.SearchQueryHighlightListener;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
Expand Down Expand Up @@ -91,14 +94,16 @@ public class EntryEditor extends BorderPane {
private final EntryEditorPreferences preferences;
private final DialogService dialogService;
private final NewDroppedFileHandler fileHandler;
private final TaskExecutor taskExecutor;

public EntryEditor(BasePanel panel, EntryEditorPreferences preferences, FileUpdateMonitor fileMonitor, DialogService dialogService, ExternalFileTypes externalFileTypes) {
public EntryEditor(BasePanel panel, EntryEditorPreferences preferences, FileUpdateMonitor fileMonitor, DialogService dialogService, ExternalFileTypes externalFileTypes, TaskExecutor taskExecutor) {
this.panel = panel;
this.databaseContext = panel.getBibDatabaseContext();
this.undoManager = panel.getUndoManager();
this.preferences = Objects.requireNonNull(preferences);
this.fileMonitor = fileMonitor;
this.dialogService = dialogService;
this.taskExecutor = taskExecutor;

fileHandler = new NewDroppedFileHandler(dialogService, databaseContext, externalFileTypes,
Globals.prefs.getFilePreferences(),
Expand Down Expand Up @@ -340,7 +345,7 @@ private void setupToolBar() {
ContextMenu fetcherMenu = new ContextMenu();
for (EntryBasedFetcher fetcher : WebFetchers.getEntryBasedFetchers(preferences.getImportFormatPreferences())) {
MenuItem fetcherMenuItem = new MenuItem(fetcher.getName());
fetcherMenuItem.setOnAction(event -> new EntryFetchAndMergeWorker(panel, getEntry(), fetcher).execute());
fetcherMenuItem.setOnAction(event -> fetchAndMerge(fetcher));
fetcherMenu.getItems().add(fetcherMenuItem);
}
fetcherButton.setOnMouseClicked(event -> fetcherMenu.show(fetcherButton, Side.RIGHT, 0, 0));
Expand All @@ -353,6 +358,24 @@ private void setupToolBar() {
generateCiteKeyButton);
}

private void fetchAndMerge(EntryBasedFetcher fetcher) {
// TODO: Merge with org.jabref.gui.mergeentries.FetchAndMergeEntry
BackgroundTask.wrap(() -> fetcher.performSearch(entry).stream().findFirst())
.onSuccess(fetchedEntry -> {
if (fetchedEntry.isPresent()) {
MergeFetchedEntryDialog dialog = new MergeFetchedEntryDialog(panel, entry, fetchedEntry.get(), fetcher.getName());
dialog.setVisible(true);
} else {
dialogService.notify(Localization.lang("Could not find any bibliographic information."));
}
})
.onFailure(exception -> {
LOGGER.error("Error while fetching entry with " + fetcher.getName(), exception);
Copy link
Member

Choose a reason for hiding this comment

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

use {} for parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not possible if the last argument should be treated as an exception.

dialogService.showErrorDialogAndWait(Localization.lang("Error while fetching from %0", fetcher.getName()), exception);
})
.executeWith(taskExecutor);
}

void addSearchListener(SearchQueryHighlightListener listener) {
// TODO: Highlight search text in entry editors
searchListeners.add(listener);
Expand Down

This file was deleted.

49 changes: 46 additions & 3 deletions src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,34 @@

import java.util.Arrays;
import java.util.List;
import java.util.Optional;

import org.jabref.Globals;
import org.jabref.JabRefGUI;
import org.jabref.gui.BasePanel;
import org.jabref.gui.DialogService;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.logic.importer.IdBasedFetcher;
import org.jabref.logic.importer.WebFetchers;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.FieldName;

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

/**
* Class for fetching and merging information based on a specific field
*
*/
public class FetchAndMergeEntry {

private static final Logger LOGGER = LoggerFactory.getLogger(FetchAndMergeEntry.class);

// A list of all field which are supported
public static List<String> SUPPORTED_FIELDS = Arrays.asList(FieldName.DOI, FieldName.EPRINT, FieldName.ISBN);
private final BasePanel panel;
private DialogService dialogService;

/**
* Convenience constructor for a single field
Expand All @@ -41,11 +54,41 @@ public FetchAndMergeEntry(BibEntry entry, String field) {
* @param fields - List of fields to get information from, one at a time in given order
*/
public FetchAndMergeEntry(BibEntry entry, BasePanel panel, List<String> fields) {
this.dialogService = panel.frame().getDialogService();
this.panel = panel;

// TODO: Don't run this method as part of the constructor
Copy link
Member

Choose a reason for hiding this comment

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

Can you resolve this TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was just too lazy to do it ;-). Fixed now.

fetchAndMerge(entry, fields);
}

public void fetchAndMerge(BibEntry entry, List<String> fields) {
for (String field : fields) {
if (entry.hasField(field)) {
new FetchAndMergeWorker(panel, entry, field).execute();
Optional<String> fieldContent = entry.getField(field);
if (fieldContent.isPresent()) {
BackgroundTask.wrap(() -> {
Optional<IdBasedFetcher> fetcher = WebFetchers.getIdBasedFetcherForField(field, Globals.prefs.getImportFormatPreferences());
if (fetcher.isPresent()) {
return fetcher.get().performSearchById(fieldContent.get());
} else {
return Optional.<BibEntry>empty();
Copy link
Member

Choose a reason for hiding this comment

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

Normally you don't need the genericc type in return Optional.empty

Copy link
Member Author

Choose a reason for hiding this comment

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

I was also surprised but it leads to compiler errors without it.

}
})
.onSuccess(fetchedEntry -> {
String type = FieldName.getDisplayName(field);
if (fetchedEntry.isPresent()) {
MergeFetchedEntryDialog dialog = new MergeFetchedEntryDialog(panel, entry, fetchedEntry.get(), type);
dialog.setVisible(true);
} else {
panel.frame().setStatus(Localization.lang("Cannot get info based on given %0: %1", type, fieldContent.get()));
}
})
.onFailure(exception -> {
LOGGER.error("Error while fetching bibliographic information", exception);
dialogService.showErrorDialogAndWait(exception);
})
.executeWith(Globals.TASK_EXECUTOR);
} else {
panel.frame().setStatus(Localization.lang("No %0 found", FieldName.getDisplayName(field)));
dialogService.notify(Localization.lang("No %0 found", FieldName.getDisplayName(field)));
}
}
}
Expand Down
76 changes: 0 additions & 76 deletions src/main/java/org/jabref/gui/mergeentries/FetchAndMergeWorker.java

This file was deleted.