Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8193442: Removing TreeItem from a TreeTableView sometime changes selectedItem #753

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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;
}
}
@@ -2630,9 +2630,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'
@@ -2648,6 +2645,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;
@@ -3503,9 +3513,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());
@@ -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;
@@ -1415,9 +1416,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'
@@ -1434,6 +1432,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;
@@ -1694,9 +1705,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());
@@ -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;
@@ -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);
@@ -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() {
@@ -6714,6 +6712,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();
@@ -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);
@@ -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() {
@@ -3778,6 +3775,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