Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public class TreeTableViewSkin<T> extends TableViewSkinBase<T, TreeItem<T>, Tree
ObjectProperty<ObservableList<TreeItem<T>>> tableBackingListProperty;
private final TreeTableViewBehavior<T> behavior;
private final EventHandler<TreeItem.TreeModificationEvent<T>> rootListener;

private boolean treeStructureDirty;


/* *************************************************************************
Expand Down Expand Up @@ -147,6 +147,12 @@ public TreeTableViewSkin(final TreeTableView<T> control) {
control.requestLayout();
break;
}
else if (eventType.equals(TreeItem.<T>childrenModificationEvent())) {
Copy link
Member

Choose a reason for hiding this comment

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

minor: else if should be in the previous line to follow the Java Code Convention: } else if (..)

markItemCountDirty();
treeStructureDirty = true;
control.requestLayout();
break;
}
eventType = eventType.getSuperType();
}
}
Expand Down Expand Up @@ -347,6 +353,10 @@ private TreeTableRow<T> createCell() {
// A unit test exists in TreeTableViewTest to ensure that
// the performance issue covered in JDK-8147483 doesn't regress.
// requestRebuildCells();
treeStructureDirty = false;
} else if (treeStructureDirty) {
requestRebuildCells();
treeStructureDirty = false;
} else {
needCellsReconfigured = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6439,6 +6439,70 @@ private void test_rt_40319(boolean toRight, boolean toBottom, boolean useMouse)
sl.dispose();
}

@Test
void test_jdk_8356770_reparentingItem() {
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend a test name that reflects what exactly we want to test here. You can reference the JDK Ticket in a javadoc comment.
Example:

/**
* The content display should also be taken into consideration when measuring the width.
* See also: <a href="https://bugs.openjdk.org/browse/JDK-8186188">JDK-8186188</a>
*/
@Test
public void testResizeColumnToFitContentWithGraphicAlignment() {

TreeItem<String> itemA1 = new TreeItem<>("item A1");
TreeItem<String> itemA2 = new TreeItem<>("item A2");
TreeItem<String> itemA = new TreeItem<>("item A");
itemA.getChildren().addAll(itemA1, itemA2);
itemA.setExpanded(true);

TreeItem<String> itemB1 = new TreeItem<>("item B1");
TreeItem<String> itemB2 = new TreeItem<>("item B2");
TreeItem<String> itemB = new TreeItem<>("item B");
itemB.getChildren().addAll(itemB1, itemB2);
itemB.setExpanded(true);

TreeItem<String> root = new TreeItem<>("Root");
root.getChildren().addAll(itemA, itemB);
root.setExpanded(true);

TreeTableView<String> table = new TreeTableView<>();
TreeTableColumn<String, String> col = new TreeTableColumn<>("Name");
col.setCellValueFactory(cd -> new SimpleStringProperty(cd.getValue().getValue()));
table.getColumns().add(col);
table.setRoot(root);
table.setShowRoot(true);

stageLoader = new StageLoader(table);
Toolkit.getToolkit().firePulse();
Copy link
Member

Choose a reason for hiding this comment

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

Is this call really needed? Just asking because most of the time, it is not needed right after the StageLoader


// Find "item B" row and record its disclosure node indent
TreeTableRow<String> rowBefore = findRow(table, "item B");
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend using VirtualFlowTestUtils.getVirtualFlow(table).getVisibleCell(index);
The index should be easy to find out, since we always know where the TreeItem should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exact, I usually prefer not to hardcode things, but sure in this case I will modify it.

assertNotNull(rowBefore, "item B row should not be null");
double xBefore = disclosureX(rowBefore);

// Reparenting "item B" under "item A"
root.getChildren().remove(itemB);
itemA.getChildren().add(itemB);
Toolkit.getToolkit().firePulse();

TreeTableRow<String> rowAfter = findRow(table, "item B");
assertNotNull(rowAfter, "item B row should not be null");
Copy link
Member

Choose a reason for hiding this comment

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

Null check is not needed, since we do test the disclosure node position instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The null check is just for safety that the item exist, not that much important.

double xAfter = disclosureX(rowAfter);

assertTrue(xAfter > xBefore,
"Indentation of item B must increase after reparenting");
}

private static TreeTableRow<String> findRow(TreeTableView<String> table, String value) {
for (Node n : table.lookupAll(".tree-table-row-cell")) {
if (n instanceof TreeTableRow<?>) {
TreeTableRow<String> row = (TreeTableRow<String>) n;
TreeItem<String> ti = row.getTreeItem();
if (ti != null && value.equals(ti.getValue())) {
return row;
}
}
}
return null;
}

private static double disclosureX(TreeTableRow<String> row) {
Copy link
Member

Choose a reason for hiding this comment

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

I know IntelliJ makes methods static by default (when extracting), but I don't think it makes sense here

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's perfectly fine to have a static method here (and in tests in general).

Node disclosureNode = row.lookup(".tree-disclosure-node");
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just call: row.getDisclosureNode() ?

return disclosureNode == null ? 0.0 : (disclosureNode.getLayoutX() + disclosureNode.getTranslateX());
}

@Test public void test_jdk_8144681_removeColumn() {
TreeTableView<Book> table = new TreeTableView<>();

Expand Down