8246745: ListCell/Skin: misbehavior on switching skin

Reviewed-by: kcr, arapte

Author: Jeanette Winzenburg

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListCellSkin.java

@@ -26,17 +26,13 @@
 package javafx.scene.control.skin;
 
 import com.sun.javafx.scene.control.behavior.BehaviorBase;
-import javafx.beans.InvalidationListener;
-import javafx.beans.Observable;
+import com.sun.javafx.scene.control.behavior.ListCellBehavior;
+
 import javafx.geometry.Orientation;
-import javafx.scene.Node;
-import javafx.scene.control.Accordion;
-import javafx.scene.control.Button;
 import javafx.scene.control.Control;
 import javafx.scene.control.ListCell;
 import javafx.scene.control.ListView;
-
-import com.sun.javafx.scene.control.behavior.ListCellBehavior;
+import javafx.scene.layout.Region;
 
 /**
  * Default skin implementation for the {@link ListCell} control.
@@ -52,12 +48,8 @@ public class ListCellSkin<T> extends CellSkinBase<ListCell<T>> {
      *                                                                         *
      **************************************************************************/
 
-    private double fixedCellSize;
-    private boolean fixedCellSizeEnabled;
     private final BehaviorBase<ListCell<T>> behavior;
 
-
-
     /***************************************************************************
      *                                                                         *
      * Constructors                                                            *
@@ -77,31 +69,8 @@ public ListCellSkin(ListCell<T> control) {
         // install default input map for the ListCell control
         behavior = new ListCellBehavior<>(control);
 //        control.setInputMap(behavior.getInputMap());
-
-        setupListeners();
     }
 
-    private void setupListeners() {
-        ListView listView = getSkinnable().getListView();
-        if (listView == null) {
-            getSkinnable().listViewProperty().addListener(new InvalidationListener() {
-                @Override public void invalidated(Observable observable) {
-                    getSkinnable().listViewProperty().removeListener(this);
-                    setupListeners();
-                }
-            });
-        } else {
-            this.fixedCellSize = listView.getFixedCellSize();
-            this.fixedCellSizeEnabled = fixedCellSize > 0;
-            registerChangeListener(listView.fixedCellSizeProperty(), e -> {
-                this.fixedCellSize = getSkinnable().getListView().getFixedCellSize();
-                this.fixedCellSizeEnabled = fixedCellSize > 0;
-            });
-        }
-    }
-
-
-
     /***************************************************************************
      *                                                                         *
      * Public API                                                              *
@@ -127,7 +96,8 @@ private void setupListeners() {
 
     /** {@inheritDoc} */
     @Override protected double computePrefHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) {
-        if (fixedCellSizeEnabled) {
+        double fixedCellSize = getFixedCellSize();
+        if (fixedCellSize > 0) {
             return fixedCellSize;
         }
 
@@ -140,7 +110,8 @@ private void setupListeners() {
 
     /** {@inheritDoc} */
     @Override protected double computeMinHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) {
-        if (fixedCellSizeEnabled) {
+        double fixedCellSize = getFixedCellSize();
+        if (fixedCellSize > 0) {
             return fixedCellSize;
         }
 
@@ -149,10 +120,15 @@ private void setupListeners() {
 
     /** {@inheritDoc} */
     @Override protected double computeMaxHeight(double width, double topInset, double rightInset, double bottomInset, double leftInset) {
-        if (fixedCellSizeEnabled) {
+        double fixedCellSize = getFixedCellSize();
+        if (fixedCellSize > 0) {
             return fixedCellSize;
         }
-
         return super.computeMaxHeight(width, topInset, rightInset, bottomInset, leftInset);
     }
+
+    private double getFixedCellSize() {
+        ListView<?> listView = getSkinnable().getListView();
+        return listView != null ? listView.getFixedCellSize() : Region.USE_COMPUTED_SIZE;
+    }
 }

modules/javafx.controls/src/test/java/test/javafx/scene/control/ListCellTest.java

@@ -42,6 +42,7 @@
 import org.junit.Ignore;
 import org.junit.Test;
 
+import static javafx.scene.control.ControlShim.*;
 import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
 import static org.junit.Assert.*;
 
@@ -810,4 +811,28 @@ private final class FocusModelMock extends FocusModel {
         ListCell cell = new ListCell();
         cell.setSkin(new ListCellSkin(cell));
     }
+
+    /**
+     * Test that min/max/pref height respect fixedCellSize.
+     * Sanity test when fixing JDK-8246745.
+     */
+    @Test
+    public void testListCellHeights() {
+        ListCell<Object> cell =  new ListCell<>();
+        ListView<Object> listView = new ListView<>();
+        cell.updateListView(listView);
+        installDefaultSkin(cell);
+        listView.setFixedCellSize(100);
+        assertEquals("pref height must be fixedCellSize",
+                listView.getFixedCellSize(),
+                cell.prefHeight(-1), 1);
+        assertEquals("min height must be fixedCellSize",
+                listView.getFixedCellSize(),
+                cell.minHeight(-1), 1);
+        assertEquals("max height must be fixedCellSize",
+                listView.getFixedCellSize(),
+                cell.maxHeight(-1), 1);
+    }
+
+
 }

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java

@@ -39,6 +39,7 @@
 import javafx.scene.control.Button;
 import javafx.scene.control.ChoiceBox;
 import javafx.scene.control.Control;
+import javafx.scene.control.ListCell;
 import javafx.scene.control.ListView;
 import javafx.scene.control.ToolBar;
 import javafx.scene.layout.Pane;
@@ -55,6 +56,32 @@ public class SkinCleanupTest {
     private Stage stage;
     private Pane root;
 
+// ------------------ ListCell
+
+    @Test
+    public void testListCellReplaceListViewWithNull() {
+        ListCell<Object> cell =  new ListCell<>();
+        ListView<Object> listView = new ListView<>();
+        cell.updateListView(listView);
+        installDefaultSkin(cell);
+        cell.updateListView(null);
+        // 8246745: updating the old listView must not throw NPE in skin
+        listView.setFixedCellSize(100);
+    }
+
+   @Test
+   public void testListCellPrefHeightOnReplaceListView() {
+       ListCell<Object> cell =  new ListCell<>();
+       cell.updateListView(new ListView<>());
+       installDefaultSkin(cell);
+       ListView<Object> listView = new ListView<>();
+       listView.setFixedCellSize(100);
+       cell.updateListView(listView);
+       assertEquals("fixed cell set to value of new listView",
+               cell.getListView().getFixedCellSize(),
+               cell.prefHeight(-1), 1);
+   }
+
   //-------------- listView
 
     @Test

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java

@@ -110,7 +110,6 @@ public static Collection<Object[]> data() {
                 ColorPicker.class,
                 ComboBox.class,
                 DatePicker.class,
-                ListCell.class,
                 MenuBar.class,
                 MenuButton.class,
                 Pagination.class,