Skip to content

Commit

Permalink
8286261: Selection of non-expanded non-leaf treeItem grows unexpected…
Browse files Browse the repository at this point in the history 
…ly when adding two-level descendants

Reviewed-by: aghaisas
  • Loading branch information
Jose Pereda committed May 22, 2022
1 parent 18b2366 commit 19a855e
Show file tree
Hide file tree
Showing 5 changed files with 179 additions and 8 deletions.
14 changes: 14 additions & 0 deletions modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,4 +232,18 @@ public static <S> int getIndexOfChildWithDescendant(TreeItem<S> parent, TreeItem
}
return -1;
}

public static <S> boolean isTreeItemIncludingAncestorsExpanded(TreeItem<S> item) {
if (item == null || !item.isExpanded()) {
return false;
}
TreeItem<S> ancestor = item.getParent();
while (ancestor != null) {
if (!ancestor.isExpanded()) {
return false;
}
ancestor = ancestor.getParent();
}
return true;
}
}
8 changes: 4 additions & 4 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2611,7 +2611,7 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
}
} else if (e.wasAdded()) {
// shuffle selection by the number of added items
shift += treeItem.isExpanded() ? addedSize : 0;
shift += ControlUtils.isTreeItemIncludingAncestorsExpanded(treeItem) ? addedSize : 0;

// RT-32963: We were taking the startRow from the TreeItem
// in which the children were added, rather than from the
Expand Down Expand Up @@ -2647,7 +2647,7 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {

// shuffle selection by the number of removed items
// only if removed items are before the current selection.
if (treeItem.isExpanded()) {
if (ControlUtils.isTreeItemIncludingAncestorsExpanded(treeItem)) {
int lastSelectedSiblingIndex = selectedItems.stream()
.map(item -> ControlUtils.getIndexOfChildWithDescendant(treeItem, item))
.max(Comparator.naturalOrder())
Expand Down Expand Up @@ -3491,7 +3491,7 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
// get the TreeItem the event occurred on - we only need to
// shift if the tree item is expanded
TreeItem<S> eventTreeItem = e.getTreeItem();
if (eventTreeItem.isExpanded()) {
if (ControlUtils.isTreeItemIncludingAncestorsExpanded(eventTreeItem)) {
for (int i = 0; i < e.getAddedChildren().size(); i++) {
// get the added item and determine the row it is in
TreeItem<S> item = e.getAddedChildren().get(i);
Expand All @@ -3513,7 +3513,7 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
}
}

if (e.getTreeItem().isExpanded()) {
if (ControlUtils.isTreeItemIncludingAncestorsExpanded(e.getTreeItem())) {
int focusedSiblingRow = ControlUtils.getIndexOfChildWithDescendant(e.getTreeItem(), getFocusedItem());
if (e.getFrom() <= focusedSiblingRow) {
// shuffle selection by the number of removed items
Expand Down
8 changes: 4 additions & 4 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1406,7 +1406,7 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
// no-op
} else if (e.wasAdded()) {
// shuffle selection by the number of added items
shift += treeItem.isExpanded() ? addedSize : 0;
shift += ControlUtils.isTreeItemIncludingAncestorsExpanded(treeItem) ? addedSize : 0;

// RT-32963: We were taking the startRow from the TreeItem
// in which the children were added, rather than from the
Expand Down Expand Up @@ -1434,7 +1434,7 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {

// shuffle selection by the number of removed items
// only if removed items are before the current selection.
if (treeItem.isExpanded()) {
if (ControlUtils.isTreeItemIncludingAncestorsExpanded(treeItem)) {
int lastSelectedSiblingIndex = selectedItems.stream()
.map(item -> ControlUtils.getIndexOfChildWithDescendant(treeItem, item))
.max(Comparator.naturalOrder())
Expand Down Expand Up @@ -1683,7 +1683,7 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
// get the TreeItem the event occurred on - we only need to
// shift if the tree item is expanded
TreeItem<T> eventTreeItem = e.getTreeItem();
if (eventTreeItem.isExpanded()) {
if (ControlUtils.isTreeItemIncludingAncestorsExpanded(eventTreeItem)) {
for (int i = 0; i < e.getAddedChildren().size(); i++) {
// get the added item and determine the row it is in
TreeItem<T> item = e.getAddedChildren().get(i);
Expand All @@ -1705,7 +1705,7 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
}
}

if (e.getTreeItem().isExpanded()) {
if (ControlUtils.isTreeItemIncludingAncestorsExpanded(e.getTreeItem())) {
int focusedSiblingRow = ControlUtils.getIndexOfChildWithDescendant(e.getTreeItem(), getFocusedItem());
if (e.getFrom() <= focusedSiblingRow) {
// shuffle selection by the number of removed items
Expand Down
83 changes: 83 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6831,4 +6831,87 @@ public void test_ChangeToStringMouseMultipleSelectionCellMode() {
Thread.currentThread().setUncaughtExceptionHandler(exceptionHandler);
}

// JDK-8286261
@Test
public void testAddTreeItemToCollapsedAncestorKeepsSelectedItem() {
TreeItem<String> rootNode = new TreeItem<>("Root");
rootNode.setExpanded(true);
TreeItem<String> level1 = new TreeItem<>("Node 0");
level1.setExpanded(false);
TreeItem<String> level2 = new TreeItem<>("Node 1");
level2.getChildren().add(new TreeItem<>("Node 2"));
level2.setExpanded(true);

rootNode.getChildren().add(level1);
rootNode.getChildren().add(new TreeItem<>("Node 3"));

level1.getChildren().add(level2);

TreeTableColumn<String, String> column = new TreeTableColumn<>("Nodes");
column.setCellValueFactory(p -> new ReadOnlyStringWrapper(p.getValue().getValue()));
column.setPrefWidth(200);

TreeTableView<String> table = new TreeTableView<>(rootNode);
table.setShowRoot(false);
table.getColumns().add(column);
table.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);

table.getSelectionModel().select(level1);
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());

// add new node at level 3, that has a collapsed ancestor
level2.getChildren().add(new TreeItem<>("Node 4"));

// selection and focus remain at level1
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());
}

// JDK-8286261
@Test
public void testRemoveTreeItemFromCollapsedAncestorKeepsSelectedItem() {
TreeItem<String> rootNode = new TreeItem<>("Root");
rootNode.setExpanded(true);
TreeItem<String> level1 = new TreeItem<>("Node 0");
level1.setExpanded(false);
TreeItem<String> level2 = new TreeItem<>("Node 1");
level2.getChildren().add(new TreeItem<>("Node 2"));
level2.getChildren().add(new TreeItem<>("Node 3"));

level2.setExpanded(true);

rootNode.getChildren().add(level1);
rootNode.getChildren().add(new TreeItem<>("Node 4"));

level1.getChildren().add(level2);

TreeTableColumn<String, String> column = new TreeTableColumn<>("Nodes");
column.setCellValueFactory(p -> new ReadOnlyStringWrapper(p.getValue().getValue()));
column.setPrefWidth(200);

TreeTableView<String> table = new TreeTableView<>(rootNode);
table.setShowRoot(false);
table.getColumns().add(column);
table.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);

table.getSelectionModel().select(level1);
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());

// remove Node 2 at level 3, that has a collapsed ancestor
level2.getChildren().remove(0);

// selection and focus remain at level1
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());
}
}
74 changes: 74 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3833,6 +3833,80 @@ public void testRemoveTreeItemChangesSelectedItem() {
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
}

// JDK-8286261
@Test
public void testAddTreeItemToCollapsedAncestorKeepsSelectedItem() {
TreeItem<String> rootNode = new TreeItem<>("Root");
rootNode.setExpanded(true);
TreeItem<String> level1 = new TreeItem<>("Node 0");
level1.setExpanded(false);
TreeItem<String> level2 = new TreeItem<>("Node 1");
level2.getChildren().add(new TreeItem<>("Node 2"));
level2.setExpanded(true);

rootNode.getChildren().add(level1);
rootNode.getChildren().add(new TreeItem<>("Node 3"));

level1.getChildren().add(level2);

TreeView<String> table = new TreeView<>(rootNode);
table.setShowRoot(false);
table.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);

table.getSelectionModel().select(level1);
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());

// add new node at level 3, that has a collapsed ancestor
level2.getChildren().add(new TreeItem<>("Node 4"));

// selection and focus remain at level1
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());
}

// JDK-8286261
@Test
public void testRemoveTreeItemFromCollapsedAncestorKeepsSelectedItem() {
TreeItem<String> rootNode = new TreeItem<>("Root");
rootNode.setExpanded(true);
TreeItem<String> level1 = new TreeItem<>("Node 0");
level1.setExpanded(false);
TreeItem<String> level2 = new TreeItem<>("Node 1");
level2.getChildren().add(new TreeItem<>("Node 2"));
level2.getChildren().add(new TreeItem<>("Node 3"));

level2.setExpanded(true);

rootNode.getChildren().add(level1);
rootNode.getChildren().add(new TreeItem<>("Node 4"));

level1.getChildren().add(level2);

TreeView<String> table = new TreeView<>(rootNode);
table.setShowRoot(false);
table.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);

table.getSelectionModel().select(level1);
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());

// remove Node 2 at level 3, that has a collapsed ancestor
level2.getChildren().remove(0);

// selection and focus remain at level1
assertEquals(0, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 0", table.getSelectionModel().getSelectedItem().getValue());
assertEquals(0, table.getFocusModel().getFocusedIndex());
assertEquals("Node 0", table.getFocusModel().getFocusedItem().getValue());
}

public static class MisbehavingOnCancelTreeCell<S> extends TreeCell<S> {

@Override
Expand Down

1 comment on commit 19a855e

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Issues

Please sign in to comment.