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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 42 commits into from
May 12, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 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
29 changes: 28 additions & 1 deletion src/main/java/org/jabref/gui/JabRefFrame.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
import javafx.application.Platform;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.ObservableValue;
import javafx.collections.ListChangeListener;
import javafx.concurrent.Task;
import javafx.event.EventHandler;
import javafx.geometry.Orientation;
import javafx.scene.Node;
import javafx.scene.control.Alert;
Expand All @@ -23,6 +26,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,6 +36,7 @@
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;
Expand All @@ -40,6 +45,7 @@
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

import javafx.stage.Window;
import org.jabref.Globals;
import org.jabref.JabRefExecutorService;
import org.jabref.gui.actions.ActionFactory;
Expand Down Expand Up @@ -107,6 +113,7 @@
import org.jabref.gui.shared.ConnectToSharedDatabaseCommand;
import org.jabref.gui.shared.PullChangesFromSharedAction;
import org.jabref.gui.specialfields.SpecialFieldMenuItemFactory;
import org.jabref.gui.taskprogressmanager.TaskProgressDialog;
import org.jabref.gui.texparser.ParseLatexAction;
import org.jabref.gui.undo.CountingUndoManager;
import org.jabref.gui.undo.UndoRedoAction;
Expand Down Expand Up @@ -517,7 +524,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 +930,24 @@ private MenuBar createMenu() {
return menu;
}

private ProgressIndicator createTaskIndicator() {
ProgressIndicator indicator = new ProgressIndicator(1);
indicator.progressProperty().bind(stateManager.tasksProgressBinding);
btut marked this conversation as resolved.
Show resolved Hide resolved

indicator.setOnMouseClicked(new EventHandler<MouseEvent>() {
@Override
public void handle(MouseEvent event) {
TaskProgressDialog taskProgressDialog = new TaskProgressDialog();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using a real dialog, what about using a collapse overlay similar to how it's done in firefox?
https://github.com/controlsfx/controlsfx/wiki/ControlsFX-Features#popover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh fancy! I'll look into it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2020-05-08 15-12-50
That looks much better! Great idea!

// @TODO add localization
taskProgressDialog.setTitle("Task progress");
Window dialogWindow = taskProgressDialog.getDialogPane().getScene().getWindow();
dialogWindow.setOnCloseRequest(windowEvent -> dialogWindow.hide());
taskProgressDialog.showAndWait();
}
});
return indicator;
}

public void addParserResult(ParserResult parserResult, boolean focusPanel) {
if (parserResult.toOpenTab()) {
// Add the entries to the open tab.
Expand Down
22 changes: 22 additions & 0 deletions src/main/java/org/jabref/gui/StateManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,23 @@
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.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;
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 +46,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<Task> backgroundTasks = new UiThreadObservableList(FXCollections.observableArrayList());

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

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

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

public void addBackgroundTask(Task backgroundTask) {
this.backgroundTasks.add(backgroundTask);
}

public BooleanBinding anyTaskRunningBinding = Bindings.createBooleanBinding(
Copy link
Member

Choose a reason for hiding this comment

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

For the bindings to update, you need to add the list as a dependency (second argument of the createXBinding method). In the case of lists, however, its easier to use EasyBind.combine: https://github.com/TomasMikula/EasyBind#combine-list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, that looks useful.
However, I think I am running into some typing issues which I am struggling to resolve.

With the library you pointed me to, I wound up with the following:

public Binding<Boolean> anyTaskRunningBinding = EasyBind.combine( backgroundTasks, stream -> stream.anyMatch(Task::getProgress) );

This gives me an error that it expects a Binding, but gets a MonadicBinding.
If I just cast it, I get the following:

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

I think this is the issue I need to resolve first. As the example in the library works fine, I guess the conversion from MonadicBinding to Binding is then done implicitly, is that correct?

I struggle solving this because I dont know the type parameter of the tasks I am storing.
When storing, the Task has type V:

https://github.com/btut/jabref/blob/b8834914e5e735abaa7a710216888abe6bf54277/src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java#L99-L104

But in the list I just use Task:

https://github.com/btut/jabref/blob/b8834914e5e735abaa7a710216888abe6bf54277/src/main/java/org/jabref/gui/StateManager.java#L49

How do I need to change the list in order for it to work?

Copy link
Member

Choose a reason for hiding this comment

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

Add the generics to the Task as welll:
private final UiThreadObservableList<Task<V>> backgroundTasks = new UiThreadObservableList(FXCollections.observableArrayList());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But where would I get the V from in this case?
Cannot resolve symbol 'V'

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 changed it to Task<?> now and now the progress, title and message properties make their way through to the dialogue.
I still cannot create the bindings for the progress indicator though.

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 change also allows me to use EasyBind's listBind to bind the task list in the view to the task list in StateManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Here is the first peak at a download in the dialogue (which will probably be changed to a popover later). As you can see, the download task has it's title and message set and progress is updated fine. However, there are still a lot of tasks that do not have any details.

Copy link
Member

@Siedlerchr Siedlerchr May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Looks good already!
There are a lot of tasks which don't concern file downloads and just perform some operations in the background. If you search for all references from backgrond task you probably have to adjust each one to add a meaningful description

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 think that should be doable. But before I do that: is it a good idea to have all background tasks listed? Or should we just show downloads? If we go with the first option, I suggest turning off the retain-tasks feature of the task view. Then, all completed tasks will automatically disappear. This way we have less tasks that only ran for a very little time filling up the view.

I got the bindings to work by storing a list of Property<Task> Instead of Task. That does the trick for the progress indicator. It now is indeterminate when one of the tasks has an indeterminate progress and shows the average progress otherwise (100% if no tasks are running).
For some reason, this breaks the task view. Since I now store a list of properties, and not a list of tasks, I cannot bind them directly to the view, so I went back to doing it manually, but that does not seem to work.

https://github.com/btut/jabref/blob/38dd89dce28b72d195de18b848b011532ef1f868/src/main/java/org/jabref/gui/taskprogressmanager/TaskProgressDialog.java#L34-L47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I just converted the list of properties back into a list of tasks using EasyBind and the bind that list to the list of tasks in the view (again using EasyBind). Now both the indicator and the dialogue work fine and are updated with the running tasks!

() -> backgroundTasks.stream().anyMatch(Task::isRunning)
);

public DoubleBinding tasksProgressBinding = Bindings.createDoubleBinding(
() -> backgroundTasks.stream().filter(Task::isRunning).mapToDouble(Task::getProgress).average().orElse(1)
);
}
2 changes: 2 additions & 0 deletions src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,8 @@ public void download() {
entry.addFile(0, newLinkedFile);
});
downloadProgress.bind(downloadTask.workDonePercentageProperty());
downloadTask.titleProperty().set("Downloading");
downloadTask.messageProperty().set(linkedFile.getLink());
taskExecutor.execute(downloadTask);
} catch (MalformedURLException exception) {
dialogService.showErrorDialogAndWait(Localization.lang("Invalid URL"), exception);
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/org/jabref/gui/taskprogressmanager/TaskProgressDialog.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
.addEntryButton {
-fx-font-size: 2em;
}

.entry-container {
/*-fx-padding: 0.5em 0em 0.5em 0em;*/
}

.list-cell {
-fx-padding: 0.5em 0 1em 0.5em;
}

.list-cell:entry-selected {
-fx-background-color: derive(-jr-selected, 60%);
}
13 changes: 13 additions & 0 deletions src/main/java/org/jabref/gui/taskprogressmanager/TaskProgressDialog.fxml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>
<?import org.controlsfx.control.TaskProgressView?>
<DialogPane prefHeight="700.0" prefWidth="1000.0" xmlns="http://javafx.com/javafx/10.0.2-internal"
xmlns:fx="http://javafx.com/fxml/1" fx:controller="org.jabref.gui.taskprogressmanager.TaskProgressDialog">
<content>
<VBox spacing="10.0">
<TaskProgressView fx:id="taskProgressView" VBox.vgrow="ALWAYS"/>
</VBox>
</content>
</DialogPane>
47 changes: 47 additions & 0 deletions src/main/java/org/jabref/gui/taskprogressmanager/TaskProgressDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package org.jabref.gui.taskprogressmanager;

import com.airhacks.afterburner.views.ViewLoader;
import javafx.collections.ListChangeListener;
import javafx.concurrent.Task;
import javafx.fxml.FXML;
import org.controlsfx.control.TaskProgressView;
import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.util.*;

import javax.inject.Inject;

public class TaskProgressDialog extends BaseDialog<Boolean> {

public TaskProgressView<Task<Object>> taskProgressView;
private TaskViewModel viewModel;
@Inject private DialogService dialogService;
@Inject private StateManager stateManager;

public TaskProgressDialog() {
ViewLoader.view(this)
.load()
.setAsDialogPane(this);
}

@FXML
private void initialize() {
viewModel = new TaskViewModel(dialogService, stateManager);
taskProgressView.setRetainTasks(true);

viewModel.getBackgroundTasks().addListener(new ListChangeListener<Task>() {
@Override
public void onChanged(Change<? extends Task> c) {
while (c.next()) {

for (Task t : c.getAddedSubList()) {
taskProgressView.getTasks().add(t);
}
for (Task t : c.getRemoved()) {
taskProgressView.getTasks().remove(t);
}
}
}
});
}
}
67 changes: 67 additions & 0 deletions src/main/java/org/jabref/gui/taskprogressmanager/TaskViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package org.jabref.gui.taskprogressmanager;

import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.concurrent.Task;
import org.jabref.Globals;
import org.jabref.JabRefGUI;
import org.jabref.gui.AbstractViewModel;
import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.duplicationFinder.DuplicateResolverDialog;
import org.jabref.gui.externalfiles.ImportHandler;
import org.jabref.gui.externalfiletype.ExternalFileTypes;
import org.jabref.gui.fieldeditors.LinkedFileViewModel;
import org.jabref.gui.groups.GroupTreeNodeViewModel;
import org.jabref.gui.groups.UndoableAddOrRemoveGroup;
import org.jabref.gui.undo.NamedCompound;
import org.jabref.gui.undo.UndoableInsertEntries;
import org.jabref.gui.undo.UndoableInsertString;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.bibtex.DuplicateCheck;
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.l10n.Localization;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.BibtexString;
import org.jabref.model.entry.LinkedFile;
import org.jabref.model.groups.GroupTreeNode;
import org.jabref.model.metadata.MetaData;
import org.jabref.model.util.FileUpdateMonitor;
import org.jabref.preferences.PreferencesService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.swing.undo.UndoManager;
import java.util.List;
import java.util.Optional;

public class TaskViewModel extends AbstractViewModel {

private static final Logger LOGGER = LoggerFactory.getLogger(TaskViewModel.class);

private final StringProperty message;
private final DialogService dialogService;
private final StateManager stateManager;
private ObservableList<Task> tasks;

public TaskViewModel(DialogService dialogService, StateManager stateManager) {
this.dialogService = dialogService;
this.stateManager = stateManager;
this.tasks = stateManager.getBackgroundTasks();
this.message = new SimpleStringProperty();
}

public String getMessage() {
return message.get();
}

public ObservableList<Task> getBackgroundTasks() {return this.tasks;};

public StringProperty messageProperty() {
return message;
}
}
5 changes: 5 additions & 0 deletions src/main/java/org/jabref/gui/util/BackgroundTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ 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);

public BackgroundTask() {
Expand Down Expand Up @@ -90,6 +91,10 @@ public StringProperty messageProperty() {
return message;
}

public StringProperty titleProperty() {
return title;
}

public double getWorkDonePercentage() {
return workDonePercentage.get();
}
Expand Down
9 changes: 8 additions & 1 deletion src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import javafx.application.Platform;
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 +98,9 @@ public static void runInJavaFXThread(Runnable runnable) {

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

@Override
Expand Down Expand Up @@ -128,8 +132,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