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

Fix JavaFX thread exception when fetching new infos #4354

Merged
merged 5 commits into from
Sep 23, 2018
Merged

Fix JavaFX thread exception when fetching new infos #4354

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c735987
Fix JavaFX thread exception when fetching new infos
tobiasdiez Sep 18, 2018
62b9984
Fix checkstyle
tobiasdiez Sep 19, 2018
75b6b7d
Improve code
tobiasdiez Sep 23, 2018
b15bfbc
Merge remote-tracking branch 'origin/fixMerge' into fixMerge
tobiasdiez Sep 23, 2018
937623e
fix checkstyle
tobiasdiez Sep 23, 2018
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
View file
Open in desktop
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
View file
Open in desktop
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
64 changes: 0 additions & 64 deletions src/main/java/org/jabref/gui/mergeentries/EntryFetchAndMergeWorker.java
View file
Open in desktop

This file was deleted.

50 changes: 47 additions & 3 deletions src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,22 @@

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
*
Expand All @@ -17,7 +26,12 @@ public class FetchAndMergeEntry {

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

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

private final BasePanel panel;
private DialogService dialogService;

/**
* Convenience constructor for a single field
*
Expand All @@ -41,11 +55,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
View file
Open in desktop

This file was deleted.