RFR: 8251480: TableColumnHeader: calc of cell width must respect row styling [v2]

Marius Hanl mhanl at openjdk.java.net
Tue Mar 29 20:43:54 UTC 2022


On Thu, 24 Mar 2022 06:14:42 GMT, Robert Lichtenberger <rlichten at 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.
>
> Robert Lichtenberger has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8251480: TableColumnHeader: calc of cell width must respect row styling
>   
>   Old behaviour restored for table rows with custom skins.
>   Unnecessary call to cell.applyCss() removed.

Just commented some minor things. 
But overall it looks good.
I did some manual tests as well and can confirm that this works. :)

![image](https://user-images.githubusercontent.com/66004280/160703014-addf1297-082b-453c-9ba9-d9cec90ff0d2.png)

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java line 715:

> 713:             tableRow = createMeasureRow(tv, tableSkin, null);
> 714:         }
> 715:         assert tableRow.getSkin() instanceof SkinBase<?>;

Not sure if there is some convention about code asserts, maybe some else can tell. At least I never saw one in a PR here.

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 276:

> 274:     }
> 275: 
> 276:     private TableRow<Person> createSmallRow(TableView<Person> tableView) {

Minor: Move this method to the other row method below?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTest.java line 295:

> 293:     private TableRow<Person> createCustomRow(TableView<Person> tableView) {
> 294:         TableRow<Person> row = new TableRow<>() {
> 295:             protected javafx.scene.control.Skin<?> createDefaultSkin() {

Very minor: `javafx.scene.control.` is not needed

-------------

Marked as reviewed by mhanl (Author).

PR: https://git.openjdk.java.net/jfx/pull/757


More information about the openjfx-dev mailing list