On Mon, 9 Dec 2019 20:58:46 GMT, Hadzic Samir <shad...@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

Reply via email to