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.

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

Added auto-key-generation task to task-progress #7797

Merged
merged 14 commits into from
Jun 10, 2021

Conversation

btut
Copy link
Contributor

@btut btut commented Jun 5, 2021

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.
    Screenshot from 2021-06-05 11-20-37

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
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

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

if (isCanceled) {
return null;
}
showToUser(true);
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 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.

titleProperty().set(Localization.lang("Autogenerate citation keys"));
messageProperty().set(entries.size() + " " + Localization.lang("entries"));

DefaultTaskExecutor.runInJavaFXThread(() -> {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

The relevant code is

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

});

}
`

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@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;
}

Copy link
Member

@tobiasdiez tobiasdiez Jun 6, 2021

Choose a reason for hiding this comment

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

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
Copy link
Member

From my point of view this looks now good

btut added 3 commits June 8, 2021 08:14
Some languages place the number after the word 'counter', this would
result in an ugly message for those languages.
@Siedlerchr
Copy link
Member

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

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 8, 2021
@koppor koppor merged commit 475b298 into JabRef:main Jun 10, 2021
Siedlerchr added a commit to koppor/jabref that referenced this pull request Jun 16, 2021
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Citation key generation on large number of entries not showing up in Background Tasks
5 participants