Added auto-key-generation task to task-progress by btut · Pull Request #7797 · JabRef/jabref

btut · 2021-06-05T09:45:06Z

Auto-key-generation tasks are now shown in the list of background tasks,
together with a progress representing entries done / entries.
Fixes #7267

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Auto-key-generation tasks are now shown in the list of background tasks, together with a progress representing entries done / entries. Fixes koppor/jabref#7267

To simplify thread synchronization, the background task is now wrapped in an anonymous class, just as in https://github.com/JabRef/jabref/blob/fa8cc829727ffddd97a9056fd466e7098577f4d7/src/main/java/org/jabref/gui/externalfiles/ImportHandler.java

tobiasdiez

A few more remarks concerning the admittedly strange JavaFX conventions around concurrency.

tobiasdiez · 2021-06-06T11:24:15Z

+                    if (isCanceled) {
+                        return null;
+                    }
+                    showToUser(true);


There is this golden rule of concurrency in JavaFX, that you shouldn't update any UI-facing properties in background tasks (or only using updateProgress and updateMessage), see for example https://docs.oracle.com/javafx/2/api/javafx/concurrent/Task.html.

Thus, setting title and message should happen in the constructor and not in the call method.

tobiasdiez · 2021-06-06T11:24:37Z

+                    titleProperty().set(Localization.lang("Autogenerate citation keys"));
+                    messageProperty().set(entries.size() + " " + Localization.lang("entries"));
+
+                    DefaultTaskExecutor.runInJavaFXThread(() -> {


The runInJavaFXThread shouldn't be necessary as this is handled by updateProgress.

I wondered about that, but it is done in ImportHandler as well. Shall I remove it there as well (in an additional PR)?

For some reason it is not working in this context, see https://github.com/JabRef/jabref/pull/7209/files#r551271470. Maybe @Siedlerchr still remembers where the problem was.

The relevant code is

jabref/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

Lines 146 to 155 in fa8cc82

this.updateMessage(task.messageProperty().get());

this.updateTitle(task.titleProperty().get());

BindingsHelper.subscribeFuture(task.progressProperty(), progress -> updateProgress(progress.getWorkDone(), progress.getMax()));

BindingsHelper.subscribeFuture(task.messageProperty(), this::updateMessage);

BindingsHelper.subscribeFuture(task.titleProperty(), this::updateTitle);

BindingsHelper.subscribeFuture(task.isCanceledProperty(), cancelled -> {

if (cancelled) {

cancel();

}

});

, but I don't see any problem with this.

The progress updating is not happening in the fx thread by the BackgroundTask. This will leead to an exception.

So shall we explicitly make updating the progress variable run on the fx thread in updateProgress?
`
protected void updateProgress(BackgroundProgress newProgress) {

DefaultTaskExecutor.runInJavaFXThread(() -> { progress.setValue(newProgress); });

}
`

Ideally yes it would happend in the BackgroundTask itself, but I am unsure if this has any side effects on existing tasks

would nesting DefaultTaskExecutor.runInJavaFXThread() be problematic (in addition to being ugly)?
I don't think there are many tasks that use updateProgress right now, let me investigate.

As far as I (and Intellij) can tell, updateProgress is only used by GenerateCitationKeyAction, FileDownloadTask and ImportHandler.
FileDownloadTask uses EasyBind to update the progress. Does EasyBind already take care of running on the JavaFX thread?

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

Lines 24 to 39 in fa8cc82

@Override

protected Path call() throws Exception {

URLDownload download = new URLDownload(source);

try (ProgressInputStream inputStream = download.asInputStream()) {

EasyBind.subscribe(

inputStream.totalNumBytesReadProperty(),

bytesRead -> updateProgress(bytesRead.longValue(), inputStream.getMaxNumBytes()));

// Make sure directory exists since otherwise copy fails

Files.createDirectories(destination.getParent());

Files.copy(inputStream, destination, StandardCopyOption.REPLACE_EXISTING);

}

return destination;

}

I'm not sure... then using setValue on progress still leads to the same problems (i.e. you cannot bind something to the progress property). Personally, I would leave the whole what happens on which thread hidden as long as possible, i.e. only the DefaultTaskExecutor cares about these things when converting a background task to a JavaFX task.

I guess these problems we are discussing can be easily fixed by not using the progress/message properties of the background task anywhere outside of the task itself. So here for the citation generation there shouldn't be any problem.

UI-facing properties are done on correct task now, and onSuccess is used.

Siedlerchr · 2021-06-07T17:39:19Z

From my point of view this looks now good

This reverts commit db72640.

Some languages place the number after the word 'counter', this would result in an ugly message for those languages.

Siedlerchr · 2021-06-08T06:55:43Z

Don't forge the l10n (see failing unit test) %0\ entries=%0 entries

* upstream/main: Added auto-key-generation task to task-progress (JabRef#7797) cleanup temporary files, use prefix "jabref-" (JabRef#7811) Add easier how-to for checklist (JabRef#7813) Fix annotation + package of ACMPortalParserTest (JabRef#7812) Implemented a select all button for the library import function (issue JabRef#7786) (JabRef#7808) Fix for issue 4682 : Restructuring the side pane structure having additional functionality and merging the remove group menus (JabRef#7714)

Added auto-key-generation task to task-progress

c58fcdd

Auto-key-generation tasks are now shown in the list of background tasks, together with a progress representing entries done / entries. Fixes koppor/jabref#7267

tobiasdiez reviewed Jun 5, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/citationkeypattern/GenerateCitationKeyAction.java Outdated

Siedlerchr requested changes Jun 5, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/citationkeypattern/GenerateCitationKeyAction.java Outdated

btut added 3 commits June 5, 2021 13:41

Run progress-update on JavaFX Thread

20530a2

Checkstyle fix

d3e1334

tobiasdiez requested changes Jun 6, 2021

View reviewed changes

Fixes for JavaFX concurrency

258b26a

UI-facing properties are done on correct task now, and onSuccess is used.

Siedlerchr reviewed Jun 7, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/util/BackgroundTask.java Outdated

btut added 2 commits June 7, 2021 20:04

Reverted making BackgroundProgress public

db72640

Revert "Reverted making BackgroundProgress public"

357219d

This reverts commit db72640.

koppor reviewed Jun 7, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/citationkeypattern/GenerateCitationKeyAction.java Outdated

btut added 3 commits June 8, 2021 08:14

Actually reverted making BackgroundProgress public

9970966

Better message (and correct localization usage)

721a9f1

Removed progress from message

18581e4

Some languages place the number after the word 'counter', this would result in an ugly message for those languages.

btut requested review from Siedlerchr and tobiasdiez June 8, 2021 06:34

Siedlerchr reviewed Jun 8, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/citationkeypattern/GenerateCitationKeyAction.java Outdated

Siedlerchr approved these changes Jun 8, 2021

View reviewed changes

btut added 2 commits June 8, 2021 10:31

Fixed localization

f5e67b1

Pass TaskExecutor instead of using Globals

3b0dfed

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 8, 2021

Siedlerchr approved these changes Jun 8, 2021

View reviewed changes

calixtus reviewed Jun 9, 2021

View reviewed changes

Comment thread src/main/java/org/jabref/gui/citationkeypattern/GenerateCitationKeyAction.java Outdated

btut and others added 2 commits June 9, 2021 09:54

Removed dependency on Globals

ff6e55f

Merge branch 'main' into fix/keygenerationBackgroundTask

10cae32

calixtus approved these changes Jun 10, 2021

View reviewed changes

koppor merged commit 475b298 into JabRef:main Jun 10, 2021

	this.updateMessage(task.messageProperty().get());
	this.updateTitle(task.titleProperty().get());
	BindingsHelper.subscribeFuture(task.progressProperty(), progress -> updateProgress(progress.getWorkDone(), progress.getMax()));
	BindingsHelper.subscribeFuture(task.messageProperty(), this::updateMessage);
	BindingsHelper.subscribeFuture(task.titleProperty(), this::updateTitle);
	BindingsHelper.subscribeFuture(task.isCanceledProperty(), cancelled -> {
	if (cancelled) {
	cancel();
	}
	});

	@Override
	protected Path call() throws Exception {
	URLDownload download = new URLDownload(source);
	try (ProgressInputStream inputStream = download.asInputStream()) {
	EasyBind.subscribe(
	inputStream.totalNumBytesReadProperty(),
	bytesRead -> updateProgress(bytesRead.longValue(), inputStream.getMaxNumBytes()));

	// Make sure directory exists since otherwise copy fails
	Files.createDirectories(destination.getParent());

	Files.copy(inputStream, destination, StandardCopyOption.REPLACE_EXISTING);
	}

	return destination;
	}

Uh oh!

Conversation

btut commented Jun 5, 2021 • edited by calixtus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez Jun 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Siedlerchr commented Jun 7, 2021

Uh oh!

Uh oh!

Uh oh!

Siedlerchr commented Jun 8, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

btut commented Jun 5, 2021 •

edited by calixtus

Loading

tobiasdiez Jun 6, 2021 •

edited

Loading