Skip to content

Commit

Permalink
8193442: Removing TreeItem from a TreeTableView sometime changes sele…
Browse files Browse the repository at this point in the history 
…ctedItem

8187596: TreeView selection incorrectly changes after deleting an unselected row

Backport-of: 3bb2db1
  • Loading branch information
Johan Vos committed Jun 29, 2022
1 parent 7d33552 commit 97a3f7f
Show file tree
Hide file tree
Showing 5 changed files with 181 additions and 18 deletions.
15 changes: 15 additions & 0 deletions modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
View file
Expand Up @@ -217,4 +217,19 @@ public static <S> void updateSelectedIndices(MultipleSelectionModelBase<S> sm, b

sm.selectedIndices._endChange();
}

public static <S> int getIndexOfChildWithDescendant(TreeItem<S> parent, TreeItem<S> item) {
if (item == null || parent == null) {
return -1;
}
TreeItem<S> child = item, ancestor = item.getParent();
while (ancestor != null) {
if (ancestor == parent) {
return parent.getChildren().indexOf(child);
}
child = ancestor;
ancestor = child.getParent();
}
return -1;
}
}
25 changes: 19 additions & 6 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
View file
Expand Up @@ -2629,9 +2629,6 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
}
}
} else if (e.wasRemoved()) {
// shuffle selection by the number of removed items
shift += treeItem.isExpanded() ? -removedSize : 0;

// the start row is incorrect - it is _not_ the index of the
// TreeItem in which the children were removed from (which is
// what it currently represents). We need to take the 'from'
Expand All @@ -2647,6 +2644,19 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
final TreeItem<S> selectedItem = getSelectedItem();
final List<? extends TreeItem<S>> removedChildren = e.getChange().getRemoved();

// shuffle selection by the number of removed items
// only if removed items are before the current selection.
if (treeItem.isExpanded()) {
int lastSelectedSiblingIndex = selectedItems.stream()
.map(item -> ControlUtils.getIndexOfChildWithDescendant(treeItem, item))
.max(Comparator.naturalOrder())
.orElse(-1);
// shift only if the last selected sibling index is after the first removed child
if (e.getFrom() <= lastSelectedSiblingIndex || lastSelectedSiblingIndex == -1) {
shift -= removedSize;
}
}

for (int i = 0; i < selectedIndices.size() && !selectedItems.isEmpty(); i++) {
int index = selectedIndices.get(i);
if (index > selectedItems.size()) break;
Expand Down Expand Up @@ -3502,9 +3512,12 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
}
}

if (row <= getFocusedIndex()) {
// shuffle selection by the number of removed items
shift += e.getTreeItem().isExpanded() ? -e.getRemovedSize() : 0;
if (e.getTreeItem().isExpanded()) {
int focusedSiblingRow = ControlUtils.getIndexOfChildWithDescendant(e.getTreeItem(), getFocusedItem());
if (e.getFrom() <= focusedSiblingRow) {
// shuffle selection by the number of removed items
shift -= e.getRemovedSize();
}
}
}
} while (e.getChange() != null && e.getChange().next());
Expand Down
26 changes: 20 additions & 6 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeView.java
View file
Expand Up @@ -66,6 +66,7 @@
import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -1407,9 +1408,6 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
// subsequently commented out due to RT-33894.
startRow = treeView.getRow(e.getChange().getAddedSubList().get(0));
} else if (e.wasRemoved()) {
// shuffle selection by the number of removed items
shift += treeItem.isExpanded() ? -removedSize : 0;

// the start row is incorrect - it is _not_ the index of the
// TreeItem in which the children were removed from (which is
// what it currently represents). We need to take the 'from'
Expand All @@ -1426,6 +1424,19 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
final TreeItem<T> selectedItem = getSelectedItem();
final List<? extends TreeItem<T>> removedChildren = e.getChange().getRemoved();

// shuffle selection by the number of removed items
// only if removed items are before the current selection.
if (treeItem.isExpanded()) {
int lastSelectedSiblingIndex = selectedItems.stream()
.map(item -> ControlUtils.getIndexOfChildWithDescendant(treeItem, item))
.max(Comparator.naturalOrder())
.orElse(-1);
// shift only if the last selected sibling index is after the first removed child
if (e.getFrom() <= lastSelectedSiblingIndex || lastSelectedSiblingIndex == -1) {
shift -= removedSize;
}
}

for (int i = 0; i < selectedIndices1.size() && !selectedItems.isEmpty(); i++) {
int index = selectedIndices1.get(i);
if (index > selectedItems.size()) break;
Expand Down Expand Up @@ -1686,9 +1697,12 @@ private void updateTreeEventListener(TreeItem<T> oldRoot, TreeItem<T> newRoot) {
}
}

if (row <= getFocusedIndex()) {
// shuffle selection by the number of removed items
shift += e.getTreeItem().isExpanded() ? -e.getRemovedSize() : 0;
if (e.getTreeItem().isExpanded()) {
int focusedSiblingRow = ControlUtils.getIndexOfChildWithDescendant(e.getTreeItem(), getFocusedItem());
if (e.getFrom() <= focusedSiblingRow) {
// shuffle selection by the number of removed items
shift -= e.getRemovedSize();
}
}
}
} while (e.getChange() != null && e.getChange().next());
Expand Down
72 changes: 69 additions & 3 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
View file
Expand Up @@ -44,6 +44,7 @@
import com.sun.javafx.scene.control.behavior.TreeTableCellBehavior;
import javafx.beans.property.ReadOnlyIntegerWrapper;
import javafx.collections.transformation.FilteredList;
import javafx.scene.control.SelectionModel;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableView;
import test.javafx.collections.MockListObserver;
Expand Down Expand Up @@ -1568,7 +1569,6 @@ private void addChildren(TreeItem parent, String name) {
assertEquals(mikeGraham, treeTableView.getFocusModel().getFocusedItem());
}

@Ignore("Bug hasn't been fixed yet")
@Test public void test_rt28114() {
myCompanyRootNode.setExpanded(true);
treeTableView.setRoot(myCompanyRootNode);
Expand All @@ -1584,8 +1584,6 @@ private void addChildren(TreeItem parent, String name) {
itSupport.getChildren().remove(mikeGraham);
assertEquals(itSupport, treeTableView.getFocusModel().getFocusedItem());
assertEquals(itSupport, treeTableView.getSelectionModel().getSelectedItem());
assertTrue(itSupport.isLeaf());
assertTrue(!itSupport.isExpanded());
}

@Test public void test_rt27820_1() {
Expand Down Expand Up @@ -6715,6 +6713,74 @@ public void test_clearAndSelectChangeMultipleSelectionCellMode() {
assertEquals(1, sm.getSelectedItems().size());
}

// JDK-8187596
@Test
public void testRemoveTreeItemShiftSelection() {
TreeItem<String> a, b, a1, a2, a3;
TreeItem<String> root = new TreeItem<>("root");
root.getChildren().addAll(
a = new TreeItem<>("a"),
b = new TreeItem<>("b")
);
root.setExpanded(true);

a.getChildren().addAll(
a1 = new TreeItem<>("a1"),
a2 = new TreeItem<>("a2"),
a3 = new TreeItem<>("a3")
);
a.setExpanded(true);

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

stringTreeTableView.setShowRoot(false);
SelectionModel sm = stringTreeTableView.getSelectionModel();

sm.clearAndSelect(3); //select a3
assertEquals(a3, sm.getSelectedItem()); //verify
root.getChildren().remove(b); //remove b
//a3 should remain selected
assertEquals(3, sm.getSelectedIndex());
assertEquals(a3, sm.getSelectedItem());
}

// JDK-8193442
@Test
public void testRemoveTreeItemChangesSelectedItem() {
TreeItem<String> rootNode = new TreeItem<>("Root");
rootNode.setExpanded(true);
for (int i = 0; i < 3; i++) {
rootNode.getChildren().add(new TreeItem<>("Node " + i));
}
for (int i = 0; i < 2; i++) {
TreeItem<String> node = rootNode.getChildren().get(i);
node.setExpanded(true);
for (int j = 0; j < 2; j++) {
node.getChildren().add(new TreeItem<>("Sub Node " + i + "-" + j));
}
}

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.getColumns().add(column);

int selectIndex = 4; // select "Node 1"
int removeIndex = 2; // remove "Node 2"
table.getSelectionModel().select(selectIndex);
assertEquals(4, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
table.getRoot().getChildren().remove(removeIndex);
assertEquals(4, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
}

@Test
public void test_ChangeToStringMouseMultipleSelectionCellMode() {
final Thread.UncaughtExceptionHandler exceptionHandler = Thread.currentThread().getUncaughtExceptionHandler();
Expand Down
61 changes: 58 additions & 3 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeViewTest.java
View file
Expand Up @@ -630,7 +630,6 @@ private void addChildren(TreeItem parent, String name) {
assertEquals(mikeGraham, treeView.getFocusModel().getFocusedItem());
}

@Ignore("Bug hasn't been fixed yet")
@Test public void test_rt28114() {
myCompanyRootNode.setExpanded(true);
treeView.setRoot(myCompanyRootNode);
Expand All @@ -646,8 +645,6 @@ private void addChildren(TreeItem parent, String name) {
itSupport.getChildren().remove(mikeGraham);
assertEquals(itSupport, treeView.getFocusModel().getFocusedItem());
assertEquals(itSupport, treeView.getSelectionModel().getSelectedItem());
assertTrue(itSupport.isLeaf());
assertTrue(!itSupport.isExpanded());
}

@Test public void test_rt27820_1() {
Expand Down Expand Up @@ -3777,6 +3774,64 @@ public void testMisbehavingCancelEditTerminatesEdit() {
}
}

// JDK-8187596
@Test
public void testRemoveTreeItemShiftSelection() {
TreeItem<String> a, b, a1, a2, a3;
TreeItem<String> root = new TreeItem<>("root");
root.getChildren().addAll(
a = new TreeItem<>("a"),
b = new TreeItem<>("b")
);
root.setExpanded(true);

a.getChildren().addAll(
a1 = new TreeItem<>("a1"),
a2 = new TreeItem<>("a2"),
a3 = new TreeItem<>("a3")
);
a.setExpanded(true);

TreeView<String> stringTreeView = new TreeView<>(root);
stringTreeView.setShowRoot(false);
SelectionModel sm = stringTreeView.getSelectionModel();

sm.clearAndSelect(3); //select a3
assertEquals(a3, sm.getSelectedItem()); //verify
root.getChildren().remove(b); //remove b
//a3 should remain selected
assertEquals(3, sm.getSelectedIndex());
assertEquals(a3, sm.getSelectedItem());
}

// JDK-8193442
@Test
public void testRemoveTreeItemChangesSelectedItem() {
TreeItem<String> rootNode = new TreeItem<>("Root");
rootNode.setExpanded(true);
for (int i = 0; i < 3; i++) {
rootNode.getChildren().add(new TreeItem<>("Node " + i));
}
for (int i = 0; i < 2; i++) {
TreeItem<String> node = rootNode.getChildren().get(i);
node.setExpanded(true);
for (int j = 0; j < 2; j++) {
node.getChildren().add(new TreeItem<>("Sub Node " + i + "-" + j));
}
}

TreeView<String> table = new TreeView<>(rootNode);

int selectIndex = 4; // select "Node 1"
int removeIndex = 2; // remove "Node 2"
table.getSelectionModel().select(selectIndex);
assertEquals(4, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
table.getRoot().getChildren().remove(removeIndex);
assertEquals(4, table.getSelectionModel().getSelectedIndex());
assertEquals("Node 1", table.getSelectionModel().getSelectedItem().getValue());
}

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

@Override
Expand Down

1 comment on commit 97a3f7f

@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.