On Tue, 27 Jul 2021 05:45:50 GMT, Marius Hanl <mh...@openjdk.org> wrote:

>> This PR fixes a long standing issue with the TreeTableView indentation.
>> 
>> ![image](https://user-images.githubusercontent.com/66004280/124681647-473e7380-dec9-11eb-906d-4228fc39cbf9.png)
>> 
>> In short:
>> **TreeTableCellSkin** overrides **leftLabelPadding()** to calculate the 
>> indentation (the result of this method is added to **x**).
>> While this is fine, this method is not always called (by 
>> **LabeledSkinBase#layoutLabelInArea**), e.g. when no text is set. 
>> So when a TreeTableCell only sets a graphic (e.g. via **setGraphic()** in 
>> **updateItem()**), the indentation will be messed up.
>> 
>> Fixed this by adding the calculated indentation to **x** before we call 
>> **layoutChildren()**.
>> 
>> We also need/should adjust every other location where `leftLabelPadding()` 
>> was used:
>> - **computePrefHeight** -> prefWidth() is always called with -1, so nothing 
>> got broken by not overriding this, but we should do it of course to be 
>> accurate in case we do one day.
>> - **computePrefWidth** -> this is needed for auto sizing. I saw that it was 
>> slightly off without, so this 100% needed.
>> - **computeMinWidth** -> the min width of a cell is not used, so nothing got 
>> broken by not overriding this but same as in computePrefHeight(), we should 
>> comply with the specs.
>> - **layoutChildren** -> I saw a slight off sizing if the indentation is not 
>> subtracted to the width.
>> 
>> As a result of this, all method do effectively the same as they did with an 
>> overridden `leftLabelPadding()` (just earlier/later and always now of 
>> course).
>> Note: I also added some tests which pass before and pass after.
>
> Marius Hanl has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' of https://github.com/openjdk/jfx into 
> 8231644-indentation
>    
>     Conflicts:
>      
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java
>  - calculated indentation in every method now which was previously done via 
> leftLabelPadding
>  - 8231644: fixed wrong indentation for tree cells with graphic only

Overall the fix looks ok. The new test fails without the fix and passes with it.

Can you please confirm the test programs provided in 2 duplicated bugs of 
JDK-8231644 also work as expected with this fix?

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java
 line 127:

> 125:             double leftInset) {
> 126:         if (isDeferToParentForPrefWidth) {
> 127:             return super.computePrefWidth(height, topInset, rightInset, 
> bottomInset, leftInset) + calculateIndentation();

If we are deferring to Parent for the preferred width, then we need not add 
indentation. 
I think, we should interchange line 127 and line 130.

-------------

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

Reply via email to