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

[Hadzic Samir]

On Thu, 7 Nov 2019 12:48:47 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

> 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).

But what if the resizeCol never worked and the value never changed at all?

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