[Rev 05] RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Mon, 9 Dec 2019 20:58:46 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:

>> Allright, this is a fix for JDK-8207957
> 
> The pull request has been updated with 1 additional commit.

The fix looks good to me.

I left a few minor comments, including adding a missing comma in the API docs, which will also need to be changed in the CSR. Once this is updated I'll review the CSR and then you can move the CSR to Finalize.

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

> 39: import javafx.collections.WeakListChangeListener;
> 40: import javafx.css.*;
> 41: import javafx.css.converter.SizeConverter;

Please revert this change. In general, we do not use wildcard imports.

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

> 604:      * custom implementation (such as ones that exclude the header, exclude {@code null} content, compute the minimum
> 605:      * width etc.).
> 606:      *

There should be a `,` before `etc.`

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

> 1: /*
> 2:  * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

add 2019, so:

  * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.

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

> 55:     @Before
> 56:     public void before(){
> 57:         ObservableList<Person> model = FXCollections.observableArrayList(

Minor: add a space before the `{`

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

> 79:     @After
> 80:     public void after(){
> 81:         sl.dispose();

Minor: add a space before the `{`

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

> 151:         tableView.getItems().get(2).setFirstName("Orrin Davies");
> 152:         tableView.getItems().get(3).setFirstName("Emma Wilson");
> 153: 

It would be nice to not have to repeat the set of strings used. I recommend putting them in a static string array (they appears either 2 or 3 times in the test file). Maybe create 4 static strings for the 4 names?

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



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