On Wed, 16 Mar 2022 08:20:59 GMT, Robert Lichtenberger <rlich...@openjdk.org>
wrote:
> This fix respects a row factory, if present.
> It will put the cell that is used to measure the column width as child below
> the row.
> In that way the row's style will be used.
The approach looks good. I left some comments and questions
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 650:
> 648: }
> 649: Callback<TableView<T>, TableRow<T>> rowFactory =
> tv.getRowFactory();
> 650: TableRow<T> tableRow = rowFactory != null ? rowFactory.call(tv)
> : new TableRow<>();
When there is no row factory, we probably should just return like in line
632-633?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 653:
> 651: tableSkin.getChildren().add(tableRow);
> 652: tableRow.applyCss();
> 653: ((SkinBase<?>) tableRow.getSkin()).getChildren().add(cell);
I don't think this is a safe cast. Instead, you probably should do an
`instanceof SkinBase` check before
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
line 667:
> 665: if ((cell.getText() != null && !cell.getText().isEmpty()) ||
> cell.getGraphic() != null) {
> 666: tableRow.applyCss();
> 667: cell.applyCss();
Just wondering: Is `cell.applyCss();` still needed here?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java
line 259:
> 257: /**
> 258: * @test
> 259: * @bug 8251480 Row style must affect the required column width
This annotations are not needed, although they do not hurt (just a note)
-------------
Changes requested by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/757
