diff --git a/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java b/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java --- a/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java +++ b/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java @@ -746,55 +746,67 @@ * * **********************************************************************/ - private ReadOnlyUnbackedObservableList createListFromBitSet(final BitSet bitset) { - return new ReadOnlyUnbackedObservableList() { - private int lastGetIndex = -1; - private int lastGetValue = -1; + ReadOnlyUnbackedObservableList createListFromBitSet(final BitSet bitset) { + return new ReadOnlyBitSetObservableList(bitset); + } - @Override public Integer get(int index) { - final int itemCount = getItemCount(); - if (index < 0 || index >= itemCount) return -1; + class ReadOnlyBitSetObservableList extends ReadOnlyUnbackedObservableList { + private int lastGetIndex = -1; + private int lastGetValue = -1; - if (index == (lastGetIndex + 1) && lastGetValue < itemCount) { - // we're iterating forward in order, short circuit for - // performance reasons (RT-39776) - lastGetIndex++; - lastGetValue = bitset.nextSetBit(lastGetValue + 1); - return lastGetValue; - } else if (index == (lastGetIndex - 1) && lastGetValue > 0) { - // we're iterating backward in order, short circuit for - // performance reasons (RT-39776) - lastGetIndex--; - lastGetValue = bitset.previousSetBit(lastGetValue - 1); - return lastGetValue; - } else { - for (lastGetIndex = 0, lastGetValue = bitset.nextSetBit(0); - lastGetValue >= 0 || lastGetIndex == index; - lastGetIndex++, lastGetValue = bitset.nextSetBit(lastGetValue + 1)) { - if (lastGetIndex == index) { - return lastGetValue; - } + private final BitSet bitset; + + public ReadOnlyBitSetObservableList(BitSet bitset) { + this.bitset = bitset; + } + + @Override public Integer get(int index) { + final int itemCount = getItemCount(); + if (index < 0 || index >= itemCount) return -1; + + if (index == (lastGetIndex + 1) && lastGetValue < itemCount) { + // we're iterating forward in order, short circuit for + // performance reasons (RT-39776) + lastGetIndex++; + lastGetValue = bitset.nextSetBit(lastGetValue + 1); + return lastGetValue; + } else if (index == (lastGetIndex - 1) && lastGetValue > 0) { + // we're iterating backward in order, short circuit for + // performance reasons (RT-39776) + lastGetIndex--; + lastGetValue = bitset.previousSetBit(lastGetValue - 1); + return lastGetValue; + } else { + for (lastGetIndex = 0, lastGetValue = bitset.nextSetBit(0); + lastGetValue >= 0 || lastGetIndex == index; + lastGetIndex++, lastGetValue = bitset.nextSetBit(lastGetValue + 1)) { + if (lastGetIndex == index) { + return lastGetValue; } } - - return -1; } - @Override public int size() { - return bitset.cardinality(); + return -1; + } + + @Override public int size() { + return bitset.cardinality(); + } + + @Override public boolean contains(Object o) { + if (o instanceof Number) { + Number n = (Number) o; + int index = n.intValue(); + + return index >= 0 && index < bitset.length() && + bitset.get(index); } - @Override public boolean contains(Object o) { - if (o instanceof Number) { - Number n = (Number) o; - int index = n.intValue(); + return false; + } - return index >= 0 && index < bitset.length() && - bitset.get(index); - } - - return false; - } - }; + public void refreshBitset() { +// System.out.println("REFRESH " + bitset); + } } } diff --git a/modules/controls/src/main/java/javafx/scene/control/TableView.java b/modules/controls/src/main/java/javafx/scene/control/TableView.java --- a/modules/controls/src/main/java/javafx/scene/control/TableView.java +++ b/modules/controls/src/main/java/javafx/scene/control/TableView.java @@ -3072,6 +3072,9 @@ fireChangeEvent = true; } + final ReadOnlyBitSetObservableList selectedIndicesSeq = (ReadOnlyBitSetObservableList)getSelectedIndices(); +// selectedIndicesSeq.refreshBitset(); + if (fireChangeEvent) { if (selectedItemChange != null) { selectedItems.callObservers(selectedItemChange); @@ -3093,8 +3096,7 @@ setSelectedItem(null); } - final ReadOnlyUnbackedObservableList selectedIndicesSeq = - (ReadOnlyUnbackedObservableList)getSelectedIndices(); +// ObservableList selectedIndicesCopy = createListFromBitSet(selectedIndices); if (! newlySelectedRows.isEmpty() && newlyUnselectedRows.isEmpty()) { // need to come up with ranges based on the actualSelectedRows, and diff --git a/modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java b/modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java --- a/modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java +++ b/modules/controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java @@ -1194,4 +1194,37 @@ sl.dispose(); } + + @Test public void test_jdk8138848_smallRange() { + model.select(1); + assertEquals(1, model.getSelectedIndex()); + assertEquals(1, model.getSelectedIndices().size()); + + // 'shift' selection downwards by just one + model.selectRange(1, 3); + assertEquals(2, model.getSelectedIndex()); + assertEquals(Arrays.asList(1,2), model.getSelectedIndices()); + + // now 'shift' selection to top + model.selectRange(2, -1); + assertEquals(0, model.getSelectedIndex()); + assertEquals(Arrays.asList(0,1,2), model.getSelectedIndices()); + } + + @Test public void test_jdk8138848_largeRange() { + model.select(2); + assertEquals(2, model.getSelectedIndex()); + assertEquals(1, model.getSelectedIndices().size()); + + // 'shift' select downwards + model.selectRange(2, 5); + assertEquals(4, model.getSelectedIndex()); + assertEquals(3, model.getSelectedIndices().size()); + + // now 'shift' select to top + model.selectRange(5, -1); + assertEquals(0, model.getSelectedIndex()); + assertEquals("Expected [0,1,2,3,4,5], but got: " + model.getSelectedIndices(), + 6, model.getSelectedIndices().size()); + } } diff --git a/modules/controls/src/test/java/test/javafx/scene/control/TableViewMouseInputTest.java b/modules/controls/src/test/java/test/javafx/scene/control/TableViewMouseInputTest.java --- a/modules/controls/src/test/java/test/javafx/scene/control/TableViewMouseInputTest.java +++ b/modules/controls/src/test/java/test/javafx/scene/control/TableViewMouseInputTest.java @@ -25,8 +25,10 @@ package test.javafx.scene.control; +import java.util.Arrays; import java.util.List; +import javafx.beans.InvalidationListener; import test.com.sun.javafx.scene.control.test.Person; import com.sun.javafx.tk.Toolkit; import javafx.collections.FXCollections; @@ -58,6 +60,7 @@ import test.com.sun.javafx.scene.control.infrastructure.MouseEventFirer; import test.com.sun.javafx.scene.control.infrastructure.StageLoader; import test.com.sun.javafx.scene.control.infrastructure.VirtualFlowTestUtils; +import test.javafx.scene.control.skin.VirtualFlowTest; import static org.junit.Assert.*; @@ -832,4 +835,40 @@ assertEquals("Expected Last Name column, but got " + cell.getTableColumn().getText(), lastNameCol, cell.getTableColumn()); } } + + @Test public void test_jdk_8138848() { + tableView.getItems().setAll("0", "1", "2", "3"); + tableView.getColumns().setAll(col0); + + VirtualFlowTestUtils.clickOnRow(tableView, 1); + assertEquals(1, sm.getSelectedIndex()); + + VirtualFlowTestUtils.clickOnRow(tableView, 2, KeyModifier.SHIFT); + assertEquals(2, sm.getSelectedIndex()); + assertEquals(2, sm.getSelectedIndices().size()); + assertTrue(sm.getSelectedIndices().containsAll(Arrays.asList(1,2))); + assertTrue(sm.getSelectedItems().containsAll(Arrays.asList("1","2"))); + + sm.getSelectedIndices().addListener((InvalidationListener) o -> { + if (sm.getSelectedIndices().contains(-1)) { + fail("Should never get -1 here!"); + } + }); + sm.getSelectedItems().addListener((InvalidationListener) o -> { + if (sm.getSelectedItems().contains(null)) { + fail("Should never get null here!"); + } + }); + + + VirtualFlowTestUtils.clickOnRow(tableView, 0, KeyModifier.SHIFT); +// System.out.println(sm.getSelectedItems()); + assertFalse(sm.getSelectedIndices().contains(-1)); + assertEquals(0, sm.getSelectedIndex()); + assertEquals(2, sm.getSelectedIndices().size()); + assertEquals(2, sm.getSelectedItems().size()); + assertTrue(sm.getSelectedIndices().containsAll(Arrays.asList(0,1))); + assertTrue(//"Expecting [\"0\", \"1\"], but got: " + sm.getSelectedItems(), + sm.getSelectedItems().containsAll(Arrays.asList("0","1"))); + } } diff --git a/modules/controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java b/modules/controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java --- a/modules/controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java +++ b/modules/controls/src/test/java/test/javafx/scene/control/TableViewSelectionModelImplTest.java @@ -732,4 +732,46 @@ } } } + + @Test public void test_jdk8138848_smallRange() { + model.setCellSelectionEnabled(false); + model.setSelectionMode(SelectionMode.MULTIPLE); + + model.select(1); + assertEquals(1, model.getSelectedIndex()); + assertEquals(1, model.getSelectedIndices().size()); + + // 'shift' selection downwards by just one + model.selectRange(1, col0, 2, col0); + assertEquals(2, model.getSelectedIndex()); + assertEquals(2, model.getSelectedIndices().size()); + assertTrue(model.getSelectedIndices().containsAll(Arrays.asList(1,2))); + + // now 'shift' selection to top + model.selectRange(2, col0, 0, col0); + assertEquals(0, model.getSelectedIndex()); + assertEquals(3, model.getSelectedIndices().size()); + assertTrue(model.getSelectedIndices().containsAll(Arrays.asList(0,1,2))); + } + + @Test public void test_jdk8138848_largeRange() { + model.setCellSelectionEnabled(false); + model.setSelectionMode(SelectionMode.MULTIPLE); + + model.select(2); + assertEquals(2, model.getSelectedIndex()); + assertEquals(1, model.getSelectedIndices().size()); + + // 'shift' select downwards + model.selectRange(2, col0, 5, col0); + assertEquals(5, model.getSelectedIndex()); + assertEquals(4, model.getSelectedIndices().size()); + assertTrue(model.getSelectedIndices().containsAll(Arrays.asList(2,3,4,5))); + + // now 'shift' select to top + model.selectRange(5, col0, 0, col0); + assertEquals(0, model.getSelectedIndex()); + assertEquals(6, model.getSelectedIndices().size()); + assertTrue(model.getSelectedIndices().containsAll(Arrays.asList(0,1,2,3,4,5))); + } }