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

[Jeanette Winzenburg]

On Wed, 9 Oct 2019 15:49:21 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

> On Wed, 9 Oct 2019 12:25:26 GMT, Hadzic Samir <shadzic at openjdk.org> wrote:
> 
>> The pull request has been updated with additional changes.
>> 
>> ----------------
>> 
>> Added commits:
>>  - e846e51c: Remove TableColumn argument for resizeColumnToFitContent for clarification on TableColumnHeader
>> 
>> Changes:
>>   - all: https://git.openjdk.java.net/jfx/pull/6/files
>>   - new: https://git.openjdk.java.net/jfx/pull/6/files/969ebb51..e846e51c
>> 
>> Webrevs:
>>  - full: https://webrevs.openjdk.java.net/jfx/6/webrev.01
>>  - incr: https://webrevs.openjdk.java.net/jfx/6/webrev.00-01
>> 
>>   Issue: https://bugs.openjdk.java.net/browse/JDK-8207957
>>   Stats: 27 lines in 3 files changed: 13 ins; 1 del; 13 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/NestedTableColumnHeader.java line 170:
> 
>> 169:             TableColumnHeader columnHeader = tableHeader.getColumnHeaderFor(column);
>> 170:             if(columnHeader != null){
>> 171:                 columnHeader.resizeColumnToFitContent(-1);
> 
> Space after `if` and before `{`.
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java line 608:
> 
>> 607:     protected void resizeColumnToFitContent(int maxRows) {
>> 608:         TableColumnBase<?, ?> tc = getTableColumn();
>> 609:         if (!tc.isResizable()) return;
> 
> Here's my suggestion (mind the max character length per line):
> 
> Resizes this {@code TableColumnHeader}'s column to fit the width of its content. The resulting column width 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.).
> 
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java line 618:
> 
>> 617:     }
>> 618: 
>> 619:     private <T,S> void resizeColumnToFitContent(TableView<T> tv, TableColumn<T, S> tc, TableViewSkinBase tableSkin, int maxRows) {
> 
> I think we put a space when casting, like `(TableView) control`. Not sure if it is a rule.
> 
> Since you are adding new API you will need to file a CSR once the API review id done.
> 
> ----------------
> 
> Disapproved by nlisker (Committer).

> 
> 
> Allright @kleopatra , I have indeed removed the TableColumn argument. It is clearer now that we are resizing the TableColumn of the header.
> 

good, IMO :) Are there any tests guaranteeing that new behaviour is the same as old behaviour?

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