RFR: 8207957: TableSkinUtils should not contain actual code implementation

[Kevin Rushforth]

On Fri, 15 Nov 2019 12:59:15 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

> On Wed, 13 Nov 2019 11:26:54 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:
> 
>> On Thu, 7 Nov 2019 12:07:55 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:
>> 
>>> On Fri, 1 Nov 2019 10:59:57 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:
>>> 
>>>> On Tue, 29 Oct 2019 13:19:27 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:
>>>> 
>>>>> On Wed, 9 Oct 2019 16:01:38 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:
>>>>> 
>>>>>> On Wed, 9 Oct 2019 12:26:31 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:
>>>>>> 
>>>>>>> On Mon, 7 Oct 2019 10:22:11 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:
>>>>>>> 
>>>>>>>> On Fri, 4 Oct 2019 06:13:48 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:
>>>>>>>> 
>>>>>>>>> Allright, this is a fix for JDK-8207957
>>>>>>>>> 
>>>>>>>>> ----------------
>>>>>>>>> 
>>>>>>>>> Commits:
>>>>>>>>>  - 969ebb51: Fixing TableColumnHeaderTest
>>>>>>>>>  - 9d379619: Removing Tablecolumnbasehelper
>>>>>>>>>  - 4fe020fc: Fix javadoc for TableColumnHeader
>>>>>>>>>  - c422c80f: Minor modification and uni test added.
>>>>>>>>>  - b2bdfb5b: Change resizeColumn to protected without static in order to be able to override
>>>>>>>>>  - 3b9b7903: Second option for resizeColumnToFitContent in TableColumnHeader
>>>>>>>>>  - d91c56e5: First attempt to extract resizeColumnToFitContent
>>>>>>>>> 
>>>>>>>>> Changes: https://git.openjdk.java.net/jfx/pull/6/files
>>>>>>>>>  Webrev: https://webrevs.openjdk.java.net/jfx/6/webrev.00
>>>>>>>>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>>>>>>>>   Stats: 523 lines in 4 files changed: 330 ins; 187 del; 6 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/main/java/javafx/scene/control/skin/TableColumnHeader.java line 600:
>>>>>>>> 
>>>>>>>>> 599:      * expensive if the number of rows is large. Subclass can either call this method or override it (no need to call
>>>>>>>>> 600:      * {@code super()}) to provide their custom algorithm.
>>>>>>>>> 601:      *
>>>>>>>> 
>>>>>>>> see https://github.com/javafxports/openjdk-jfx/pull/289#discussion_r245482213 - I think I made a suggestion (that probably needs some native speaker's fine tuning :)
>>>>>>> 
>>>>>>> Allright @kleopatra , I have indeed removed the TableColumn argument. It is clearer now that we are resizing the TableColumn of the header.
>>>>>>> 
>>>>>>> I have updated the description a bit but really I'm really not good at method description so I'm open to all suggestions..
>>>>>> 
>>>>>> A Draft CSR was filed here: [JDK-8215554](https://bugs.openjdk.java.net/browse/JDK-8215554). It will need to be updated once the API review is complete.
>>>>> 
>>>>> Do you think this looks good now @kleopatra @nlisker ?
>>>> 
>>>> Yes I was waiting for the final 'go'. I will update it asap, thanks
>>>> 
>>>> Le ven. 1 nov. 2019 à 09:49, Jeanette Winzenburg <notifications at github.com>
>>>> a écrit :
>>>> 
>>>>> *@kleopatra* commented on this pull request.
>>>>> ------------------------------
>>>>>
>>>>> In
>>>>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java
>>>>> <https://github.com/openjdk/jfx/pull/6#discussion_r341492115>:
>>>>>
>>>>> > +     * Resizes this {@code TableColumnHeader}'s column to fit the width of its content.
>>>>> +     *
>>>>> +     * @implSpec The resulting column width for this implementation is the maximum of the preferred width of the header
>>>>> +     * cell and the preferred width of the first {@code maxRow} cells.
>>>>> +     * <p>
>>>>> +     * Subclasses can either use this method or override it (without the need to call {@code super()}) to provide their
>>>>> +     * custom implementation (such as ones that exclude the header, exclude {@code null} content, compute the minimum
>>>>> +     * width etc.).
>>>>> +     *
>>>>> +     * @param maxRows the number of rows considered when resizing. If -1 is given, all rows are considered.
>>>>> +     * @since 14
>>>>>
>>>>> looks good to me :) You would have to update the crs eventually, right?
>>>>>
>>>>> —
>>>>> You are receiving this because you authored the thread.
>>>>> Reply to this email directly, view it on GitHub
>>>>> <https://github.com/openjdk/jfx/pull/6?email_source=notifications&email_token=ACEL43PISZ6DM46QLSYDLR3QRPURHA5CNFSM4I5EYYAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ7XYEQ#discussion_r341492115>,
>>>>> or unsubscribe
>>>>> <https://github.com/notifications/unsubscribe-auth/ACEL43PROHPIZYN7WA5M6KDQRPURHANCNFSM4I5EYYAA>
>>>>> .
>>>>>
>>> 
>>> As to the test, it's a bit ... confusing, will try to suggest some changes - take them with a grain of salt, though, because I'm still groping to find my path through the wilderness here :)
>> 
>> I have changed the unit test according to @kleopatra review and also add two more unit test regarding the headerContent and maxRows.
> 
> looks good to me - would approve if I could :)

> looks good to me - would approve if I could :)

You can add yourself as a reviewer, even though it wouldn't count as an approving review by someone with a "R"eviewer role in the project.

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