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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We added a feature that allows the user to open all linked files of multiple selected entries by "Open file" option. [#6966](https://github.com/JabRef/jabref/issues/6966)
- We added a keybinding preset for new entries. [#7705](https://github.com/JabRef/jabref/issues/7705)
- We added a select all button for the library import function. [#7786](https://github.com/JabRef/jabref/issues/7786)
- We added auto-key-generation progress to the background task list. [#7267](https://github.com/JabRef/jabref/issues/7267)

### Changed

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/JabRefFrame.java
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ private Node createToolbar() {

new HBox(
pushToApplicationButton,
factory.createIconButton(StandardActions.GENERATE_CITE_KEYS, new GenerateCitationKeyAction(this, dialogService, stateManager)),
factory.createIconButton(StandardActions.GENERATE_CITE_KEYS, new GenerateCitationKeyAction(this, dialogService, stateManager, taskExecutor, prefs)),
factory.createIconButton(StandardActions.CLEANUP_ENTRIES, new CleanupAction(this, prefs, dialogService, stateManager))
),

Expand Down Expand Up @@ -727,7 +727,7 @@ private MenuBar createMenu() {
new SeparatorMenuItem(),

factory.createMenuItem(StandardActions.REPLACE_ALL, new ReplaceStringAction(this, stateManager)),
factory.createMenuItem(StandardActions.GENERATE_CITE_KEYS, new GenerateCitationKeyAction(this, dialogService, stateManager)),
factory.createMenuItem(StandardActions.GENERATE_CITE_KEYS, new GenerateCitationKeyAction(this, dialogService, stateManager, taskExecutor, prefs)),

new SeparatorMenuItem(),

Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
package org.jabref.gui.citationkeypattern;

import java.util.List;
import java.util.function.Consumer;

import org.jabref.gui.DialogService;
import org.jabref.gui.Globals;
import org.jabref.gui.JabRefFrame;
import org.jabref.gui.StateManager;
import org.jabref.gui.actions.ActionHelper;
import org.jabref.gui.actions.SimpleCommand;
import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableKeyChange;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.DefaultTaskExecutor;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.citationkeypattern.CitationKeyGenerator;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.entry.BibEntry;
import org.jabref.preferences.PreferencesService;

public class GenerateCitationKeyAction extends SimpleCommand {

Expand All @@ -24,10 +27,15 @@ public class GenerateCitationKeyAction extends SimpleCommand {
private List<BibEntry> entries;
private boolean isCanceled;

public GenerateCitationKeyAction(JabRefFrame frame, DialogService dialogService, StateManager stateManager) {
private final TaskExecutor taskExecutor;
private final PreferencesService preferencesService;

public GenerateCitationKeyAction(JabRefFrame frame, DialogService dialogService, StateManager stateManager, TaskExecutor taskExecutor, PreferencesService preferencesService) {
this.frame = frame;
this.dialogService = dialogService;
this.stateManager = stateManager;
this.taskExecutor = taskExecutor;
this.preferencesService = preferencesService;

this.executable.bind(ActionHelper.needsEntriesSelected(stateManager));
}
Expand All @@ -45,20 +53,26 @@ public void execute() {

checkOverwriteKeysChosen();

BackgroundTask.wrap(this::generateKeys)
.executeWith(Globals.TASK_EXECUTOR);
if (!this.isCanceled) {
BackgroundTask backgroundTask = this.generateKeysInBackground();
backgroundTask.showToUser(true);
backgroundTask.titleProperty().set(Localization.lang("Autogenerate citation keys"));
backgroundTask.messageProperty().set(Localization.lang("%0/%1 entries", 0, entries.size()));

backgroundTask.executeWith(this.taskExecutor);
}
}

public static boolean confirmOverwriteKeys(DialogService dialogService) {
if (Globals.prefs.getCitationKeyPatternPreferences().shouldWarnBeforeOverwriteCiteKey()) {
public static boolean confirmOverwriteKeys(DialogService dialogService, PreferencesService preferencesService) {
if (preferencesService.getCitationKeyPatternPreferences().shouldWarnBeforeOverwriteCiteKey()) {
return dialogService.showConfirmationDialogWithOptOutAndWait(
Localization.lang("Overwrite keys"),
Localization.lang("One or more keys will be overwritten. Continue?"),
Localization.lang("Overwrite keys"),
Localization.lang("Cancel"),
Localization.lang("Do not ask again"),
optOut -> Globals.prefs.storeCitationKeyPatternPreferences(
Globals.prefs.getCitationKeyPatternPreferences().withWarnBeforeOverwriteCiteKey(!optOut)));
optOut -> preferencesService.storeCitationKeyPatternPreferences(
preferencesService.getCitationKeyPatternPreferences().withWarnBeforeOverwriteCiteKey(!optOut)));
} else {
// Always overwrite keys by default
return true;
Expand All @@ -67,11 +81,11 @@ public static boolean confirmOverwriteKeys(DialogService dialogService) {

private void checkOverwriteKeysChosen() {
// We don't want to generate keys for entries which already have one thus remove the entries
if (Globals.prefs.getCitationKeyPatternPreferences().shouldAvoidOverwriteCiteKey()) {
if (this.preferencesService.getCitationKeyPatternPreferences().shouldAvoidOverwriteCiteKey()) {
entries.removeIf(BibEntry::hasCitationKey);
// if we're going to override some citation keys warn the user about it
} else if (entries.parallelStream().anyMatch(BibEntry::hasCitationKey)) {
boolean overwriteKeys = confirmOverwriteKeys(dialogService);
boolean overwriteKeys = confirmOverwriteKeys(dialogService, this.preferencesService);

// The user doesn't want to override citation keys
if (!overwriteKeys) {
Expand All @@ -80,30 +94,54 @@ private void checkOverwriteKeysChosen() {
}
}

private void generateKeys() {
if (isCanceled) {
return;
}

stateManager.getActiveDatabase().ifPresent(databaseContext -> {
// generate the new citation keys for each entry
final NamedCompound compound = new NamedCompound(Localization.lang("Autogenerate citation keys"));
CitationKeyGenerator keyGenerator =
new CitationKeyGenerator(databaseContext, Globals.prefs.getCitationKeyPatternPreferences());
for (BibEntry entry : entries) {
keyGenerator.generateAndSetKey(entry)
.ifPresent(fieldChange -> compound.addEdit(new UndoableKeyChange(fieldChange)));
private BackgroundTask generateKeysInBackground() {
return new BackgroundTask<Void>() {

private NamedCompound compound;

@Override
protected Void call() {
if (isCanceled) {
return null;
}
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.

updateProgress(0, entries.size());
messageProperty().set(Localization.lang("%0/%1 entries", 0, entries.size()));
});
stateManager.getActiveDatabase().ifPresent(databaseContext -> {
// generate the new citation keys for each entry
compound = new NamedCompound(Localization.lang("Autogenerate citation keys"));
CitationKeyGenerator keyGenerator =
new CitationKeyGenerator(databaseContext, preferencesService.getCitationKeyPatternPreferences());
int entriesDone = 0;
for (BibEntry entry : entries) {
keyGenerator.generateAndSetKey(entry)
.ifPresent(fieldChange -> compound.addEdit(new UndoableKeyChange(fieldChange)));
entriesDone++;
int finalEntriesDone = entriesDone;
DefaultTaskExecutor.runInJavaFXThread(() -> {
updateProgress(finalEntriesDone, entries.size());
messageProperty().set(Localization.lang("%0/%1 entries", finalEntriesDone, entries.size()));
});
}
compound.end();
});
return null;
}
compound.end();

// register the undo event only if new citation keys were generated
if (compound.hasEdits()) {
frame.getUndoManager().addEdit(compound);
@Override
public BackgroundTask<Void> onSuccess(Consumer<Void> onSuccess) {
// register the undo event only if new citation keys were generated
if (compound.hasEdits()) {
frame.getUndoManager().addEdit(compound);
}

frame.getCurrentLibraryTab().markBaseChanged();
dialogService.notify(formatOutputMessage(Localization.lang("Generated citation key for"), entries.size()));
return super.onSuccess(onSuccess);
}
};

frame.getCurrentLibraryTab().markBaseChanged();
dialogService.notify(formatOutputMessage(Localization.lang("Generated citation key for"), entries.size()));
});
}

private String formatOutputMessage(String start, int count) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public GenerateCitationKeySingleAction(BibEntry entry, BibDatabaseContext databa

@Override
public void execute() {
if (!entry.hasCitationKey() || GenerateCitationKeyAction.confirmOverwriteKeys(dialogService)) {
if (!entry.hasCitationKey() || GenerateCitationKeyAction.confirmOverwriteKeys(dialogService, preferencesService)) {
new CitationKeyGenerator(databaseContext, preferencesService.getCitationKeyPatternPreferences())
.generateAndSetKey(entry)
.ifPresent(change -> undoManager.addEdit(new UndoableKeyChange(change)));
Expand Down
2 changes: 2 additions & 0 deletions src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Unable\ to\ monitor\ file\ changes.\ Please\ close\ files\ and\ processes\ and\

%0\ doesn't\ contain\ the\ term\ <b>%1</b>=%0 doesn't contain the term <b>%1</b>

%0/%1\ entries=%0/%1 entries

%0\ export\ successful=%0 export successful

%0\ matches\ the\ regular\ expression\ <b>%1</b>=%0 matches the regular expression <b>%1</b>
Expand Down