8242001: ChoiceBox: must update value on setting SelectionModel, part2

Reviewed-by: aghaisas
openjdk · May 8, 2020 · 4ec163d · 4ec163d
1 parent 236e2d6
commit 4ec163d
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 4 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java b/modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java
@@ -182,8 +182,7 @@ public ChoiceBox(ObservableList<T> items) {
  oldSM = sm;
  if (sm != null) {
  sm.selectedItemProperty().addListener(selectedItemListener);
- // FIXME JDK-8242001 - must sync to model state always
- if (sm.getSelectedItem() != null && ! valueProperty().isBound()) {
+ if (!valueProperty().isBound()) {
  ChoiceBox.this.setValue(sm.getSelectedItem());
  }
  }

diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxSelectionTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxSelectionTest.java
@@ -33,10 +33,13 @@
 
 import static org.junit.Assert.*;
 
+import javafx.beans.property.SimpleStringProperty;
+import javafx.beans.property.StringProperty;
 import javafx.collections.FXCollections;
 import javafx.scene.Node;
 import javafx.scene.Scene;
 import javafx.scene.control.ChoiceBox;
+import javafx.scene.control.ChoiceBoxShim;
 import javafx.scene.control.ContextMenu;
 import javafx.scene.control.Control;
 import javafx.scene.control.MenuItem;
@@ -314,6 +317,41 @@ public void testSyncedClearSelectionUncontained() {
  assertEquals(uncontained, box.getValue());
  }
 
+ //------------- tests for JDK-8242001
+
+ /**
+ * Testing JDK-8242001: box value not updated on replacing selection model.
+ *
+ * Happens if replacing.selectedItem == null
+ *
+ */
+ @Test
+ public void testSyncedContainedValueReplaceSMEmpty() {
+ box.setValue(box.getItems().get(1));
+ SingleSelectionModel<String> replaceSM = ChoiceBoxShim.get_ChoiceBoxSelectionModel(box);
+ assertNull(replaceSM.getSelectedItem());
+ box.setSelectionModel(replaceSM);
+ assertEquals(replaceSM.getSelectedItem(), box.getValue());
+ }
+
+ @Test
+ public void testSyncedUncontainedValueReplaceSMEmpty() {
+ box.setValue(uncontained);
+ SingleSelectionModel<String> replaceSM = ChoiceBoxShim.get_ChoiceBoxSelectionModel(box);
+ assertNull(replaceSM.getSelectedItem());
+ box.setSelectionModel(replaceSM);
+ assertEquals(replaceSM.getSelectedItem(), box.getValue());
+ }
+
+ @Test
+ public void testSyncedBoundValueReplaceSMEmpty() {
+ StringProperty valueSource = new SimpleStringProperty("stickyValue");
+ box.valueProperty().bind(valueSource);
+ SingleSelectionModel<String> replaceSM = ChoiceBoxShim.get_ChoiceBoxSelectionModel(box);
+ assertNull(replaceSM.getSelectedItem());
+ box.setSelectionModel(replaceSM);
+ assertEquals(valueSource.get(), box.getValue());
+ }
 
  //----------- setup and sanity test for initial state
 

diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxTest.java
@@ -342,7 +342,8 @@ protected void startApp(Parent root) {
  assertEquals("Orange", box.getValue());
  }
 
- @Test public void ensureValueIsUpdatedByCorrectSelectionModelWhenSelectionModelIsChanged() {
+ @Test
+ public void ensureValueIsUpdatedByCorrectSelectionModelWhenSelectionModelIsChanged() {
  box.getItems().addAll("Apple", "Orange", "Banana");
  SelectionModel sm1 = box.getSelectionModel();
  sm1.select(1);
@@ -351,7 +352,10 @@ protected void startApp(Parent root) {
  box.setSelectionModel(sm2);
 
  sm1.select(2); // value should not change as we are using old SM
- assertEquals("Orange", box.getValue());
+ // was: incorrect test assumption
+ // - setting the new model (with null selected item) changed the value to null
+ // assertEquals("Orange", box.getValue());
+ assertEquals(sm2.getSelectedItem(), box.getValue());
 
  sm2.select(0); // value should change, as we are using new SM
  assertEquals("Apple", box.getValue());