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

Conversation

btut
Copy link
Contributor

@btut btut commented May 7, 2020

Implements a background-task progress indicator in JabRefs toolbar as first discussed in #6381 (comment).
Screenshot from 2020-05-10 14-08-47

The indicator is located at the right-most position in the toolbar. Clicking it opens a pop-over that lists the background tasks with an icon, title, message and progress.

In order not to overwhelm the user with background tasks, only interesting ones are shown. To make a background task show, it needs to have the showToUser property set. For it to have a meaningful entry, one should set the title and message to appropriate values. If progress can be tracked, that should be done as well. If not, an indeterminate indicator / progress bar is shown.
To set the icon of the task, add a mapping from the task-title to an icon to the iconMap in BackgroundTask.java

  • Change in CHANGELOG.md described (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.

btut added 2 commits May 7, 2020 16:38
Implemented a task progress dialog using a TaskProgressView.
Tasks show up, but without info. Neither the progress nor title and
message are shown.
There now i a progress indicator at the rightmost postition of JabRefs
toolbar. It shows the average progress of all running background tasks.
Clicking it will show a TaskProgressDialog.

still looks ugly and the binding to the average progress does not seem
to be working.
@calixtus calixtus changed the title WIP: Implement task progress indicator (and dialog) in the toolbar [WIP] Implement task progress indicator (and dialog) in the toolbar May 7, 2020
@calixtus calixtus marked this pull request as draft May 7, 2020 21:36
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

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!

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!

btut added 3 commits May 8, 2020 11:17
This makes the view work with download tasks for example. Most other
tasks are still shown without title, message (because none are set) and
progress.
Also, there are a lot of tasks somehow.

The progress indicator in the main view still does not work as I can't
get the bindings to work.
The progress indicator is now successfully bound to the list of tasks.
However, tasks do not show up in the dialogue any more.
When using ObjectProperties in the list of tasks, we can use EasyBind to
convert the list into a list of tasks which in turn can be bound to the
list of tasks in the view.

With this, the basic functionality works. There is a progress indicator
in the toolbar that shows the average progress of all running background
tasks. It is indeterminate if any task has indeterminate progress and
shows 100% if no tasks are running.

Clicking it opens an overview of all running tasks and their progress.

Currently, there are many tasks running all the time. The only tasks
that were adapted for this to be pretty are the download tasks, and they
are also still missing an icon.
@btut
Copy link
Contributor Author

btut commented May 8, 2020

I would like to start a discussion here.

Right now, both the dialogue and the view are working (not perfect and pretty, but working).
At the very least, we have to manage the size of the vie wetter, as right now the progressbars are larger than the window so there is a horizontal scroll bar.
Also, we have to give all background tasks appropriate titles and messages for the view to be meaningful.

Also, there are many background tasks even after just starting up JabRef:
image
I think this is a little overwhelming.
I suggest three options and am open to any ideas:

  • Leave it as it is
  • Only add certain tasks (like download tasks)
  • Show only tasks that are currently running. This will hide most startup tasks and the user will never see most of the tasks as they are too quick to complete.

@Siedlerchr
Copy link
Member

I would only add some tasks. Most tasks which are run in the background are irrelevant to the user

btut added 3 commits May 8, 2020 15:15
These are shorter and therefore the task progress view does not need a
horizontal scroll bar.
@btut
Copy link
Contributor Author

btut commented May 8, 2020

I changed it to only show download tasks. This looks much better!
Screenshot from 2020-05-08 16-14-59
I also changed the message to display the cite key instead of the url. The long url made the label in the task progress view so wide that it needed a scrollbar. Like this, it fits.

As you can see the progress indicator broke again, but I guess it never really displayed the correct value, the other tasks make me think that it did.

So next up I will fix that and then I will introduce a dialog that warns the user in case JabRef is closed during an ongoing download.

@calixtus
Copy link
Member

calixtus commented May 8, 2020

I would say: display all tasks that have a title (or put a property in the task: visible). So in theory, you could also add another button "show all" (or something similar).

@calixtus
Copy link
Member

calixtus commented May 8, 2020

Btw. I think this is a huge improvement to JabRef. Thank you for your effort.

@btut
Copy link
Contributor Author

btut commented May 8, 2020

I would say: display all tasks that have a title (or put a property in the task: visible). So in theory, you could also add another button "show all" (or something similar).

I went with a property showToUser, a button to show all tasks does sound interesting, but I think that one only makes sense if all tasks set the message and title properties. When looking at the task-list, I guess that might be many pieces of code to adapt. Maybe it is better not to do that at first and maybe add that functionality later.

@btut
Copy link
Contributor Author

btut commented May 8, 2020

Btw. I think this is a huge improvement to JabRef. Thank you for your effort.

Glad to be of help. It's a welcome distraction from my master's thesis :)

@calixtus
Copy link
Member

calixtus commented May 8, 2020

Glad to be of help. It's a welcome distraction from my master's thesis :)

Yeah. This is exactly how I came (back) to JabRef and JabRef programming. 😆

@@ -41,6 +48,7 @@
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!

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.

Thanks again! Looks really good to me. Your solution for the width problem is also fine for me.

I have a few remarks, but nothing big.

src/main/java/org/jabref/gui/Base.css Show resolved Hide resolved
src/main/java/org/jabref/gui/JabRefFrame.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/StateManager.java Outdated Show resolved Hide resolved
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

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

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java Outdated Show resolved Hide resolved
@btut btut requested a review from tobiasdiez May 12, 2020 06:19
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.

Thanks a lot for the quick follow-up! Looks very good to me new.

(I've slightly changed the getIcon method - hopefully without destroying anything).

@btut
Copy link
Contributor Author

btut commented May 12, 2020

Great! Thanks for the quick review! There is a reason though I passed Objects to getIcon, that's the signature needed for setGraphicFactory in TaskProgressView.

@tobiasdiez
Copy link
Member

Ok I see, can you then please revert my two last commits (stupid me...sorry actually wanted to help :()

@btut
Copy link
Contributor Author

btut commented May 12, 2020

Ok I see, can you then please revert my two last commits (stupid me...sorry actually wanted to help :()

Happens to the best :)

I have a couple of questions that are kind of off-topic here, I hope you don't mind:

  • Why is the deployment failing?
  • Do .deb packages built from current master contain a repository where updates can be fetched via apt? Or does one have to regularly fetch the new deb's himself?
  • If these are not updated, how fast du you think these changes will make it into the ubuntu repos?

Completely unrelated:

  • I found a bug in JabRef and am working on a fix. Shall I create an issue anyway or just create a PR once I am done?

@btut
Copy link
Contributor Author

btut commented May 12, 2020

Completely unrelated:

* I found a bug in JabRef and am working on a fix. Shall I create an issue anyway or just create a PR once I am done?

I am already done and just create a PR. I hope that is fine with you.

@tobiasdiez
Copy link
Member

tobiasdiez commented May 12, 2020

Why is the deployment failing?

It's a problem with our build server (no space left). You can safely ignore it.

Do .deb packages built from current master contain a repository where updates can be fetched via apt? Or does one have to regularly fetch the new deb's himself?

The easiest way is to manually download the deb file or use snap.

If these are not updated, how fast du you think these changes will make it into the ubuntu repos?

We had problems with the official ubuntu repo. This is why the version there is pretty old (3.8 if I remember correctly). If you are interested, feel free to revisit the problem - maybe now that we provide deb packages this might be easier. @koppor may tell you more about the issues. Refs #1371

I found a bug in JabRef and am working on a fix. Shall I create an issue anyway or just create a PR once I am done?

You don't need to create a issue before submitting a PR.

@tobiasdiez tobiasdiez removed the status: changes required Pull requests that are not yet complete label May 12, 2020
Localization.lang("Please wait..."),
Localization.lang("Waiting for background tasks to finish. Quit anyway?"),
stateManager
).orElse(ButtonType.CANCEL) == ButtonType.YES)) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks odd to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please elaborate?
If you mean the condition at the end, I just want to make sure that JabRef only exists if the user pressed yes. So if the optional is empty I just put ButtonType.CANCEL so the comparison to YES returns false.
Do you want me to change this or add a comment? Maybe it would be clearer to store the result in a variable and check for (result.isPresent() && result.get() == ButtonType.YES).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think this is more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 3db3997

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

In general looks good to me, there is only that one piece of code which looks wrong or odd to me

@btut
Copy link
Contributor Author

btut commented May 12, 2020

We had problems with the official ubuntu repo. This is why the version there is pretty old (3.8 if I remember correctly). If you are interested, feel free to revisit the problem - maybe now that we provide deb packages this might be easier. @koppor may tell you more about the issues. Refs #1371

The one I was using on Ubuntu 20.04 is 5.0.50001-1. Thats what you get from the official repos.

@tobiasdiez tobiasdiez merged commit 4423d27 into JabRef:master May 12, 2020
@tobiasdiez
Copy link
Member

Thanks again!

@btut
Copy link
Contributor Author

btut commented May 13, 2020

Thanks for all your help along the way and for JabRef!

Siedlerchr added a commit that referenced this pull request May 16, 2020
* upstream/master: (50 commits)
  Keep group pane size when resizing window (#6180) (#6423)
  Changelog: Fix missing citation for biblatex-mla
  Update AUTHORS
  Check duplicate DOI (#6333)
  Fix missing citation for biblatex-mla
  Change EasyBind dependency (#6480)
  Add testing of latest dev version as mandatory
  Squashed 'src/main/resources/csl-styles/' changes from 5dad23d..586e0b8
  Fix libre office connection and other progress dialogs (#6478)
  Fix clear year and month field when converting to biblatex (#6434)
  Add truncate as a BibTex key modifier (#6427)
  Add new authors (not all - they need more work)
  Remove empty line
  Add simple Unit Tests for #6207 (#6240)
  Enforce LeftCurly rule (#6452)
  Implement task progress indicator (and dialog) in the toolbar (#6443)
  Consider empty brackets
  Changelog update
  Added a test
  Fixed brackets in regular expressions
  ...
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.

4 participants