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

Jeanette Winzenburg fastegal at openjdk.org
Thu Nov 7 12:48:47 UTC 2019

On Wed, 30 Oct 2019 13:59:08 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:

> The pull request has been updated with additional changes.
> ----------------
> Added commits:
>  - 2b088993: Add @implSpec tag for javadoc of TableColumnHeader
> Changes:
>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>   - new: https://git.openjdk.java.net/jfx/pull/6/files/1f1f7c44..2b088993
> Webrevs:
>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.04
>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.03-04
>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>   Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod
>   Patch: https://git.openjdk.java.net/jfx/pull/6.diff
>   Fetch: git fetch https://git.openjdk.java.net/jfx pull/6/head:pull/6

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

> 53:     private MyTableColumnHeader tableColumnHeader;
> 54: 
> 55:     @Before

If I understand the intention correctly, the only reason for subclassing the TableView with a custom skin, headerRow, etc deep down into a custom columnHeader is to access its protected resizeColumnToFitContent? If so, an alternative approach might be to 

a) add a accessing method to TableColumnHeaderUtil in the shims hierarchy
b) don't subclass but use that new accessor

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

> 60:     @Test
> 61:     public void test_resizeColumnToFitContent() {
> 62:         ObservableList<Person> model = FXCollections.observableArrayList(

This method actually is three-in-one :) It's testing

a) trigger a resize initially doesn't change the with 
b) change content to larger and trigger a resize increases column width
c) change content to smaller and trigger a resize decreases column width

Not sure about conventions here, but I would separate them out into distinct methods.

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

> 116:         column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(
> 117:                 tableView, new TablePosition<Person, String>(tableView, 0, column), (EventType) eventType, "This is a big text inside that column"));
> 118:         tableColumnHeader.resizeCol();

that looks like a rather convoluted way of changing content - what's wrong with straightforward changing the firstName of the first item :)

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

> 120:                 width < column.getWidth());
> 121: 
> 122:         column.getOnEditCommit().handle(new TableColumn.CellEditEvent<Person, String>(

feels a bit unspecific (though I can't think of how to do it better - the internal measuring is ... doohoo ;)

what I usually do in such a case is to take this as a first step, then change content back to original and again trigger a resize: now the current width should be the same as the initial width.

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

> 132:         assertTrue("Column width must be smaller",
> 133:                 width > column.getWidth());
> 134:     }

same as above but the other way round.

Until, the tests verified that the column width is effected by cell content and goes into the course direction of what is expected. What's still missing are tests that cover the complete specification (we now have one :), that is verify that/how resize is indeed depend on

- header content 
- maxRows 

Plus maybe that custom implementations are respected: f.i. doing nothing, making them constant, header only

An open question for me is: what is the standard procedure here? 

a) there are no tests around sizing (that I could find, maybe overlooked them)
b) the change in this pull request is (more or less) simply moving around old code and pulling from private into protected scope, no changes in implementation/functionality

Now is the contributor responsible for writing those missing tests? It can be considered a good opportunity ;) And strictly speaking is mandatory (probably?) for all public/protected api. On the other extreme, doing nothing (no tests because basically nothing changed) might be appropriate as well.


Disapproved by fastegal (Author).

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

More information about the openjfx-dev mailing list