ParagraphText: immediately remove listeners from selections and carets on disposal (#791)

* ParagraphText: immediately remove listeners from SelectionPath and Caret on dispose instead of waiting for GC (this improves PR #779, issue #627)

* ParagraphText: since weak listeners are no longer required for selection and caret listeners, remove listener classes again and move listener code to constructor (saves 100 lines of code)

* ParagraphText: MapChangeListener.Change.wasAdded() and wasRemoved() may both return true when replacing an item. So check both and handle wasRemoved() before wasAdded().

* Fixed (rare) NPE in class ParagraphText when GC frees object between two WeakReference.get() invocations

Author: JFormDesigner

richtextfx/src/main/java/org/fxmisc/richtext/GenericStyledArea.java

@@ -1463,6 +1463,7 @@ public void dispose() {
                 box.wrapTextProperty().unbind();
                 box.graphicFactoryProperty().unbind();
                 box.graphicOffset.unbind();
+                box.dispose();
 
                 firstParPseudoClass.unsubscribe();
                 lastParPseudoClass.unsubscribe();

richtextfx/src/main/java/org/fxmisc/richtext/ParagraphBox.java

@@ -22,7 +22,6 @@
 import javafx.geometry.Insets;
 import javafx.geometry.Point2D;
 import javafx.scene.Node;
-import javafx.scene.control.IndexRange;
 import javafx.scene.layout.Region;
 import javafx.scene.paint.Paint;
 import javafx.scene.text.TextFlow;
@@ -118,6 +117,10 @@ public final ObservableMap<Selection<PS, SEG, S>, SelectionPath> selectionsPrope
         graphicOffset.addListener(obs -> requestLayout());
     }
 
+    void dispose() {
+        text.dispose();
+    }
+
     @Override
     public String toString() {
         return String.format(

richtextfx/src/main/java/org/fxmisc/richtext/ParagraphText.java

@@ -1,6 +1,5 @@
 package org.fxmisc.richtext;
 
-import java.lang.ref.WeakReference;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
@@ -19,7 +18,6 @@
 import javafx.beans.property.ObjectProperty;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.beans.value.ChangeListener;
-import javafx.beans.value.ObservableValue;
 import javafx.collections.FXCollections;
 import javafx.collections.MapChangeListener;
 import javafx.collections.ObservableMap;
@@ -62,6 +60,9 @@ class ParagraphText<PS, SEG, S> extends TextFlowExt {
             FXCollections.observableMap(new HashMap<>(1));
     public final ObservableMap<Selection<PS, SEG, S>, SelectionPath> selectionsProperty() { return selections; }
 
+    private final ChangeListener<IndexRange> selectionRangeListener;
+    private final ChangeListener<Integer> caretPositionListener;
+
     // FIXME: changing it currently has not effect, because
     // Text.impl_selectionFillProperty().set(newFill) doesn't work
     // properly for Text node inside a TextFlow (as of JDK8-b100).
@@ -95,11 +96,47 @@ public ObjectProperty<Paint> highlightTextFillProperty() {
         Val<Double> leftInset = Val.map(insetsProperty(), Insets::getLeft);
         Val<Double> topInset = Val.map(insetsProperty(), Insets::getTop);
 
-        ChangeListener<IndexRange> requestLayout1 = new SelectionRangeChangeListener<>(this);
-        selections.addListener(new SelectionsSetListener<>(leftInset, requestLayout1, topInset, this));
+        selectionRangeListener = (obs, ov, nv) -> requestLayout();
+        selections.addListener((MapChangeListener.Change<? extends Selection<PS, SEG, S>, ? extends SelectionPath> change) -> {
+            if (change.wasRemoved()) {
+                SelectionPath p = change.getValueRemoved();
+                p.rangeProperty().removeListener(selectionRangeListener);
+                p.layoutXProperty().unbind();
+                p.layoutYProperty().unbind();
 
-        ChangeListener<Integer> requestLayout2 = new CaretPositionChangeListener<>(this);
-        carets.addListener(new CaretsChangeListener<>(leftInset, requestLayout2, topInset, this));
+                getChildren().remove(p);
+            }
+            if (change.wasAdded()) {
+                SelectionPath p = change.getValueAdded();
+                p.rangeProperty().addListener(selectionRangeListener);
+                p.layoutXProperty().bind(leftInset);
+                p.layoutYProperty().bind(topInset);
+
+                getChildren().add(selectionShapeStartIndex, p);
+                updateSingleSelection(p);
+            }
+        });
+
+        caretPositionListener = (obs, ov, nv) -> requestLayout();
+        carets.addListener((SetChangeListener.Change<? extends CaretNode> change) -> {
+            if (change.wasRemoved()) {
+                CaretNode caret = change.getElementRemoved();
+                caret.columnPositionProperty().removeListener(caretPositionListener);
+                caret.layoutXProperty().unbind();
+                caret.layoutYProperty().unbind();
+
+                getChildren().remove(caret);
+            }
+            if (change.wasAdded()) {
+                CaretNode caret = change.getElementAdded();
+                caret.columnPositionProperty().addListener(caretPositionListener);
+                caret.layoutXProperty().bind(leftInset);
+                caret.layoutYProperty().bind(topInset);
+
+                getChildren().add(caret);
+                updateSingleCaret(caret);
+            }
+        });
 
         // XXX: see the note at highlightTextFill
 //        highlightTextFill.addListener(new ChangeListener<Paint>() {
@@ -182,6 +219,12 @@ public ObjectProperty<Paint> highlightTextFillProperty() {
         );
     }
 
+    void dispose() {
+        // this removes listeners (in selections and carets listeners) and avoids memory leaks
+        selections.clear();
+        carets.clear();
+    }
+
     public Paragraph<PS, SEG, S> getParagraph() {
         return paragraph;
     }
@@ -402,131 +445,6 @@ protected void layoutChildren() {
         updateBackgroundShapes();
     }
 
-    private static final class SelectionsSetListener<PS, SEG, S> implements
-                MapChangeListener<Selection<PS, SEG, S>, SelectionPath> {
-        private final Val<Double> leftInset;
-        private final ChangeListener<IndexRange> requestLayout1;
-        private final Val<Double> topInset;
-        private final WeakReference<ParagraphText<PS, SEG, S>> ref;
-
-        public SelectionsSetListener(
-                    Val<Double> leftInset,
-                    ChangeListener<IndexRange> requestLayout1,
-                    Val<Double> topInset,
-                    ParagraphText<PS, SEG, S> paragraphText) {
-            this.leftInset = leftInset;
-            this.requestLayout1 = requestLayout1;
-            this.topInset = topInset;
-            ref = new WeakReference<>(paragraphText);
-        }
-
-        @Override
-        public void onChanged(
-                    javafx.collections.MapChangeListener.Change<? extends Selection<PS, SEG, S>, ? extends SelectionPath> change) {
-            ParagraphText<PS, SEG, S> paragraphText = ref.get();
-            if (null == paragraphText) {
-                change.getMap().removeListener(this);
-                return;
-            }
-
-            if (change.wasAdded()) {
-                SelectionPath p = change.getValueAdded();
-                p.rangeProperty().addListener(requestLayout1);
-
-                p.layoutXProperty().bind(leftInset);
-                p.layoutYProperty().bind(topInset);
-
-                paragraphText.getChildren().add(paragraphText.selectionShapeStartIndex, p);
-                paragraphText.updateSingleSelection(p);
-            } else if (change.wasRemoved()) {
-                SelectionPath p = change.getValueRemoved();
-                p.rangeProperty().removeListener(requestLayout1);
-
-                p.layoutXProperty().unbind();
-                p.layoutYProperty().unbind();
-
-                paragraphText.getChildren().remove(p);
-            }
-        }
-    }
-
-    private static final class SelectionRangeChangeListener<PS, SEG, S> implements ChangeListener<IndexRange> {
-        private final WeakReference<ParagraphText<PS, SEG, S>> ref;
-
-        public SelectionRangeChangeListener(ParagraphText<PS, SEG, S> paragraphText) {
-            ref = new WeakReference<>(paragraphText);
-        }
-
-        @Override
-        public void changed(ObservableValue<? extends IndexRange> observable, IndexRange oldValue, IndexRange newValue) {
-            if (null == ref.get()) {
-                observable.removeListener(this);
-            } else {
-                ref.get().requestLayout();
-            }
-        }
-    }
-
-    private static final class CaretPositionChangeListener<PS, SEG, S> implements ChangeListener<Integer> {
-        private final WeakReference<ParagraphText<PS, SEG, S>> ref;
-
-        public CaretPositionChangeListener(ParagraphText<PS, SEG, S> paragraphText) {
-            ref = new WeakReference<>(paragraphText);
-        }
-
-        @Override
-        public void changed(ObservableValue<? extends Integer> observable, Integer oldValue, Integer newValue) {
-            if (null == ref.get()) {
-                observable.removeListener(this);
-            } else {
-                ref.get().requestLayout();
-            }
-        }
-    }
-
-    private static final class CaretsChangeListener<PS, SEG, S> implements SetChangeListener<CaretNode> {
-        private final Val<Double> leftInset;
-        private final ChangeListener<Integer> requestLayout2;
-        private final Val<Double> topInset;
-        private final WeakReference<ParagraphText<PS, SEG, S>> ref;
-
-        private CaretsChangeListener(
-                    Val<Double> leftInset,
-                    ChangeListener<Integer> requestLayout2,
-                    Val<Double> topInset,
-                    ParagraphText<PS, SEG, S> paragraphText) {
-            ref = new WeakReference<>(paragraphText);
-            this.leftInset = leftInset;
-            this.requestLayout2 = requestLayout2;
-            this.topInset = topInset;
-        }
-
-        @Override
-        public void onChanged(Change<? extends CaretNode> change) {
-            ParagraphText<PS, SEG, S> paragraphText = ref.get();
-            if (null == paragraphText) {
-                change.getSet().removeListener(this);
-                return;
-            }
-            if (change.wasAdded()) {
-                CaretNode caret = change.getElementAdded();
-                caret.columnPositionProperty().addListener(requestLayout2);
-                caret.layoutXProperty().bind(leftInset);
-                caret.layoutYProperty().bind(topInset);
-
-                paragraphText.getChildren().add(caret);
-                paragraphText.updateSingleCaret(caret);
-            } else if (change.wasRemoved()) {
-                CaretNode caret = change.getElementRemoved();
-                caret.columnPositionProperty().removeListener(requestLayout2);
-                caret.layoutXProperty().unbind();
-                caret.layoutYProperty().unbind();
-
-                paragraphText.getChildren().remove(caret);
-            }
-        }
-    }
-
     private static class CustomCssShapeHelper<T> {
 
         private final List<Tuple2<T, IndexRange>> ranges = new LinkedList<>();