RFR: 8231644: TreeTableView Regression: Indentation wrong using Label as column content type [v3]
Ajit Ghaisas
aghaisas at openjdk.java.net
Fri Sep 24 10:53:01 UTC 2021
On Tue, 27 Jul 2021 05:45:50 GMT, Marius Hanl <mhanl at openjdk.org> wrote:
>> This PR fixes a long standing issue with the TreeTableView indentation.
>>
>> 
>>
>> 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
More information about the openjfx-dev
mailing list