On Tue, 29 Mar 2022 20:35:10 GMT, Marius Hanl <mh...@openjdk.org> wrote:
>> 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.
>
> 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.
I found assertions in various other parts of the code (e.g.
`javafx.scene.control.skin.VirtualFlow<T>`) so I assumed this is ok.
> 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?
You're right. I'll move the method.
> 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
You're right. I'll remove this.
-------------
PR: https://git.openjdk.java.net/jfx/pull/757
