Skip to content

Commit

Permalink
8344067: TableCell indices may not match the TableRow index
Browse files Browse the repository at this point in the history 
Reviewed-by: angorya
  • Loading branch information
Marius Hanl committed Nov 14, 2024
1 parent 286c3a8 commit b0e763c
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 9 deletions.
10 changes: 1 addition & 9 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,15 +165,7 @@ public TableRowSkinBase(C control) {
// use invalidation listener here to update even when item equality is true
// (e.g. see RT-22463)
registerInvalidationListener(control.itemProperty(), o -> requestCellUpdate());
registerChangeListener(control.indexProperty(), e -> {
// Fix for RT-36661, where empty table cells were showing content, as they
// had incorrect table cell indices (but the table row index was correct).
// Note that we only do the update on empty cells to avoid the issue
// noted below in requestCellUpdate().
if (getSkinnable().isEmpty()) {
requestCellUpdate();
}
});
registerChangeListener(control.indexProperty(), e -> requestCellUpdate());
}


Expand Down
28 changes: 28 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewRowTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import javafx.scene.control.SelectionMode;
Expand Down Expand Up @@ -205,4 +207,30 @@ protected boolean isItemChanged(String oldItem, String newItem) {

assertTrue(isItemChangedCalled.get());
}

@Test
void testUpdateRowIndexManually() {
TableView<String> table = ControlUtils.createTableView();

TableRow<String> row = new TableRow<>();
row.updateTableView(table);

stageLoader = new StageLoader(row);

row.updateIndex(0);

List<TableCell<String, String>> cells = row.getChildrenUnmodifiable().stream()
.filter(TableCell.class::isInstance).map(e -> (TableCell<String, String>) e).toList();
for (TableCell<String, String> cell : cells) {
assertEquals(0, cell.getIndex());
}

row.updateIndex(1);

cells = row.getChildrenUnmodifiable().stream()
.filter(TableCell.class::isInstance).map(e -> (TableCell<String, String>) e).toList();
for (TableCell<String, String> cell : cells) {
assertEquals(1, cell.getIndex());
}
}
}
50 changes: 50 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6329,4 +6329,54 @@ public void startEdit() {

assertEquals(1, startEditCounter.get());
}

@Test
void testReSetItemsWithSameItemShouldUpdateCellIndices() {
table.setFixedCellSize(24);

final TableColumn<String, String> c = new TableColumn<>("C");
c.setCellValueFactory(value -> new SimpleStringProperty(value.getValue()));
table.getColumns().add(c);

for (int i = 0; i < 60; i++) {
table.getItems().add(String.valueOf(i));
}
String lastItem = "UniqueLastItem";
table.getItems().add(lastItem);

stageLoader = new StageLoader(table);
stageLoader.getStage().setWidth(300);
stageLoader.getStage().setHeight(300);

// Scroll to the bottom.
VirtualScrollBar scrollBar = VirtualFlowTestUtils.getVirtualFlowVerticalScrollbar(table);
scrollBar.setValue(1);

Toolkit.getToolkit().firePulse();

IndexedCell<String> row = VirtualFlowTestUtils.getCell(table, 60);
assertEquals(lastItem, row.getItem());

List<TableCell<String, String>> cells = row.getChildrenUnmodifiable().stream()
.filter(TableCell.class::isInstance).map(e -> (TableCell<String, String>) e).toList();

for (TableCell<String, String> cell : cells) {
assertEquals(60, cell.getIndex());
}

// Re-set the items, but reuse one item from the previous items list.
table.setItems(FXCollections.observableArrayList(lastItem));

Toolkit.getToolkit().firePulse();

row = VirtualFlowTestUtils.getCell(table, 0);
assertEquals(lastItem, row.getItem());

cells = row.getChildrenUnmodifiable().stream()
.filter(TableCell.class::isInstance).map(e -> (TableCell<String, String>) e).toList();

for (TableCell<String, String> cell : cells) {
assertEquals(0, cell.getIndex());
}
}
}
28 changes: 28 additions & 0 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableRowTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.assertStyleClassContains;

import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import javafx.scene.control.SelectionMode;
Expand Down Expand Up @@ -990,4 +992,30 @@ protected boolean isItemChanged(String oldItem, String newItem) {

assertTrue(isItemChangedCalled.get());
}

@Test
void testUpdateRowIndexManually() {
TreeTableView<String> table = ControlUtils.createTreeTableView();

TreeTableRow<String> row = new TreeTableRow<>();
row.updateTreeTableView(table);

stageLoader = new StageLoader(row);

row.updateIndex(0);

List<TreeTableCell<String, String>> cells = row.getChildrenUnmodifiable().stream()
.filter(TreeTableCell.class::isInstance).map(e -> (TreeTableCell<String, String>) e).toList();
for (TreeTableCell<String, String> cell : cells) {
assertEquals(0, cell.getIndex());
}

row.updateIndex(1);

cells = row.getChildrenUnmodifiable().stream()
.filter(TreeTableCell.class::isInstance).map(e -> (TreeTableCell<String, String>) e).toList();
for (TreeTableCell<String, String> cell : cells) {
assertEquals(1, cell.getIndex());
}
}
}
54 changes: 54 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 @@ -7536,4 +7536,58 @@ public void startEdit() {

assertEquals(1, startEditCounter.get());
}

@Test
void testReSetItemsWithSameItemShouldUpdateCellIndices() {
treeTableView.setRoot(new TreeItem<>());
treeTableView.getRoot().setExpanded(true);
treeTableView.setShowRoot(false);
treeTableView.setFixedCellSize(24);

final TreeTableColumn<String, String> c = new TreeTableColumn<>("C");
c.setCellValueFactory(value -> new SimpleStringProperty(value.getValue().getValue()));
treeTableView.getColumns().add(c);

for (int i = 0; i < 60; i++) {
treeTableView.getRoot().getChildren().add(new TreeItem<>(String.valueOf(i)));
}
String lastItem = "UniqueLastItem";
TreeItem<String> lastTreeItem = new TreeItem<>(lastItem);
treeTableView.getRoot().getChildren().add(lastTreeItem);

stageLoader = new StageLoader(treeTableView);
stageLoader.getStage().setWidth(300);
stageLoader.getStage().setHeight(300);

// Scroll to the bottom.
VirtualScrollBar scrollBar = VirtualFlowTestUtils.getVirtualFlowVerticalScrollbar(treeTableView);
scrollBar.setValue(1);

Toolkit.getToolkit().firePulse();

IndexedCell<String> row = VirtualFlowTestUtils.getCell(treeTableView, 60);
assertEquals(lastItem, row.getItem());

List<TreeTableCell<String, String>> cells = row.getChildrenUnmodifiable().stream()
.filter(TreeTableCell.class::isInstance).map(e -> (TreeTableCell<String, String>) e).toList();

for (TreeTableCell<String, String> cell : cells) {
assertEquals(60, cell.getIndex());
}

// Re-set the items, but reuse one item from the previous items list.
treeTableView.getRoot().getChildren().setAll(lastTreeItem);

Toolkit.getToolkit().firePulse();

row = VirtualFlowTestUtils.getCell(treeTableView, 0);
assertEquals(lastItem, row.getItem());

cells = row.getChildrenUnmodifiable().stream()
.filter(TreeTableCell.class::isInstance).map(e -> (TreeTableCell<String, String>) e).toList();

for (TreeTableCell<String, String> cell : cells) {
assertEquals(0, cell.getIndex());
}
}
}

1 comment on commit b0e763c

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