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

Implement task progress indicator (and dialog) in the toolbar #6443

Merged
merged 42 commits into from
May 12, 2020
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
fdfe074
First draft of a task progress dialog
btut May 7, 2020
6fd1811
Added progress indicator in JabRefFrame
btut May 7, 2020
b883491
Beautified progressindicator and added localization
btut May 8, 2020
255a6e4
Changed to Task<?> in the Tasklist.
btut May 8, 2020
38dd89d
Resolved typing issues for bindings
btut May 8, 2020
1b2aaa6
Converted list of Properties to tasks for listbind
btut May 8, 2020
baf9bd0
New Tasks are first in the list
btut May 8, 2020
40a8feb
Use a PopOver instead of a dialog
btut May 8, 2020
cac989b
Only show download tasks
btut May 8, 2020
8f525b8
Better messages for download tasks
btut May 8, 2020
c877431
Type Witnesses are not needed any more.
btut May 8, 2020
1ad9598
Added extractor to task list
btut May 9, 2020
cd4e38e
Made anyTaskRunningBinding public
btut May 9, 2020
23f8cf0
Removed ObjectProperty wrapping
btut May 9, 2020
4628c3d
NOT WORKING: quit dialogue
btut May 9, 2020
90ff19e
Cleanup
btut May 9, 2020
62d7c14
Fix in dialog service
btut May 9, 2020
008d55e
Add extractor for isRunning
btut May 10, 2020
a9f4493
More informative (and working) quit dialog
btut May 10, 2020
66ac316
Added graphics callback
btut May 10, 2020
e9a7176
Fixed some style issues
btut May 10, 2020
d7442cc
Registered the save task as background task
btut May 10, 2020
5defe3e
Revert "Registered the save task as background task"
btut May 10, 2020
fba9c70
Added note on dialog-order upon close
btut May 10, 2020
6a62e6f
Updated changelog
btut May 10, 2020
a97af13
Fixed style
btut May 10, 2020
3ee4571
Merge branch 'master' of https://github.com/JabRef/jabref into featur…
btut May 10, 2020
44d9ca8
Quickfix for resizing indicator when indeterminate
btut May 11, 2020
bd2eefd
Styled dialog waiting for background tasks
btut May 11, 2020
41efc04
Minor style fix
btut May 11, 2020
2c9ccea
Removed Globals from DefaultTaskExecutor
btut May 11, 2020
ff9ce00
Removed WaitForBackgroundtasksFinishedDialog
btut May 11, 2020
d56138b
Made Bindings in StateManager private
btut May 11, 2020
cf10859
Added tooltip to progress indicator
btut May 11, 2020
fcb1d0c
Not working: own styleclass for toolbar progress indicator
btut May 11, 2020
396411a
Changed callback to method in BackgroundTask
btut May 11, 2020
e24c141
Fixed progress-indicator styling
btut May 12, 2020
478ee05
Improve getIcon method
tobiasdiez May 12, 2020
0557c67
Well, I said hopefully ;-)
tobiasdiez May 12, 2020
23b7e69
Revert "Well, I said hopefully ;-)"
btut May 12, 2020
e3ae796
Revert "Improve getIcon method"
btut May 12, 2020
3db3997
Improved readability in JabRefFrame
btut May 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We now show the number of items found and selected to import in the online search dialog. [#6248](https://github.com/JabRef/jabref/pull/6248)
- We created a new install screen for macOS. [#5759](https://github.com/JabRef/jabref/issues/5759)
- We implemented an option to download fulltext files while importing. [#6381](https://github.com/JabRef/jabref/pull/6381)
- We added a progress-indicator showing the average progress of background tasks to the toolbar. Clicking it reveals a pop-over with a list of running background tasks. [6443](https://github.com/JabRef/jabref/pull/6443)
- We fixed the bug when strike the delete key in the text field. [#6421](https://github.com/JabRef/jabref/issues/6421)

### Changed
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/org/jabref/gui/Base.css
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,21 @@
-fx-padding: -0.1em 0.5em 0.5em 0.5em;
}

.progress-indicator {
btut marked this conversation as resolved.
Show resolved Hide resolved
-fx-progress-color: -jr-theme;
-fx-border-width: 0px;
-fx-background-color: -jr-icon-background;
-fx-padding: 0.5em;
}

.progress-indicator .percentage {
-fx-fill:null;
}

.progress-indicator:hover {
-fx-background-color: -jr-icon-background-active;
}

.check-box {
-fx-label-padding: 0.0em 0.0em 0.0em 0.75em;
-fx-text-fill: -fx-text-background-color;
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/org/jabref/gui/DialogService.java
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,9 @@ Optional<ButtonType> showCustomButtonDialogAndWait(Alert.AlertType type, String
* @param title title of the dialog
* @param content message to show above the progress bar
* @param task The {@link Task} which executes the work and for which to show the dialog
* @return
*/
<V> void showProgressDialogAndWait(String title, String content, Task<V> task);
<V> Optional<Void> showProgressDialogAndWait(String title, String content, Task<V> task);

/**
* Notify the user in an non-blocking way (i.e., in form of toast in a snackbar).
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/jabref/gui/JabRefDialogService.java
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ public <R> Optional<R> showCustomDialogAndWait(Dialog<R> dialog) {
}

@Override
public <V> void showProgressDialogAndWait(String title, String content, Task<V> task) {
public <V> Optional<Void> showProgressDialogAndWait(String title, String content, Task<V> task) {
ProgressDialog progressDialog = new ProgressDialog(task);
progressDialog.setHeaderText(null);
progressDialog.setTitle(title);
Expand All @@ -283,7 +283,7 @@ public <V> void showProgressDialogAndWait(String title, String content, Task<V>
progressDialog.close();
});
themeLoader.installCss(progressDialog.getDialogPane().getScene(), preferences);
progressDialog.show();
return progressDialog.showAndWait();
}

@Override
Expand Down
62 changes: 60 additions & 2 deletions src/main/java/org/jabref/gui/JabRefFrame.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import javafx.beans.value.ChangeListener;
import javafx.beans.value.ObservableValue;
import javafx.geometry.Orientation;
import javafx.scene.Group;
import javafx.scene.Node;
import javafx.scene.control.Alert;
import javafx.scene.control.Button;
Expand All @@ -23,6 +24,7 @@
import javafx.scene.control.Menu;
import javafx.scene.control.MenuBar;
import javafx.scene.control.MenuItem;
import javafx.scene.control.ProgressIndicator;
import javafx.scene.control.Separator;
import javafx.scene.control.SeparatorMenuItem;
import javafx.scene.control.SplitPane;
Expand All @@ -38,6 +40,7 @@
import javafx.scene.layout.Pane;
import javafx.scene.layout.Priority;
import javafx.scene.layout.VBox;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;

import org.jabref.Globals;
Expand Down Expand Up @@ -135,6 +138,8 @@
import org.jabref.preferences.LastFocusedTabPreferences;

import com.google.common.eventbus.Subscribe;
import org.controlsfx.control.PopOver;
import org.controlsfx.control.TaskProgressView;
import org.fxmisc.easybind.EasyBind;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -397,7 +402,19 @@ private void tearDownJabRef(List<String> filenames) {
* @return true if the user chose to quit; false otherwise
*/
public boolean quit() {
// First ask if the user really wants to close, if the library has not been saved since last save.
// First ask if the user really wants to close, if there are still background tasks running
/*
It is important to wait for unfinished background tasks before checking if a save-operation is needed, because
the background tasks may make changes themselves that need saving.
*/
if (stateManager.anyTaskRunningBinding.getValue()) {
WaitForBackgroundtasksFinishedDialog waitForBackgroundtasksFinishedDialog = new WaitForBackgroundtasksFinishedDialog(dialogService);
if (!waitForBackgroundtasksFinishedDialog.showAndWait(stateManager, themeLoader, prefs)) {
return false;
}
}

// Then ask if the user really wants to close, if the library has not been saved since last save.
List<String> filenames = new ArrayList<>();
for (int i = 0; i < tabbedPane.getTabs().size(); i++) {
BasePanel panel = getBasePanelAt(i);
Expand Down Expand Up @@ -517,7 +534,9 @@ private Node createToolbar() {
new Separator(Orientation.VERTICAL),
factory.createIconButton(StandardActions.OPEN_GITHUB, new OpenBrowserAction("https://github.com/JabRef/jabref")),
factory.createIconButton(StandardActions.OPEN_FACEBOOK, new OpenBrowserAction("https://www.facebook.com/JabRef/")),
factory.createIconButton(StandardActions.OPEN_TWITTER, new OpenBrowserAction("https://twitter.com/jabref_org"))
factory.createIconButton(StandardActions.OPEN_TWITTER, new OpenBrowserAction("https://twitter.com/jabref_org")),
new Separator(Orientation.VERTICAL),
createTaskIndicator()
);

HBox.setHgrow(globalSearchBar, Priority.ALWAYS);
Expand Down Expand Up @@ -921,6 +940,45 @@ private MenuBar createMenu() {
return menu;
}

private Group createTaskIndicator() {
ProgressIndicator indicator = new ProgressIndicator();
indicator.getStyleClass().setAll("progress-indicator");
indicator.progressProperty().bind(stateManager.tasksProgressBinding);
btut marked this conversation as resolved.
Show resolved Hide resolved

/*
The label of the indicator cannot be removed with styling. Therefore,
hide it and clip it to a square of (width x width) each time width is updated.
*/
indicator.widthProperty().addListener((observable, oldValue, newValue) -> {
/*
The indeterminate spinner is wider than the determinate spinner.
We must make sure they are the same width for the clipping to result in a square of the same size always.
*/
if (!indicator.isIndeterminate()) {
indicator.setPrefWidth(newValue.doubleValue());
}
if (newValue.doubleValue() > 0) {
Rectangle clip = new Rectangle(newValue.doubleValue(), newValue.doubleValue());
indicator.setClip(clip);
}
});

indicator.setOnMouseClicked(event -> {
TaskProgressView taskProgressView = new TaskProgressView();
EasyBind.listBind(taskProgressView.getTasks(), stateManager.getBackgroundTasks());
taskProgressView.setRetainTasks(true);
taskProgressView.setGraphicFactory(BackgroundTask.iconCallback);

PopOver progressViewPopOver = new PopOver(taskProgressView);
progressViewPopOver.setTitle(Localization.lang("Background Tasks"));
progressViewPopOver.setArrowLocation(PopOver.ArrowLocation.RIGHT_TOP);

progressViewPopOver.show(indicator);
});

return new Group(indicator);
}

public void addParserResult(ParserResult parserResult, boolean focusPanel) {
if (parserResult.toOpenTab()) {
// Add the entries to the open tab.
Expand Down
24 changes: 24 additions & 0 deletions src/main/java/org/jabref/gui/StateManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@
import java.util.Optional;
import java.util.stream.Collectors;

import javafx.beans.Observable;
import javafx.beans.binding.Bindings;
import javafx.beans.binding.BooleanBinding;
import javafx.beans.binding.DoubleBinding;
import javafx.beans.property.IntegerProperty;
import javafx.beans.property.ReadOnlyListProperty;
import javafx.beans.property.ReadOnlyListWrapper;
import javafx.beans.property.SimpleIntegerProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.collections.ObservableMap;
import javafx.concurrent.Task;
import javafx.scene.Node;

import org.jabref.gui.util.CustomLocalDragboard;
Expand Down Expand Up @@ -41,6 +45,17 @@ public class StateManager {
private final OptionalObjectProperty<SearchQuery> activeSearchQuery = OptionalObjectProperty.empty();
private final ObservableMap<BibDatabaseContext, IntegerProperty> searchResultMap = FXCollections.observableHashMap();
private final OptionalObjectProperty<Node> focusOwner = OptionalObjectProperty.empty();
private final ObservableList<Task<?>> backgroundTasks = FXCollections.observableArrayList(taskProperty -> {
return new Observable[] {taskProperty.progressProperty(), taskProperty.runningProperty()};
});

public BooleanBinding anyTaskRunningBinding = Bindings.createBooleanBinding(
btut marked this conversation as resolved.
Show resolved Hide resolved
() -> backgroundTasks.stream().anyMatch(Task::isRunning), backgroundTasks
);

public DoubleBinding tasksProgressBinding = Bindings.createDoubleBinding(
() -> backgroundTasks.stream().filter(Task::isRunning).mapToDouble(Task::getProgress).average().orElse(1), backgroundTasks
);

public StateManager() {
activeGroups.bind(Bindings.valueAt(selectedGroups, activeDatabase.orElse(null)));
Expand Down Expand Up @@ -112,4 +127,13 @@ public void setSearchQuery(SearchQuery searchQuery) {
public OptionalObjectProperty<Node> focusOwnerProperty() { return focusOwner; }

public Optional<Node> getFocusOwner() { return focusOwner.get(); }

public ObservableList<Task<?>> getBackgroundTasks() {
return backgroundTasks;
}

public void addBackgroundTask(Task<?> backgroundTask) {
this.backgroundTasks.add(0, backgroundTask);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package org.jabref.gui;

import java.util.Optional;

import javafx.scene.control.Alert;
import javafx.scene.control.ButtonType;
import javafx.scene.control.DialogPane;
import javafx.scene.control.Label;
import javafx.scene.layout.Region;
import javafx.scene.layout.VBox;

import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.ThemeLoader;
import org.jabref.logic.l10n.Localization;
import org.jabref.preferences.JabRefPreferences;

import org.controlsfx.control.TaskProgressView;
import org.fxmisc.easybind.EasyBind;

/**
* Dialog shown when closing of application needs to wait for some background tasks.
*/
public class WaitForBackgroundtasksFinishedDialog {

private final DialogService dialogService;

public WaitForBackgroundtasksFinishedDialog(DialogService dialogService) {
this.dialogService = dialogService;
}

public boolean showAndWait(StateManager stateManager, ThemeLoader themeLoader, JabRefPreferences preferences) {
TaskProgressView taskProgressView = new TaskProgressView();
EasyBind.listBind(taskProgressView.getTasks(), stateManager.getBackgroundTasks());
taskProgressView.setRetainTasks(false);
taskProgressView.setGraphicFactory(BackgroundTask.iconCallback);

Label message = new Label(Localization.lang("Waiting for background tasks to finish. Quit anyway?"));

VBox box = new VBox(taskProgressView, message);

DialogPane contentPane = new DialogPane();
contentPane.setContent(box);

FXDialog alert = new FXDialog(Alert.AlertType.NONE, Localization.lang("Please wait..."));
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/DialogService.java#L179 (or the other overload) cannot be used here? I think it would be slightly cleaner to make WaitForBackgroundtasksFinishedDialog a "real" dialog (i.e. inherit from Dialog) or even convert it to a proper fxml-based dialog similar to most dialogs.

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 did use the custom dialogue at the beginning, but it gives no access to the actual javafx dialogue, meaning I cannot hide it on an event. The thing with the JabRef dialogues created by the show*AndWait is that they create the actual javafx dialogue in the method, so they can't be accessed later. I just found the createDialog method which would probably be the best to use here. It creates the dialog and takes care of some styling and such, and then returns the actual dialog which can then be shown and waited for. I'll do that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this ok? I tried to follow how showProgressDialogAndWait is implemented.
ff9ce00

alert.setDialogPane(contentPane);
alert.getButtonTypes().setAll(ButtonType.YES, ButtonType.CANCEL);
alert.getDialogPane().setMinHeight(Region.USE_PREF_SIZE);
alert.setResizable(true);
themeLoader.installCss(alert.getDialogPane().getScene(), preferences);

stateManager.anyTaskRunningBinding.addListener((observable, oldValue, newValue) -> {
if (!newValue) {
alert.setResult(ButtonType.YES);
alert.close();
}
});

Dialog<ButtonType> dialog = () -> alert.showAndWait();

Optional<ButtonType> pressedButton = dialogService.showCustomDialogAndWait(dialog);

return pressedButton.isPresent() && pressedButton.get() == ButtonType.YES;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ private void addLinkedFileFromURL(BibDatabaseContext databaseContext, URL url, B
dialogService.notify(Localization.lang("Finished downloading full text document for entry %0.",
entry.getCiteKeyOptional().orElse(Localization.lang("undefined"))));
});
downloadTask.titleProperty().set(Localization.lang("Downloading"));
downloadTask.messageProperty().set(
Localization.lang("Fulltext for") + ": " + entry.getCiteKeyOptional().orElse(Localization.lang("New entry")));
downloadTask.showToUser(true);
Globals.TASK_EXECUTOR.execute(downloadTask);
} catch (MalformedURLException exception) {
dialogService.showErrorDialogAndWait(Localization.lang("Invalid URL"), exception);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,10 @@ public void download() {
entry.addFile(0, newLinkedFile);
});
downloadProgress.bind(downloadTask.workDonePercentageProperty());
downloadTask.titleProperty().set(Localization.lang("Downloading"));
downloadTask.messageProperty().set(
Localization.lang("Fulltext for") + ": " + entry.getCiteKeyOptional().orElse(Localization.lang("New entry")));
downloadTask.showToUser(true);
taskExecutor.execute(downloadTask);
} catch (MalformedURLException exception) {
dialogService.showErrorDialogAndWait(Localization.lang("Invalid URL"), exception);
Expand Down
33 changes: 33 additions & 0 deletions src/main/java/org/jabref/gui/util/BackgroundTask.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.concurrent.Task;
import javafx.scene.Node;
import javafx.util.Callback;

import org.jabref.gui.icon.IconTheme;
import org.jabref.logic.l10n.Localization;

import com.google.common.collect.ImmutableMap;
import org.fxmisc.easybind.EasyBind;

/**
Expand All @@ -27,14 +33,29 @@
* @param <V> type of the return value of the task
*/
public abstract class BackgroundTask<V> {

public static ImmutableMap<String, Node> iconMap = ImmutableMap.of(
Copy link
Member

Choose a reason for hiding this comment

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

Why do you prefer a map here instead of adding a new property icon to the BackgroundTask class (so that downloadTask.setIcon(IconTheme.JabRefIcons.DOWNLOAD) works)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because the TaskProgressView works on a list of tasks. Tasks have no icon property, therefore one must provide a callback that gets an icon from a task.
If BackgroundTask were derived from Task, we could add a property to BackgroundTask. The callback could then check if we have a BackgroundTask at hand and if so, provide the icon stored in a property in BackgroundTask. But BackgroundTask is not derived from Task so this is not possible.
We need a key to map from a Task to an Icon.
I used the title of the task as a key property to map to the icon, so an immutable map seemed like the best way to go.

Copy link
Member

@tobiasdiez tobiasdiez May 11, 2020

Choose a reason for hiding this comment

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

Ok, I see. It's a bit unfortunate but your solution makes sense. I would propose to change the Callback below to a normal method (public static Node getIcon(Task<?> task) which you then use as a callback using BackgroundTask::getIcon) and replace the iconMap by a normal switch statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to method is easy, but I cannot use a switch, because I would need a constant expression. As I use the title of the task, and there is localization on that, it is not constant.
396411a

Localization.lang("Downloading"), IconTheme.JabRefIcons.DOWNLOAD.getGraphicNode()
);

public static Callback<Task<?>, Node> iconCallback = task -> {
if (BackgroundTask.iconMap.containsKey(task.getTitle())) {
return BackgroundTask.iconMap.get(task.getTitle());
} else {
return null;
}
};

private Runnable onRunning;
private Consumer<V> onSuccess;
private Consumer<Exception> onException;
private Runnable onFinished;
private BooleanProperty isCanceled = new SimpleBooleanProperty(false);
private ObjectProperty<BackgroundProgress> progress = new SimpleObjectProperty<>(new BackgroundProgress(0, 0));
private StringProperty message = new SimpleStringProperty("");
private StringProperty title = new SimpleStringProperty(this.getClass().getSimpleName());
private DoubleProperty workDonePercentage = new SimpleDoubleProperty(0);
private BooleanProperty showToUser = new SimpleBooleanProperty(false);

public BackgroundTask() {
workDonePercentage.bind(EasyBind.map(progress, BackgroundTask.BackgroundProgress::getWorkDonePercentage));
Expand Down Expand Up @@ -90,6 +111,10 @@ public StringProperty messageProperty() {
return message;
}

public StringProperty titleProperty() {
return title;
}

public double getWorkDonePercentage() {
return workDonePercentage.get();
}
Expand All @@ -106,6 +131,14 @@ public ObjectProperty<BackgroundProgress> progressProperty() {
return progress;
}

public boolean showToUser() {
return showToUser.get();
}

public void showToUser(boolean show) {
showToUser.set(show);
}

/**
* Sets the {@link Runnable} that is invoked after the task is started.
*/
Expand Down
Loading