Post change notifications on JavaFX (#4871)

* Post change notifications on JavaFX

Fixes #4817. The problem is that changes in other threads produce a "Not on FX application thread" exception. This is fixed by adding a wrapper around the list of entries. The wrapper only posts changes in the correct thread, without making a copy of the underlying list - so performance shouldn't be affected.

* fix compile error

* fix checkstyle

Author: tobiasdiez

src/main/java/org/jabref/gui/BasePanel.java

@@ -255,7 +255,7 @@ public void output(String s) {
 
     private void setupActions() {
         SaveDatabaseAction saveAction = new SaveDatabaseAction(this, Globals.prefs);
-        CleanupAction cleanUpAction = new CleanupAction(this, Globals.prefs);
+        CleanupAction cleanUpAction = new CleanupAction(this, Globals.prefs, Globals.TASK_EXECUTOR);
 
         actions.put(Actions.UNDO, undoAction);
         actions.put(Actions.REDO, redoAction);

src/main/java/org/jabref/gui/actions/CleanupAction.java

@@ -9,6 +9,8 @@
 import org.jabref.gui.cleanup.CleanupDialog;
 import org.jabref.gui.undo.NamedCompound;
 import org.jabref.gui.undo.UndoableFieldChange;
+import org.jabref.gui.util.BackgroundTask;
+import org.jabref.gui.util.TaskExecutor;
 import org.jabref.logic.cleanup.CleanupPreset;
 import org.jabref.logic.cleanup.CleanupWorker;
 import org.jabref.logic.l10n.Localization;
@@ -20,15 +22,17 @@ public class CleanupAction implements BaseAction {
 
     private final BasePanel panel;
     private final DialogService dialogService;
+    private final TaskExecutor taskExecutor;
 
     private boolean isCanceled;
     private int modifiedEntriesCount;
     private final JabRefPreferences preferences;
 
-    public CleanupAction(BasePanel panel, JabRefPreferences preferences) {
+    public CleanupAction(BasePanel panel, JabRefPreferences preferences, TaskExecutor taskExecutor) {
         this.panel = panel;
         this.preferences = preferences;
         this.dialogService = panel.frame().getDialogService();
+        this.taskExecutor = taskExecutor;
     }
 
     @Override
@@ -58,8 +62,9 @@ public void action() {
 
             preferences.setCleanupPreset(chosenPreset.get());
 
-            this.cleanup(chosenPreset.get());
-            this.showResults();
+            BackgroundTask.wrap(() -> cleanup(chosenPreset.get()))
+                          .onSuccess(result -> showResults())
+                          .executeWith(taskExecutor);
         }
     }
 

src/main/java/org/jabref/gui/maintable/MainTable.java

@@ -36,6 +36,7 @@
 import org.jabref.gui.keyboard.KeyBindingRepository;
 import org.jabref.gui.undo.NamedCompound;
 import org.jabref.gui.undo.UndoableInsertEntry;
+import org.jabref.gui.util.BindingsHelper;
 import org.jabref.gui.util.CustomLocalDragboard;
 import org.jabref.gui.util.ViewModelTableRowFactory;
 import org.jabref.logic.l10n.Localization;
@@ -108,7 +109,7 @@ public MainTable(MainTableDataModel model, JabRefFrame frame,
         }
         this.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
 
-        this.setItems(model.getEntriesFilteredAndSorted());
+        this.setItems(BindingsHelper.forUI(model.getEntriesFilteredAndSorted()));
         // Enable sorting
         model.getEntriesFilteredAndSorted().comparatorProperty().bind(this.comparatorProperty());
 

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

@@ -60,6 +60,7 @@ public static <A, B> MappedList<B, A> mapBacked(ObservableList<A> source, Functi
 
     public static <T, U> ObservableList<U> map(ObservableValue<T> source, Function<T, List<U>> mapper) {
         PreboundBinding<List<U>> binding = new PreboundBinding<List<U>>(source) {
+
             @Override
             protected List<U> computeValue() {
                 return mapper.apply(source.getValue());
@@ -103,10 +104,10 @@ public static <A, B> void bindBidirectional(ObservableValue<A> propertyA, Observ
 
     public static <A, B> void bindContentBidirectional(ObservableList<A> propertyA, ListProperty<B> propertyB, Consumer<ObservableList<B>> updateA, Consumer<List<A>> updateB) {
         bindContentBidirectional(
-                propertyA,
-                (ObservableValue<ObservableList<B>>) propertyB,
-                updateA,
-                updateB);
+                                 propertyA,
+                                 (ObservableValue<ObservableList<B>>) propertyB,
+                                 updateA,
+                                 updateB);
     }
 
     public static <A, B> void bindContentBidirectional(ObservableList<A> propertyA, ObservableValue<B> propertyB, Consumer<B> updateA, Consumer<List<A>> updateB) {
@@ -124,10 +125,10 @@ public static <A, B> void bindContentBidirectional(ListProperty<A> listProperty,
         Consumer<List<A>> updateB = newValueList -> property.setValue(mapToB.apply(newValueList));
 
         bindContentBidirectional(
-                listProperty,
-                property,
-                updateList,
-                updateB);
+                                 listProperty,
+                                 property,
+                                 updateList,
+                                 updateB);
     }
 
     public static <A, V, B> void bindContentBidirectional(ObservableMap<A, V> propertyA, ObservableValue<B> propertyB, Consumer<B> updateA, Consumer<Map<A, V>> updateB) {
@@ -143,14 +144,15 @@ public static <A, V, B> void bindContentBidirectional(ObservableMap<A, V> proper
     public static <A, V, B> void bindContentBidirectional(ObservableMap<A, V> propertyA, Property<B> propertyB, Consumer<B> updateA, Function<Map<A, V>, B> mapToB) {
         Consumer<Map<A, V>> updateB = newValueList -> propertyB.setValue(mapToB.apply(newValueList));
         bindContentBidirectional(
-                propertyA,
-                propertyB,
-                updateA,
-                updateB);
+                                 propertyA,
+                                 propertyB,
+                                 updateA,
+                                 updateB);
     }
 
     public static <T> ObservableValue<T> constantOf(T value) {
         return new ObjectBinding<T>() {
+
             @Override
             protected T computeValue() {
                 return value;
@@ -160,6 +162,7 @@ protected T computeValue() {
 
     public static ObservableValue<Boolean> constantOf(boolean value) {
         return new BooleanBinding() {
+
             @Override
             protected boolean computeValue() {
                 return value;
@@ -169,13 +172,21 @@ protected boolean computeValue() {
 
     public static ObservableValue<? extends String> emptyString() {
         return new StringBinding() {
+
             @Override
             protected String computeValue() {
                 return "";
             }
         };
     }
 
+    /**
+     * Returns a wrapper around the given list that posts changes on the JavaFX thread.
+     */
+    public static <T> ObservableList<T> forUI(ObservableList<T> list) {
+        return new UiThreadList<>(list);
+    }
+
     private static class BidirectionalBinding<A, B> {
 
         private final ObservableValue<A> propertyA;

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

@@ -0,0 +1,32 @@
+package org.jabref.gui.util;
+
+import javafx.application.Platform;
+import javafx.collections.ListChangeListener;
+import javafx.collections.ObservableList;
+import javafx.collections.transformation.TransformationList;
+
+class UiThreadList<T> extends TransformationList<T, T> {
+    public UiThreadList(ObservableList<? extends T> source) {
+        super(source);
+    }
+
+    @Override
+    protected void sourceChanged(ListChangeListener.Change<? extends T> change) {
+        Platform.runLater(() -> fireChange(change));
+    }
+
+    @Override
+    public int getSourceIndex(int index) {
+        return index;
+    }
+
+    @Override
+    public T get(int index) {
+        return getSource().get(index);
+    }
+
+    @Override
+    public int size() {
+        return getSource().size();
+    }
+}