Duplicate check on import should be run in background Task

[Siedlerchr]

Import a large database.
Import dialog nearly freezes. In debug mode you see that the DuplicateCheck is run on the FXThread and is time consuming.

Should be running in a background task.

[dimmonn]

Hi @Siedlerchr, I would work on this
could you help me to understand it better? what action do I do to import db?
is it File -> Import ?
how many entries shall the db have to be able to reproduce the issue?

[Siedlerchr]

@dimmonn Thanks for your interest in this issue.
First start JabRef with debug parameter http://help.jabref.org/en/CommandLine#debug-mode---debug
Then File->Import into current library

Large database: We have a simple pyhton script to generate a large bib database in the /scripts/ folder.
It generates 10,000 entries or so. I think 100 entries or so should be enough.

When you now import, you see the import window which listst the entries to be imported and you see that it freezes because of the duplicate check .

Codewise a starting point is to look at the methods in ImportEntriesViewModel.java
there are two methods findInternalDuplicates and DuplicateCheck.
At least the internalDuplicate method is running on the FX Thread and therefore blocking the user input.

[dimmonn]

@Siedlerchr thank you, ill try to figure this out

[tobiasdiez]

It might be a good idea to show a progress dialog for the complete import. For this you can use the following method:

jabref/src/main/java/org/jabref/gui/DialogService.java

Line 196 in c754a79

<V> void showProgressDialogAndWait(String title, String content, Task<V> task);

[dimmonn]

@tobiasdiez thank you

[dimmonn]

@tobiasdiez shall I implement DialogService or shall I somehow re-use jabref/src/main/java/org/jabref/gui/JabRefDialogService.java to show the progress?

in terms of having duplicate check in the background task, is it about to move this work to some other thread?

[Siedlerchr]

@dimmonn You should wrap the code in a BackgroundTask, that is the javafx high level concept.
See for example here:

jabref/src/main/java/org/jabref/gui/journals/AbbreviateAction.java

Lines 41 to 43 in a273eb1

	BackgroundTask.wrap(this::abbreviate)
	.onSuccess(panel::output)
	.executeWith(Globals.TASK_EXECUTOR);

And for the ProgressDialog:
See here for an example:

jabref/src/main/java/org/jabref/gui/externalfiles/AutoLinkFilesAction.java

Lines 73 to 76 in 251c44d

	dialogService.showProgressDialogAndWait(
	Localization.lang("Automatically setting file links"),
	Localization.lang("Searching for files"),
	linkFilesTask

[dimmonn]

@Siedlerchr ok thx

[Siedlerchr]

@dimmonn Bascially our BackgroundTask is a wrapper around the javafx Task
https://docs.oracle.com/javase/8/javafx/interoperability-tutorial/concurrency.htm

[dimmonn]

Right got it thx

[dimmonn]

hey gents, just wanted to check if im on the right way
I am trying to do the below
org/jabref/gui/importer/ImportEntriesDialog.java:103

I introduce changes lilke

   Callable<Boolean> hasDuplicateEntryTask = () -> viewModel.hasDuplicate(entry);
                    BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {
                        if (e) {
                            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;

so that this method org.jabref.logic.bibtex.DuplicateCheck#isDuplicate

will be seen in the other thread

[Siedlerchr]

Best is to create a Pull Request, so we can directly see the code changes in the context and see the results of unit tests.
But generally looks like it goes in the right direction
See also that the method hasDuplicate is called from the importEntries method in viewModel and at least one other method. So you need to wrap those as well.

[Siedlerchr]

Fixed by #4981