Skip to content

Commit 97a3f7f

Browse files
author
Johan Vos
committed
8193442: Removing TreeItem from a TreeTableView sometime changes selectedItem
8187596: TreeView selection incorrectly changes after deleting an unselected row Backport-of: 3bb2db1
1 parent 7d33552 commit 97a3f7f

File tree

5 files changed

+181
-18
lines changed

5 files changed

+181
-18
lines changed

‎modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java

+15
Original file line numberDiff line numberDiff line change
@@ -217,4 +217,19 @@ public static <S> void updateSelectedIndices(MultipleSelectionModelBase<S> sm, b
217217

218218
sm.selectedIndices._endChange();
219219
}
220+
221+
public static <S> int getIndexOfChildWithDescendant(TreeItem<S> parent, TreeItem<S> item) {
222+
if (item == null || parent == null) {
223+
return -1;
224+
}
225+
TreeItem<S> child = item, ancestor = item.getParent();
226+
while (ancestor != null) {
227+
if (ancestor == parent) {
228+
return parent.getChildren().indexOf(child);
229+
}
230+
child = ancestor;
231+
ancestor = child.getParent();
232+
}
233+
return -1;
234+
}
220235
}

‎modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java

+19-6
Original file line numberDiff line numberDiff line change
@@ -2629,9 +2629,6 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
26292629
}
26302630
}
26312631
} else if (e.wasRemoved()) {
2632-
// shuffle selection by the number of removed items
2633-
shift += treeItem.isExpanded() ? -removedSize : 0;
2634-
26352632
// the start row is incorrect - it is _not_ the index of the
26362633
// TreeItem in which the children were removed from (which is
26372634
// what it currently represents). We need to take the 'from'
@@ -2647,6 +2644,19 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
26472644
final TreeItem<S> selectedItem = getSelectedItem();
26482645
final List<? extends TreeItem<S>> removedChildren = e.getChange().getRemoved();
26492646

2647+
// shuffle selection by the number of removed items
2648+
// only if removed items are before the current selection.
2649+
if (treeItem.isExpanded()) {
2650+
int lastSelectedSiblingIndex = selectedItems.stream()
2651+
.map(item -> ControlUtils.getIndexOfChildWithDescendant(treeItem, item))
2652+
.max(Comparator.naturalOrder())
2653+
.orElse(-1);
2654+
// shift only if the last selected sibling index is after the first removed child
2655+
if (e.getFrom() <= lastSelectedSiblingIndex || lastSelectedSiblingIndex == -1) {
2656+
shift -= removedSize;
2657+
}
2658+
}
2659+
26502660
for (int i = 0; i < selectedIndices.size() && !selectedItems.isEmpty(); i++) {
26512661
int index = selectedIndices.get(i);
26522662
if (index > selectedItems.size()) break;
@@ -3502,9 +3512,12 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
35023512
}
35033513
}
35043514

3505-
if (row <= getFocusedIndex()) {
3506-
// shuffle selection by the number of removed items
3507-
shift += e.getTreeItem().isExpanded() ? -e.getRemovedSize() : 0;
3515+
if (e.getTreeItem().isExpanded()) {
3516+
int focusedSiblingRow = ControlUtils.getIndexOfChildWithDescendant(e.getTreeItem(), getFocusedItem());
3517+
if (e.getFrom() <= focusedSiblingRow) {
3518+
// shuffle selection by the number of removed items
3519+
shift -= e.getRemovedSize();
3520+
}
35083521
}
35093522
}
35103523
} while (e.getChange() != null && e.getChange().next());

‎modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java

+20-6
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
import java.lang.ref.WeakReference;
6767
import java.util.ArrayList;
6868
import java.util.Collections;
69+
import java.util.Comparator;
6970
import java.util.HashMap;
7071
import java.util.List;
7172
import java.util.Map;
@@ -1407,9 +1408,6 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
14071408
// subsequently commented out due to RT-33894.
14081409
startRow = treeView.getRow(e.getChange().getAddedSubList().get(0));
14091410
} else if (e.wasRemoved()) {
1410-
// shuffle selection by the number of removed items
1411-
shift += treeItem.isExpanded() ? -removedSize : 0;
1412-
14131411
// the start row is incorrect - it is _not_ the index of the
14141412
// TreeItem in which the children were removed from (which is
14151413
// what it currently represents). We need to take the 'from'
@@ -1426,6 +1424,19 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
14261424
final TreeItem<T> selectedItem = getSelectedItem();
14271425
final List<? extends TreeItem<T>> removedChildren = e.getChange().getRemoved();
14281426

1427+
// shuffle selection by the number of removed items
1428+
// only if removed items are before the current selection.
1429+
if (treeItem.isExpanded()) {
1430+
int lastSelectedSiblingIndex = selectedItems.stream()
1431+
.map(item -> ControlUtils.getIndexOfChildWithDescendant(treeItem, item))
1432+
.max(Comparator.naturalOrder())
1433+
.orElse(-1);
1434+
// shift only if the last selected sibling index is after the first removed child
1435+
if (e.getFrom() <= lastSelectedSiblingIndex || lastSelectedSiblingIndex == -1) {
1436+
shift -= removedSize;
1437+
}
1438+
}
1439+
14291440
for (int i = 0; i < selectedIndices1.size() && !selectedItems.isEmpty(); i++) {
14301441
int index = selectedIndices1.get(i);
14311442
if (index > selectedItems.size()) break;
@@ -1686,9 +1697,12 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
16861697
}
16871698
}
16881699

1689-
if (row <= getFocusedIndex()) {
1690-
// shuffle selection by the number of removed items
1691-
shift += e.getTreeItem().isExpanded() ? -e.getRemovedSize() : 0;
1700+
if (e.getTreeItem().isExpanded()) {
1701+
int focusedSiblingRow = ControlUtils.getIndexOfChildWithDescendant(e.getTreeItem(), getFocusedItem());
1702+
if (e.getFrom() <= focusedSiblingRow) {
1703+
// shuffle selection by the number of removed items
1704+
shift -= e.getRemovedSize();
1705+
}
16921706
}
16931707
}
16941708
} while (e.getChange() != null && e.getChange().next());

‎modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java

+69-3
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import com.sun.javafx.scene.control.behavior.TreeTableCellBehavior;
4545
import javafx.beans.property.ReadOnlyIntegerWrapper;
4646
import javafx.collections.transformation.FilteredList;
47+
import javafx.scene.control.SelectionModel;
4748
import javafx.scene.control.TableColumn;
4849
import javafx.scene.control.TableView;
4950
import test.javafx.collections.MockListObserver;
@@ -1568,7 +1569,6 @@ private void addChildren(TreeItem parent, String name) {
15681569
assertEquals(mikeGraham, treeTableView.getFocusModel().getFocusedItem());
15691570
}
15701571

1571-
@Ignore("Bug hasn't been fixed yet")
15721572
@Test public void test_rt28114() {
15731573
myCompanyRootNode.setExpanded(true);
15741574
treeTableView.setRoot(myCompanyRootNode);
@@ -1584,8 +1584,6 @@ private void addChildren(TreeItem parent, String name) {
15841584
itSupport.getChildren().remove(mikeGraham);
15851585
assertEquals(itSupport, treeTableView.getFocusModel().getFocusedItem());
15861586
assertEquals(itSupport, treeTableView.getSelectionModel().getSelectedItem());
1587-
assertTrue(itSupport.isLeaf());
1588-
assertTrue(!itSupport.isExpanded());
15891587
}
15901588

15911589
@Test public void test_rt27820_1() {
@@ -6715,6 +6713,74 @@ public void test_clearAndSelectChangeMultipleSelectionCellMode() {
67156713
assertEquals(1, sm.getSelectedItems().size());
67166714
}
67176715

6716+
// JDK-8187596
6717+
@Test
6718+
public void testRemoveTreeItemShiftSelection() {
6719+
TreeItem<String> a, b, a1, a2, a3;
6720+
TreeItem<String> root = new TreeItem<>("root");
6721+
root.getChildren().addAll(
6722+
a = new TreeItem<>("a"),
6723+
b = new TreeItem<>("b")
6724+
);
6725+
root.setExpanded(true);
6726+
6727+
a.getChildren().addAll(
6728+
a1 = new TreeItem<>("a1"),
6729+
a2 = new TreeItem<>("a2"),
6730+
a3 = new TreeItem<>("a3")
6731+
);
6732+
a.setExpanded(true);
6733+
6734+
TreeTableView<String> stringTreeTableView = new TreeTableView<>(root);
6735+
TreeTableColumn<String, String> column = new TreeTableColumn<>("Nodes");
6736+
column.setCellValueFactory(p -> new ReadOnlyStringWrapper(p.getValue().getValue()));
6737+
column.setPrefWidth(200);
6738+
stringTreeTableView.getColumns().add(column);
6739+
6740+
stringTreeTableView.setShowRoot(false);
6741+
SelectionModel sm = stringTreeTableView.getSelectionModel();
6742+
6743+
sm.clearAndSelect(3); //select a3
6744+
assertEquals(a3, sm.getSelectedItem()); //verify
6745+
root.getChildren().remove(b); //remove b
6746+
//a3 should remain selected
6747+
assertEquals(3, sm.getSelectedIndex());
6748+
assertEquals(a3, sm.getSelectedItem());
6749+
}
6750+
6751+
// JDK-8193442
6752+
@Test
6753+
public void testRemoveTreeItemChangesSelectedItem() {
6754+
TreeItem<String> rootNode = new TreeItem<>("Root");
6755+
rootNode.setExpanded(true);
6756+
for (int i = 0; i < 3; i++) {
6757+
rootNode.getChildren().add(new TreeItem<>("Node " + i));
6758+
}
6759+
for (int i = 0; i < 2; i++) {
6760+
TreeItem<String> node = rootNode.getChildren().get(i);
6761+
node.setExpanded(true);
6762+
for (int j = 0; j < 2; j++) {
6763+
node.getChildren().add(new TreeItem<>("Sub Node " + i + "-" + j));
6764+
}
6765+
}
6766+
6767+
TreeTableColumn<String, String> column = new TreeTableColumn<>("Nodes");
6768+
column.setCellValueFactory(p -> new ReadOnlyStringWrapper(p.getValue().getValue()));
6769+
column.setPrefWidth(200);
6770+
6771+
TreeTableView<String> table = new TreeTableView<>(rootNode);
6772+
table.getColumns().add(column);
6773+
6774+
int selectIndex = 4; // select "Node 1"
6775+
int removeIndex = 2; // remove "Node 2"
6776+
table.getSelectionModel().select(selectIndex);
6777+
assertEquals(4, table.getSelectionModel().getSelectedIndex());
6778+
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
6779+
table.getRoot().getChildren().remove(removeIndex);
6780+
assertEquals(4, table.getSelectionModel().getSelectedIndex());
6781+
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
6782+
}
6783+
67186784
@Test
67196785
public void test_ChangeToStringMouseMultipleSelectionCellMode() {
67206786
final Thread.UncaughtExceptionHandler exceptionHandler = Thread.currentThread().getUncaughtExceptionHandler();

‎modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java

+58-3
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,6 @@ private void addChildren(TreeItem parent, String name) {
630630
assertEquals(mikeGraham, treeView.getFocusModel().getFocusedItem());
631631
}
632632

633-
@Ignore("Bug hasn't been fixed yet")
634633
@Test public void test_rt28114() {
635634
myCompanyRootNode.setExpanded(true);
636635
treeView.setRoot(myCompanyRootNode);
@@ -646,8 +645,6 @@ private void addChildren(TreeItem parent, String name) {
646645
itSupport.getChildren().remove(mikeGraham);
647646
assertEquals(itSupport, treeView.getFocusModel().getFocusedItem());
648647
assertEquals(itSupport, treeView.getSelectionModel().getSelectedItem());
649-
assertTrue(itSupport.isLeaf());
650-
assertTrue(!itSupport.isExpanded());
651648
}
652649

653650
@Test public void test_rt27820_1() {
@@ -3777,6 +3774,64 @@ public void testMisbehavingCancelEditTerminatesEdit() {
37773774
}
37783775
}
37793776

3777+
// JDK-8187596
3778+
@Test
3779+
public void testRemoveTreeItemShiftSelection() {
3780+
TreeItem<String> a, b, a1, a2, a3;
3781+
TreeItem<String> root = new TreeItem<>("root");
3782+
root.getChildren().addAll(
3783+
a = new TreeItem<>("a"),
3784+
b = new TreeItem<>("b")
3785+
);
3786+
root.setExpanded(true);
3787+
3788+
a.getChildren().addAll(
3789+
a1 = new TreeItem<>("a1"),
3790+
a2 = new TreeItem<>("a2"),
3791+
a3 = new TreeItem<>("a3")
3792+
);
3793+
a.setExpanded(true);
3794+
3795+
TreeView<String> stringTreeView = new TreeView<>(root);
3796+
stringTreeView.setShowRoot(false);
3797+
SelectionModel sm = stringTreeView.getSelectionModel();
3798+
3799+
sm.clearAndSelect(3); //select a3
3800+
assertEquals(a3, sm.getSelectedItem()); //verify
3801+
root.getChildren().remove(b); //remove b
3802+
//a3 should remain selected
3803+
assertEquals(3, sm.getSelectedIndex());
3804+
assertEquals(a3, sm.getSelectedItem());
3805+
}
3806+
3807+
// JDK-8193442
3808+
@Test
3809+
public void testRemoveTreeItemChangesSelectedItem() {
3810+
TreeItem<String> rootNode = new TreeItem<>("Root");
3811+
rootNode.setExpanded(true);
3812+
for (int i = 0; i < 3; i++) {
3813+
rootNode.getChildren().add(new TreeItem<>("Node " + i));
3814+
}
3815+
for (int i = 0; i < 2; i++) {
3816+
TreeItem<String> node = rootNode.getChildren().get(i);
3817+
node.setExpanded(true);
3818+
for (int j = 0; j < 2; j++) {
3819+
node.getChildren().add(new TreeItem<>("Sub Node " + i + "-" + j));
3820+
}
3821+
}
3822+
3823+
TreeView<String> table = new TreeView<>(rootNode);
3824+
3825+
int selectIndex = 4; // select "Node 1"
3826+
int removeIndex = 2; // remove "Node 2"
3827+
table.getSelectionModel().select(selectIndex);
3828+
assertEquals(4, table.getSelectionModel().getSelectedIndex());
3829+
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
3830+
table.getRoot().getChildren().remove(removeIndex);
3831+
assertEquals(4, table.getSelectionModel().getSelectedIndex());
3832+
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
3833+
}
3834+
37803835
public static class MisbehavingOnCancelTreeCell<S> extends TreeCell<S> {
37813836

37823837
@Override

0 commit comments

Comments
 (0)