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

Duplicate check on import should be run in background Task #4963 #4981

Merged
merged 9 commits into from
Jun 8, 2019
Merged

Duplicate check on import should be run in background Task #4963 #4981

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f86ce7c
Duplicate check on import should be run in background Task #4963
May 18, 2019
ebefc85
fixup! fixup! Too big error message #4963 (updating controlsfx, setti…
May 20, 2019
6ec9ee8
Duplicate check on import should be run in background Task #4963 (ad…
May 20, 2019
c27f6e6
Duplicate check on import should be run in background Task #4963 (ch…
May 20, 2019
bb5b2a9
Duplicate check on import should be run in background Task #4963
May 21, 2019
f7c0551
Duplicate check on import should be run in background Task #4963
May 26, 2019
300eae7
Duplicate check on import should be run in background Task (changing …
May 27, 2019
006a443
Duplicate check on import should be run in background Task #4963
Jun 2, 2019
a6fc6f3
Duplicate check on import should be run in background #4963
Jun 2, 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
15 changes: 9 additions & 6 deletions src/main/java/org/jabref/gui/importer/ImportEntriesDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import javafx.scene.layout.VBox;
import javafx.scene.text.Text;

import org.jabref.Globals;
import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.icon.IconTheme;
Expand Down Expand Up @@ -108,12 +109,14 @@ private void initialize() {
container.getStyleClass().add("entry-container");
BindingsHelper.includePseudoClassWhen(container, entrySelected, addToggle.selectedProperty());

if (viewModel.hasDuplicate(entry)) {
Button duplicateButton = IconTheme.JabRefIcons.DUPLICATE.asButton();
duplicateButton.setTooltip(new Tooltip(Localization.lang("Possible duplicate of existing entry. Click to resolve.")));
duplicateButton.setOnAction(event -> viewModel.resolveDuplicate(entry));
container.getChildren().add(1, duplicateButton);
}
BackgroundTask.wrap(() -> viewModel.hasDuplicate(entry)).onSuccess(e -> {
if (e) {
Copy link
Member

Choose a reason for hiding this comment

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

Please no single abbrevations, try to specify the result in the variable name, e.g. duplicateFound

Copy link
Member

Choose a reason for hiding this comment

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

You apparently overlooked this one here

Button duplicateButton = IconTheme.JabRefIcons.DUPLICATE.asButton();
duplicateButton.setTooltip(new Tooltip(Localization.lang("Possible duplicate of existing entry. Click to resolve.")));
duplicateButton.setOnAction(event -> viewModel.resolveDuplicate(entry));
container.getChildren().add(1, duplicateButton);
}
}).executeWith(Globals.TASK_EXECUTOR);

return container;
})
Expand Down
44 changes: 26 additions & 18 deletions src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;

import org.jabref.Globals;
import org.jabref.gui.AbstractViewModel;
import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
Expand Down Expand Up @@ -50,7 +51,7 @@ public ImportEntriesViewModel(BackgroundTask<List<BibEntry>> task, TaskExecutor
this.message.bind(task.messageProperty());

task.onSuccess(entriesToImport -> entries.addAll(entriesToImport))
.executeWith(taskExecutor);
.executeWith(taskExecutor);
}

public String getMessage() {
Expand All @@ -75,24 +76,33 @@ public void importEntries(List<BibEntry> entriesToImport) {
// Check if we are supposed to warn about duplicates.
// If so, then see if there are duplicates, and warn if yes.
if (preferences.shouldWarnAboutDuplicatesForImport()) {
boolean containsDuplicate = entriesToImport.stream()
.anyMatch(this::hasDuplicate);

if (containsDuplicate) {
boolean continueImport = dialogService.showConfirmationDialogWithOptOutAndWait(Localization.lang("Duplicates found"),
Localization.lang("There are possible duplicates (marked with an icon) that haven't been resolved. Continue?"),
Localization.lang("Continue with import"),
Localization.lang("Cancel import"),
Localization.lang("Disable this confirmation dialog"),
optOut -> preferences.setShouldWarnAboutDuplicatesForImport(!optOut));

if (!continueImport) {
dialogService.notify(Localization.lang("Import canceled"));
return;
BackgroundTask.wrap(() -> entriesToImport.stream()
.anyMatch(this::hasDuplicate)).onSuccess(duplicateFound -> {
if (duplicateFound) {
boolean continueImport = dialogService.showConfirmationDialogWithOptOutAndWait(Localization.lang("Duplicates found"),
Localization.lang("There are possible duplicates (marked with an icon) that haven't been resolved. Continue?"),
Localization.lang("Continue with import"),
Localization.lang("Cancel import"),
Localization.lang("Disable this confirmation dialog"),
optOut -> preferences.setShouldWarnAboutDuplicatesForImport(!optOut));

if (!continueImport) {
dialogService.notify(Localization.lang("Import canceled"));
Copy link
Member

Choose a reason for hiding this comment

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

There is a small problem with the current solution: because the background task is called asynchronously, the importEntries is always called even if there are duplicates. That is the code now runs in the following order: import entries > duplicate check > ask user. But the desired order is duplicate check > ask user > import entries.

Copy link
Contributor Author

@dimmonn dimmonn May 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The importEntries function depends on duplicateCheck from the beginning it seems.

Does it mean the importEntries requires some refactoring so that duplicate check will not be called inside that method, or I took it wrong?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be enough to move the rest of the method up here:

if (!continueImport) {
    dialog.notify(aborted);
} else {
    init file handler
    import();
}

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the update. It looks almost good now: the import should also be performed if (preferences.shouldWarnAboutDuplicatesForImport()) returns false and if there is no duplicates (i.e e = false).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, sure

Copy link
Member

Choose a reason for hiding this comment

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

Same as abvoe, please no abbreviations like "e" for variables (except for exceptions maybe)

} else {
buildImportHandlerThenImportEntries(entriesToImport);
dialogService.notify(Localization.lang("Number of entries successfully imported") + ": " + entriesToImport.size());
Copy link
Member

Choose a reason for hiding this comment

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

Please move the notify to the end of buildImportHandlerThenImportEntries so that the message is shown in every case after the import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, should be done

}
} else {
buildImportHandlerThenImportEntries(entriesToImport);
}
}
}).executeWith(Globals.TASK_EXECUTOR);
} else {
buildImportHandlerThenImportEntries(entriesToImport);
}

}

private void buildImportHandlerThenImportEntries(List<BibEntry> entriesToImport) {
ImportHandler importHandler = new ImportHandler(
dialogService,
database,
Expand All @@ -104,8 +114,6 @@ public void importEntries(List<BibEntry> entriesToImport) {
undoManager,
stateManager);
importHandler.importEntries(entriesToImport);

dialogService.notify(Localization.lang("Number of entries successfully imported") + ": " + entriesToImport.size());
}

/**
Expand Down