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 10 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
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
50 changes: 49 additions & 1 deletion src/main/java/org/jabref/gui/JabRefFrame.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,15 @@
import java.util.TimerTask;

import javafx.application.Platform;
import javafx.beans.property.ObjectProperty;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.ObservableValue;
import javafx.collections.ListChangeListener;
import javafx.collections.ObservableList;
import javafx.concurrent.Task;
import javafx.event.EventHandler;
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 +29,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 @@ -32,14 +39,19 @@
import javafx.scene.control.Tooltip;
import javafx.scene.control.skin.TabPaneSkin;
import javafx.scene.input.KeyEvent;
import javafx.scene.input.MouseEvent;
import javafx.scene.input.TransferMode;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
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 javafx.stage.Window;
import org.controlsfx.control.PopOver;
import org.controlsfx.control.TaskProgressView;
import org.jabref.Globals;
import org.jabref.JabRefExecutorService;
import org.jabref.gui.actions.ActionFactory;
Expand Down Expand Up @@ -517,7 +529,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 +935,40 @@ 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) -> {
if(newValue.doubleValue()>0){
Rectangle clip=new Rectangle(newValue.doubleValue(),newValue.doubleValue());
indicator.setClip(clip);
}
});


indicator.setOnMouseClicked(event -> {

TaskProgressView taskProgressView = new TaskProgressView();
ObservableList<Task<?>> tasks = EasyBind.map(stateManager.getBackgroundTasks(), ObjectProperty<Task<?>>::<Task<?>>get);
EasyBind.listBind(taskProgressView.getTasks(), tasks);
taskProgressView.setRetainTasks(true);

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
28 changes: 28 additions & 0 deletions src/main/java/org/jabref/gui/StateManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,25 @@
import java.util.Optional;
import java.util.stream.Collectors;

import javafx.beans.binding.Binding;
import javafx.beans.binding.Bindings;
import javafx.beans.binding.BooleanBinding;
import javafx.beans.binding.DoubleBinding;
import javafx.beans.property.IntegerProperty;
import javafx.beans.property.ObjectProperty;
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.fxmisc.easybind.EasyBind;
import org.jabref.gui.util.CustomLocalDragboard;
import org.jabref.gui.util.OptionalObjectProperty;
import org.jabref.gui.util.uithreadaware.UiThreadObservableList;
import org.jabref.logic.search.SearchQuery;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
Expand All @@ -41,6 +48,7 @@ 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 UiThreadObservableList<ObjectProperty<Task<?>>> backgroundTasks = new UiThreadObservableList(FXCollections.observableArrayList());
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to wrap the tasks around in a ObjectProperty? It should also work without this wrapper. If there were problems with the updates, you might need to add an "extractor" to the observableArrayList which specifies that the list should update if the underlying data changes (in this case probably the progress of the task).

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'm confused. I though EasyBind's combineList takes care of updating the observable when the progress updates? From the doc:

Turns an observable list of observable values into a single observable value. The resulting observable value is updated when elements are added or removed to or from the list, as well as when element values change.

I had to turn it into a list of ObjectProperty because that's what EasyBind's combineList expects, but I guess it then only registers changes of the ObjectProperty, not the task.

So to update upon progress changes, I need to tell the list that I am interested in the progress by defining an extractor. I found some code online working with extractors, so I think I should be able to implement that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, combineList takes updates into account but for this the elements have to observable themselves. For example, if ObservableList<ObservableDoubleValue> is a list containing the progress property of each task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hints. This works!
Screenshot from 2020-05-09 19-37-23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the ObjectProperty wrapping issue. I tried changing it back just to see what happens, and I still get the same error for the bindings:

no instance(s) of type variable(s) T exist so that Task<?> conforms to ObservableValue<? extends T>

I think that the combine method only works on lists of observables.
If I create the binding with Bindings.createDoubleBinding, it does not update.

Copy link
Member

Choose a reason for hiding this comment

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

So

public Binding<Double> tasksProgressBinding = Bindings.createDoubleBinding(
            backgroundTasks.filter(Task::isRunning).mapToDouble(Task::getProgress).average().orElse(1), backgroundTasks);`

doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it does (except for some minor mistakes in your code).
I did not put the list name (backgroundTasks) as a second argument. Does that mean that the binding would have worked ONLY on the extractor? So adding and removing does not update the value, but a change in the progress would have? Or why else do we need to pass the list as a second argument?

Copy link
Member

Choose a reason for hiding this comment

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

The second argument specifies the observables that the function (the first argument) depends on. That is, every time these observables change, the function is called and the binding is updated with the new value. If you don't specify any observables in the second argument, then the binding is never updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that makes sense! Thanks for explaining!


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

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

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

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

Binding<Boolean> anyTaskRunningBinding = EasyBind.<Task<?>, Boolean>combine(
backgroundTasks,
tStream -> tStream.anyMatch(Task::isRunning)
);

public Binding<Double> tasksProgressBinding = EasyBind.<Task<?>, Double>combine(
backgroundTasks,
stream -> stream.<Task<?>>filter(Task::isRunning).mapToDouble(Task::getProgress)
.average().orElse(1)
);

}
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
14 changes: 14 additions & 0 deletions src/main/java/org/jabref/gui/util/BackgroundTask.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ public abstract class BackgroundTask<V> {
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 +92,10 @@ public StringProperty messageProperty() {
return message;
}

public StringProperty titleProperty() {
return title;
}

public double getWorkDonePercentage() {
return workDonePercentage.get();
}
Expand All @@ -106,6 +112,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
12 changes: 11 additions & 1 deletion src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@
import java.util.function.Consumer;

import javafx.application.Platform;
import javafx.beans.property.SimpleObjectProperty;
import javafx.concurrent.Task;

import org.jabref.Globals;
import org.jabref.gui.StateManager;
import org.jabref.logic.util.DelayTaskThrottler;

import org.slf4j.Logger;
Expand Down Expand Up @@ -96,7 +99,11 @@ public static void runInJavaFXThread(Runnable runnable) {

@Override
public <V> Future<V> execute(BackgroundTask<V> task) {
return execute(getJavaFXTask(task));
Task<V> javafxTask = getJavaFXTask(task);
if (task.showToUser()) {
Globals.stateManager.addBackgroundTask(new SimpleObjectProperty<Task<?>>(javafxTask));
}
return execute(javafxTask);
}

@Override
Expand Down Expand Up @@ -128,8 +135,11 @@ private <V> Task<V> getJavaFXTask(BackgroundTask<V> task) {
Task<V> javaTask = new Task<V>() {

{
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();
Expand Down
6 changes: 6 additions & 0 deletions src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ Available\ import\ formats=Available import formats

%0\ source=%0 source

Background\ Tasks=Background Tasks

Browse=Browse

by=by
Expand Down Expand Up @@ -209,6 +211,8 @@ Default\ encoding=Default encoding

Default\ grouping\ field=Default grouping field

Downloading=Downloading

Execute\ default\ action\ in\ dialog=Execute default action in dialog

Delete=Delete
Expand Down Expand Up @@ -368,6 +372,8 @@ Formatter\ name=Formatter name

found\ in\ AUX\ file=found in AUX file

Fulltext\ for=Fulltext for

Further\ information\ about\ Mr.\ DLib\ for\ JabRef\ users.=Further information about Mr. DLib for JabRef users.

General=General
Expand Down