fix-for-issue-4490: Download fulltext for multiple entries

[nrmancuso]

This fixes the asyncronous fulltext download dialog issue.

[nrmancuso]

This "fixes" issue #4490, namely, the downloads are working, but the dialogs were not because they were being broadcasted on a thread other than the JavaFx thread. This is my first time working with asynchronous tasks, so I'm not sure if there is a better way to show these dialogs or not. I'd welcome any feedback!

[Siedlerchr]

the onSuccess should already be on the FX thread

[nrmancuso]

Only one (usually the last) dialog message appears when downloading multiple full texts. What do you think the cause could be? Also, to clarify my commit message, the reference to the dialogs being broadcasted to the wrong thread was regarding the findFullTexts() method. Thanks for the help!

[tobiasdiez]

What's the status of this PR?

Usually, to notify about the current status, the task should derive from BackgroundTask and then call updateMessage and updateProgress accordingly. See for example here: https://github.com/JabRef/jabref/blob/8520833e684aa9e305dd8745e45696ffd6d984fb/src/main/java/org/jabref/gui/importer/UnlinkedFilesCrawler.java

[nrmancuso]

I have done no further work on this PR. I am currently swamped, but can get back to work on this in a week or so after this semester is over if that's ok. Thanks for the suggestion as how to proceed.

[nrmancuso]

@tobiasdiez Hi, I have been studying the "UnlinkedFileCrawler", as well as the "FindUnlinkedFilesDialog" in hopes of finding some inspiration to fix this issue. I've attempted to figure out the best way to employ the updateMessage and updateProgress methods, but I can't think of a straightforward way to implement this. "UnlinkedFileCrawler" inherits these methods since it extends BackgroundTask, but since "FindFullTextAction" does not, I don't know what the best way to get this functionality would be. Should I create a "FindFullTextTask" that inherits from "BackgroundTask", then instantiate that in "FindFullTextAction"?

[Siedlerchr]

@nmancus1 Hi, if you look into the FindFullTextAction code, there is a new Task<> created. There you can add the udpateMessage and updateProgress methods:
The TASK is the javafx Task class, which is simliar or nearly identical to the BackgroundTask class.

You should maybe start by merging in the latest master in your branch.

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

Lines 72 to 93 in 3ed02ee

	Task<Map<Optional<URL>, BibEntry>> findFullTextsTask = new Task<Map<Optional<URL>, BibEntry>>() {
	@Override
	protected Map<Optional<URL>, BibEntry> call() {
	Map<Optional<URL>, BibEntry> downloads = new ConcurrentHashMap<>();
	int count = 0;
	for (BibEntry entry : basePanel.getSelectedEntries()) {
	FulltextFetchers fetchers = new FulltextFetchers(Globals.prefs.getImportFormatPreferences());
	downloads.put(fetchers.findFullTextPDF(entry), entry);
	updateProgress(++count, basePanel.getSelectedEntries().size());
	}
	return downloads;
	}
	};
	findFullTextsTask.setOnSucceeded(value -> downloadFullTexts(findFullTextsTask.getValue()));
	dialogService.showProgressDialogAndWait(
	Localization.lang("Look up full text documents"),
	Localization.lang("Looking for full text document..."),
	findFullTextsTask);
	Globals.TASK_EXECUTOR.execute(findFullTextsTask);

[nrmancuso]

Hello, after merging the latest master into my branch, it seems like the only issue that remains is the map collision, which prevents multiple key/value pairs missing an Optional<URL> key to exist simultaneously. This is what is preventing dialogService.notify from alerting the user if a text doesn't exist. Should I just reverse the roles of the key and value pair so that all the keys (BibEntry's) are unique?

[Siedlerchr]

Ah yeah I think this could

[Siedlerchr]

Ah yeah I think this could be a good idea to change the key with the value.
Even theoretical equal bib entry still have another internal id as far as I know. So that should be no problem

[nrmancuso]

@Siedlerchr ready for review. Why did all these commits appear above when I pushed my commit to this branch? Sorry, I am still getting the hang of source control.

[Siedlerchr]

Thanks for the follow-up!

If this is ready for review then please remove the wip from the title.

The commits appeared because they are the newer commits from the master branch which in the meantime have been added to the master since you created your branch.
With the merge all commits have been added to your branch to make it equal (plus your commits) with the master.

To prevent this you would use git rebase to put your commits on top of the current master branch

https://git-scm.com/book/en/v1/Git-Branching-Rebasing

[nrmancuso]

wip tag removed! Thank you very much for the article, it was very helpful.

[tobiasdiez]

can you please try again with git merge because as of now all these other commits are represented as changes, which makes this PR unreviewable at the moment. @koppor any suggestions for git magic?

[Siedlerchr]

Maybe a git rebase?

[nrmancuso]

I think I've got almost everything sorted, except even after a rebase, it shows the older version of the original file instead of the latest version from master.

[tobiasdiez]

I guess the latest merge commit overwrites some of your previous changes. Maybe the easiest way is to quickly reimplement them using copy pasting of the important parts.

[nrmancuso]

I am submitting a new PR since i had to delete my fork to resolve this commit history issue, resulting in the "unknown repository" tag above. Sorry about all of this, I have learned a lot about source control today though.