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

dimmonn · 2019-05-18T18:53:57Z

created background task for the work done in org/jabref/gui/importer/ImportEntriesViewModel.java:68

Siedlerchr · 2019-05-19T20:24:00Z

src/main/java/org/jabref/gui/importer/ImportEntriesDialog.java

-                        container.getChildren().add(1, duplicateButton);
-                    }
+                    Callable<Boolean> hasDuplicateEntryTask = () -> viewModel.hasDuplicate(entry);
+                    BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {


Suggested change

BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {

BackgroundTask.wrap(viewModel::hasDuplicate).onSuccess(e -> {

or if it's not possible you can inline the callable as folows

Suggested change

BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {

BackgroundTask.wrap(()->viewModel.hasDuplicate(entry)).onSuccess(e -> {

Siedlerchr · 2019-05-19T20:25:12Z

src/main/java/org/jabref/gui/importer/ImportEntriesDialog.java

-                    }
+                    Callable<Boolean> hasDuplicateEntryTask = () -> viewModel.hasDuplicate(entry);
+                    BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {
+                        if (e) {


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

You apparently overlooked this one here

Siedlerchr

The approach you took is generally right, but as I answered in the issue, there is the other importEntries function in the viewModel wich also does this check.
Try to find out where it is called (Call Hierachy) and see if that needs also such a Background task

… setting maxWidth and reverting off the temporary fix)

…(adding background task for view model)

…(checkstyle fix)

dimmonn · 2019-05-20T06:09:10Z

@Siedlerchr thank you for the review, your suggestion is committed, as per call flow I haven't found some bottleneck for viewModel as per call flow if gets called mainly org/jabref/gui/importer/ImportEntriesDialog.java:73

but I guess as you wrote it worth to have this task done in a separate thread for both use cases

tobiasdiez · 2019-05-21T12:29:31Z

src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java

+                            optOut -> preferences.setShouldWarnAboutDuplicatesForImport(!optOut));
+
+                    if (!continueImport) {
+                        dialogService.notify(Localization.lang("Import canceled"));


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.

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?

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(); }

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).

thanks, sure

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

…var name)

tobiasdiez

Thanks for the follow-up. The code looks very good now and is almost ready to merge. Please change the variable name e (as remarked by Christoph) and move the notification statement, then we can merge.

tobiasdiez · 2019-06-02T11:42:14Z

src/main/java/org/jabref/gui/importer/ImportEntriesViewModel.java

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


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

sure, should be done

tobiasdiez

Thanks!

Siedlerchr

Thanks for your contribution!

469deb6 Sage Harvard: Add translator/editor 1f51ef8 Add Pensoft dependents (#4981) 5338ff6 Create electrophoresis.csl (#4883) 4de9a97 Update spec_helper.rb fe81f17 Create uirs-urbani-izziv.csl (#4928) 69d37d8 Create uirs-urban-challenge-journal.csl (#4927) d807202 Changes to match Taxon guidelines for authors (#4980) aeb2170 Create ucl-university-college-denmark-apa.csl (#4962) 16f58df Update American Physical Society styles (#4794) b77bd91 Update ucl-university-college-denmark-vancouver.csl (#4963) 81d1a59 Update bibliotheque-d-archeologie-mediterraneenne-et-africaine-biama.csl (#4979) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: 469deb6

fad76fe Update Gemfile.lock a8dafef Update harvard-cite-them-right.csl (JabRef#4993) c8784ff Update and rename emerald-harvard.csl to emerald-harvard-2.csl (JabRef#4991) 197ec0e Update circulation-journal.csl (JabRef#4994) f36dabe Update American Physical Society dependents (JabRef#4992) 93469bb Create journal-of-musculoskeletal-research.csl (JabRef#4990) 43287c7 Update taxon.csl a little more (JabRef#4989) 9f32917 Update and rename university-college-lillebaelt-harvard.csl to ucl-un… (JabRef#4982) 00c3cec Update isnad-metinici.csl (JabRef#4988) 864a64c Update medicine-and-science-in-sports-and-exercise.csl (JabRef#4983) 0c76011 Don't demote particles in Equine Vet J (JabRef#4984) 603ee73 Fix MLA editor translator (JabRef#4985) 40d207c Update pravnik.csl (JabRef#4986) c5fe30c Update isnad-dipnotlu.csl (JabRef#4987) 469deb6 Sage Harvard: Add translator/editor 1f51ef8 Add Pensoft dependents (JabRef#4981) 5338ff6 Create electrophoresis.csl (JabRef#4883) 4de9a97 Update spec_helper.rb fe81f17 Create uirs-urbani-izziv.csl (JabRef#4928) 69d37d8 Create uirs-urban-challenge-journal.csl (JabRef#4927) d807202 Changes to match Taxon guidelines for authors (JabRef#4980) aeb2170 Create ucl-university-college-denmark-apa.csl (JabRef#4962) 16f58df Update American Physical Society styles (JabRef#4794) b77bd91 Update ucl-university-college-denmark-vancouver.csl (JabRef#4963) 81d1a59 Update bibliotheque-d-archeologie-mediterraneenne-et-africaine-biama.csl (JabRef#4979) 0895562 Create pensoft-journals.csl (JabRef#4972) 4f8fa44 creates the gigascience.csl file (JabRef#4961) 1b9c7a0 Create acta-ornithologica.csl (JabRef#4977) c8c6c6d Create sorbonne-student-law-review.csl (JabRef#4956) git-subtree-dir: src/main/resources/csl-styles git-subtree-split: fad76fe

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

f86ce7c

tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 19, 2019

Siedlerchr reviewed May 19, 2019

View reviewed changes

Siedlerchr requested changes May 19, 2019

View reviewed changes

dpolishc added 3 commits May 20, 2019 07:26

fixup! fixup! Too big error message JabRef#4963 (updating controlsfx,…

ebefc85

… setting maxWidth and reverting off the temporary fix)

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

6ec9ee8

…(adding background task for view model)

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

c27f6e6

…(checkstyle fix)

tobiasdiez reviewed May 21, 2019

View reviewed changes

dpolishc added 3 commits May 21, 2019 21:28

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

bb5b2a9

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

f7c0551

Duplicate check on import should be run in background Task (changing …

300eae7

…var name)

tobiasdiez requested changes Jun 2, 2019

View reviewed changes

dpolishc added 2 commits June 2, 2019 16:00

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

006a443

Duplicate check on import should be run in background JabRef#4963

a6fc6f3

tobiasdiez approved these changes Jun 3, 2019

View reviewed changes

Siedlerchr approved these changes Jun 8, 2019

View reviewed changes

Siedlerchr merged commit 08388b6 into JabRef:master Jun 8, 2019

Siedlerchr mentioned this pull request Jun 8, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dimmonn commented May 18, 2019

Siedlerchr May 19, 2019

Siedlerchr May 19, 2019

Siedlerchr May 27, 2019

Siedlerchr left a comment

dimmonn commented May 20, 2019 •

edited

Loading

tobiasdiez May 21, 2019

dimmonn May 21, 2019 •

edited

Loading

tobiasdiez May 21, 2019

tobiasdiez May 26, 2019

dimmonn May 27, 2019

Siedlerchr May 27, 2019

tobiasdiez left a comment

tobiasdiez Jun 2, 2019

dimmonn Jun 2, 2019

tobiasdiez left a comment

Siedlerchr left a comment

	BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {
	BackgroundTask.wrap(viewModel::hasDuplicate).onSuccess(e -> {

	BackgroundTask.wrap(hasDuplicateEntryTask).onSuccess(e -> {
	BackgroundTask.wrap(()->viewModel.hasDuplicate(entry)).onSuccess(e -> {

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

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

Conversation

dimmonn commented May 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

dimmonn commented May 20, 2019 • edited Loading

Choose a reason for hiding this comment

dimmonn May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

dimmonn commented May 20, 2019 •

edited

Loading

dimmonn May 21, 2019 •

edited

Loading