Abbreviate action update #5441

Brainsucker92 · 2019-10-14T11:04:07Z

REQUIRES TESTING!

I removed unnecessary field dependencies from the AbbreviateAction class to make it much more flexible to use. There is no longer a BasePanel required to abbreviate BibEntries.
Also I split up the relatively big abbreviation task into a lot of smaller tasks, which increase concurrency a lot (divide and conquer FTW!)

Important events during the abbreviation process can now be listened to via the AbbreviateActionListener interface or AbbreviateActionAdapter class.

Unfortunately I cannot test any of the changes since I don't have any big database to run an abbreviation on.
Is there any testing data available?

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2019-10-14T11:11:38Z

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

-    public <V> Future<?> schedule(BackgroundTask<V> task, long delay, TimeUnit unit) {
-        return scheduledExecutor.schedule(getJavaFXTask(task), delay, unit);
+    public <V> Future<V> schedule(BackgroundTask<V> task, long delay, TimeUnit unit) {
+        return scheduledExecutor.schedule(task::call, delay, unit);


Why did you remove the getJavaFX call?

Delegating less tasks to the JavaFX thread helps to prevent any possible GUI freezes. The JavaFX Thread should be used for UI related stuff only. This helps to keep the GUI interactive and improve user experience when working with JabRef. Actually there are way too many methods that delegate tasks to the UI thread. One of them should be sufficient (at most). Everything else should be executed by another ExecutorService.

Update: I added the method call again and managed to generify the Method so it no longer returns Future<?> which looks horrible to me.

tobiasdiez

Thanks for your contribution. It's really good that somebody works on the parallelization. However, the current code needs more work.

tobiasdiez · 2019-10-14T17:13:15Z

src/main/java/org/jabref/gui/copyfiles/CopyFilesAction.java

@@ -56,7 +57,7 @@ public void execute() {
                    Localization.lang("Copy linked files to folder..."),
                    Localization.lang("Copy linked files to folder..."),
                    exportTask);
-            Globals.TASK_EXECUTOR.execute(exportTask);
+            BackgroundTask.wrap(exportTask).executeWith(Globals.TASK_EXECUTOR);


This doesn't seem to be equivalent to the previous code as the completion listerners of the CopyFilesTask are no longer invoked. Wrap exportTask just takes the run command from exportTask.

tobiasdiez · 2019-10-14T17:14:50Z

src/main/java/org/jabref/gui/util/BackgroundTask.java

@@ -118,6 +129,7 @@ public BackgroundProgress getProgress() {
     * Sets the {@link Consumer} that is invoked after the task is successfully finished.
     * The consumer always runs on the JavaFX thread.
     */
+    @Deprecated


why did you deprecated the onSuccess and onFailure method?

This functionality is now handled by the CompletableFuture object. See here. I really don't like the fact that the chained task is always executed on the JavaFX thread, which potentially leads to GUI freezes. See the java doc of Platform.runLater:

* NOTE: applications should avoid flooding JavaFX with too many * pending Runnables. Otherwise, the application may become unresponsive. * Applications are encouraged to batch up multiple operations into fewer * runLater calls. * Additionally, long-running operations should be done on a background * thread where possible, freeing up the JavaFX Application Thread for GUI * operations.

Therefore this method has to be deprecated as long as it's 'flooding' the JavaFX thread.

These background tasks are not executed on the javafx thread! Only the onsucess/onfailure methods are, which is really nice since you usually want to display messages there which should be done on the javafx thread. Thus this functionality is absolutely not covered by the completetable future.

The javafx Task Api is similar to the old swing worker.
Running stuff on a background thread, but having the possibility to have updates etc on the FX thread:
https://docs.oracle.com/javase/8/javafx/interoperability-tutorial/concurrency.htm

tobiasdiez · 2019-10-14T17:15:45Z

src/main/java/org/jabref/gui/texparser/ParseTexResultViewModel.java

@@ -70,7 +73,7 @@ public void activeReferenceChanged(ReferenceViewModel reference) {
     * Search and import unknown references from associated BIB files.
     */
    public void importButtonClicked() {
-        ImportEntriesDialog dialog = new ImportEntriesDialog(databaseContext, BackgroundTask.wrap(() ->
+        ImportEntriesDialog dialog = new ImportEntriesDialog(databaseContext, BackgroundTask.wrap((Callable<List<BibEntry>>) () ->


Why is it now necessary to add all these Callable statements. This is really ugly.

That's because of the Supplier method. Both the Supplier and Callable are functional interfaces, which require explicit type casting now. But I do agree, it's kind of ugly.

tobiasdiez · 2019-10-14T17:18:49Z

src/main/java/org/jabref/gui/util/BackgroundTask.java

@@ -49,6 +51,15 @@ protected V call() throws Exception {
        };
    }

+    public static <V> BackgroundTask<V> wrap(Supplier<V> supplier) {


What's the reason for this addition? Callable and Supplier are essentially equivalent, just that callable allows checked exceptions (which is what we want).

When working with CompletableFuture it's always handy to have a method available that accepts the Supplier interface. The CompletableFuture only accepts either Runnable or Supplier types when submitting a new task. Check out the static methods here

Ok, but there shouldn't ever be a need to use these static methods directly as everything should be wrapped by a background task. Please remove this for the moment. In case, we need it later it's easy to add.

tobiasdiez · 2019-10-14T17:20:17Z

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

-    public <V> Future<?> schedule(BackgroundTask<V> task, long delay, TimeUnit unit) {
-        return scheduledExecutor.schedule(getJavaFXTask(task), delay, unit);
+    public <V> Future<V> schedule(BackgroundTask<V> task, long delay, TimeUnit unit) {
+        return scheduledExecutor.schedule(() -> getJavaFXTask(task).get(), delay, unit);


As above, this is not equivalent. You really need to submit the task, otherwise it doesn't trigger the handlers.

tobiasdiez · 2019-10-14T17:21:11Z

src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java

-        executor.submit(task);
-        return task;
+    public <V> CompletableFuture<V> execute(Task<V> task) {
+        CompletableFuture<V> future = new CompletableFuture<>();


The future is never completed. There seems to be something off here

tobiasdiez · 2019-10-14T17:25:51Z

src/main/java/org/jabref/gui/journals/AbbreviateAction.java

-    public AbbreviateAction(BasePanel panel, boolean iso) {
-        this.panel = panel;
-        this.iso = iso;
+    public AbbreviateAction(Supplier<List<BibEntry>> bibEntryListSupplier, Supplier<UndoableAbbreviator> undoableAbbreviatorSupplier, Supplier<BibDatabase> databaseSupplier, Supplier<NamedCompound> compoundSupplier, AbbreviateActionListener listener) {


The point of these action classes is that they represent a single user interaction. Thus, there should be no (almost no) outside configuration possible. For this reason, I'm not a big fan of your changes.
For example, designing wise it's not bad that the AbbreviateAction handles logging and submitting of changes (although DialogService and UndoManager should be used directly insdead of BasePanel).

Most of the 'outside configuration' has been passed into the class via the BasePanel object previously. This field dependecy to require a certain part of the GUI when you're actually processing data is completely dubious in my opinion. Therefore I simply removed this GUI related requirement, which makes this class a lot more flexible but still doing pretty much the same job as before.

Plase keep in mind not to 'weave' GUI components and data processing components into each other as this is really bad practice from an architectural point of view. But this is exactly what happenend in this case.

I agree that there should be a clear separation of logic and gui, and most of JabRef's code sucks in this regard (although its getting better). However, the abbreviation class is not an example in my opinion. In contrast, it's actually quite good. Its prepares the input (get selected entries), invokes the logic class for the main action (UndoableAbbreviator) and then cleans up (submit changes to undo manager). That's relatively clean and straightforward (you could argue that the line for (Field journalField : FieldFactory.getJournalNameFields()) { should be moved to UndoableAbbreviator as it is business logic). You have created a flexible interface around the already pretty flexible UndoableAbbreviator - and I don't see the need for this honestly.

One should keep in mind that the AbbreviateAction is a GUI class and thus should handle the UI interaction part of the feature. The base panel is definitely not the right place to do this as it is too big even now.

Its prepares the input (get selected entries), invokes the logic class for the main action (UndoableAbbreviator) and then cleans up (submit changes to undo manager).

Everything you described here is a clear indication of a data processing object and has to be strictly separated from any GUI related stuff. Therefore this class cannot be part of the GUI.
Even the JavaDoc of the class itself clearly indicates the manipulation of data which is not supposed to happen inside a GUI related object at all.
Converts journal full names to either iso or medline abbreviations for all selected entries.

Following this model shown below, there has to be a strict separation of the three main layers
(data, controller, UI) to provide a clean software architecture. (as we agreed already)
http://web.mit.edu/6.813/www/sp18/classes/05-ui-sw-arch/figures/05.png

These layers are supposed to communicate via interfaces to exchange data/information as needed.
I'm afraid to say, that there is currently a lot of data management/processing going on within the BasePanel class, which should be part of the data layer instead of the GUI layer.
Have a look at BibDatabaseContext for example. Usually a GUI does not need such information to work. Instead this class is used to initialize even more data related stuff like SuggestionProviders and MainTableDataModel

I'm very sorry to say, but in conclusion there is no doubt, that there is currently a huge mess going on in these GUI classes. And that's exactly what I was trying to achieve with this pull request - decouple data processing from GUI.

Best way to exchange the required data between data layer and GUI layer would be to create an interface that allows the BasePanel to retrieve exactly the data it requires or another interface to set the data into the GUI as it's available (that's by the way exactly what I programmed in this PR. Have a look at AbbreviateActionListener)

This way we'll achieve clean code and best maintainability for the whole project.

In general, I totally agree. I think you just misunderstood how this class fits into JabRef's general architecture. Roughly speaking, data = model package, logic = logic package and gui = gui package. There does not need to be a further separation of "logic" and "data" in the gui as you cannot separate user input ("data") from initial pre-progressing ("logic") in a meaningful way.

If you want to see a real bad example: https://github.com/JabRef/jabref/blob/master/src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java
There the code is doing a lot of actual business logic that should be moved to the logic package. (But please don't start to refactor this code, it's so integral to JabRef that we do not want to touch it before the 5.0 release.)

tobiasdiez · 2019-10-14T17:26:18Z

src/main/java/org/jabref/JabRefExecutorService.java

@@ -96,6 +101,24 @@ public void executeAndWait(Runnable command) {
        }
    }

+    public <T> CompletableFuture<T> execute(Supplier<T> supplier) {


Please remove these methods (they are not used).

koppor · 2019-10-15T17:00:40Z

I think, this is highly controversal.

I would ask to leave everything as is for now. We should discuss that at JabCon 2020. - Currently, we need our energy to fix other issues. See for instance the high priority issues at https://github.com/JabRef/jabref/projects/5.

koppor · 2019-10-15T17:04:52Z

What we do need architecture-wise is a focus on #110. This should not touch the GUI code that much.

I especially need net.sf.jabref.bst.VM, because then bst files are directly supported by JabRef!

Siedlerchr · 2019-10-17T20:05:02Z

Thanks for your contribution to JabRef. We appreciate that. However, in this case it's a bit dificult. We understand your good intention, however, this is a crucial part of JabRef's logic.

Brainsucker92 added 5 commits October 12, 2019 02:33

Overload execute methods to support CompletableFuture

5b664e3

Add methods to invoke interruptible tasks with CompletableFuture

b48aa69

Add deprecated annotation

149bd43

Update to CompletableFuture pattern

a2a022a

Add deprecate java doc

b71705b

Siedlerchr reviewed Oct 14, 2019

View reviewed changes

Brainsucker92 added 3 commits October 14, 2019 14:31

Update files to new BackgroundTask

13d2d79

Major update to increase flexibility and throughput of abbreviations

ea6dfc6

Generify schedule method

a3f7ff8

tobiasdiez requested changes Oct 14, 2019

View reviewed changes

koppor closed this Oct 15, 2019

koppor added the freeze label Oct 15, 2019

koppor mentioned this pull request Oct 15, 2019

JabRefExecutorService improvement #5439

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abbreviate action update #5441

Abbreviate action update #5441

Brainsucker92 commented Oct 14, 2019

Siedlerchr Oct 14, 2019

Brainsucker92 Oct 14, 2019

Brainsucker92 Oct 14, 2019

tobiasdiez left a comment

tobiasdiez Oct 14, 2019

tobiasdiez Oct 14, 2019

Brainsucker92 Oct 14, 2019

tobiasdiez Oct 14, 2019

Siedlerchr Oct 14, 2019

tobiasdiez Oct 14, 2019

Brainsucker92 Oct 14, 2019

tobiasdiez Oct 14, 2019

Brainsucker92 Oct 14, 2019

tobiasdiez Oct 14, 2019

tobiasdiez Oct 14, 2019

tobiasdiez Oct 14, 2019

tobiasdiez Oct 14, 2019

Brainsucker92 Oct 14, 2019

tobiasdiez Oct 14, 2019

Brainsucker92 Oct 14, 2019

tobiasdiez Oct 15, 2019

tobiasdiez Oct 14, 2019

koppor commented Oct 15, 2019

koppor commented Oct 15, 2019

Siedlerchr commented Oct 17, 2019

Abbreviate action update #5441

Abbreviate action update #5441

Conversation

Brainsucker92 commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Oct 15, 2019

koppor commented Oct 15, 2019

Siedlerchr commented Oct 17, 2019