8231644: TreeTableView Regression: Indentation wrong using Label as column content type #568

Maran23 · 2021-07-07T00:25:15Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8231644: TreeTableView Regression: Indentation wrong using Label as column content type

Reviewers

Ajit Ghaisas (@aghaisas - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/568/head:pull/568
$ git checkout pull/568

Update a local copy of the PR:
$ git checkout pull/568
$ git pull https://git.openjdk.java.net/jfx pull/568/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 568

View PR using the GUI difftool:
$ git pr show -t 568

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/568.diff

bridgekeeper · 2021-07-07T00:25:37Z

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-07-07T00:30:13Z

Webrevs

kevinrushforth · 2021-07-07T18:45:43Z

/reviewers 2

openjdk · 2021-07-07T18:46:14Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

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

…via leftLabelPadding

…indentation � Conflicts: � modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java

aghaisas · 2021-09-24T10:48:53Z

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

Maran23 · 2021-09-24T16:37:59Z

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?

Just tried them. They both work fine. :)

aghaisas · 2021-09-27T10:53:47Z

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?

Just tried them. They both work fine. :)

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?

Just tried them. They both work fine. :)
That's great!

kevinrushforth

Both the fix and test look good.

openjdk · 2021-09-29T15:14:14Z

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8231644: TreeTableView Regression: Indentation wrong using Label as column content type

Reviewed-by: aghaisas, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 37 new commits pushed to the master branch:

55faac4: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
478512b: 8274107: Cherry pick GTK WebKit 2.32.4 changes
4b9cb21: 8273969: Memory Leak on the Runnable provided to Platform.startup
338b999: 8273946: Move clearQuad method to BaseShaderGraphics superclass
5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars
f59a057: 8090547: Allow for transparent backgrounds in WebView
d356340: 8172095: Let Node.managed become CSS-styleable
b2ecfac: 8271398: GTK3 drag view image swaps red and blue color channels
7329279: 8273754: Re-introduce Automatic-Module-Name in empty jars
51265c0: 8270107: Open source FXMediaPlayer test app
... and 27 more: https://git.openjdk.java.net/jfx/compare/653e24baef01c52ec11efab255bf506ee55e4d2a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@aghaisas, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Maran23 · 2021-09-29T15:21:32Z

/integrate

openjdk · 2021-09-29T15:22:31Z

@Maran23
Your change (at version 3776a37) is now ready to be sponsored by a Committer.

kevinrushforth · 2021-09-29T15:27:46Z

/sponsor

openjdk · 2021-09-29T15:29:23Z

Going to push as commit 30f5606.
Since your change was applied there have been 37 commits pushed to the master branch:

55faac4: 8271474: Tree-/TableCell: inconsistent edit event firing pattern
478512b: 8274107: Cherry pick GTK WebKit 2.32.4 changes
4b9cb21: 8273969: Memory Leak on the Runnable provided to Platform.startup
338b999: 8273946: Move clearQuad method to BaseShaderGraphics superclass
5c355fa: 8090158: Wrong implementation of adjustValue in scrollBars
f59a057: 8090547: Allow for transparent backgrounds in WebView
d356340: 8172095: Let Node.managed become CSS-styleable
b2ecfac: 8271398: GTK3 drag view image swaps red and blue color channels
7329279: 8273754: Re-introduce Automatic-Module-Name in empty jars
51265c0: 8270107: Open source FXMediaPlayer test app
... and 27 more: https://git.openjdk.java.net/jfx/compare/653e24baef01c52ec11efab255bf506ee55e4d2a...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-29T15:29:36Z

@kevinrushforth @Maran23 Pushed as commit 30f5606.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8231644: fixed wrong indentation for tree cells with graphic only

e84ad59

openjdk bot added the rfr Ready for review label Jul 7, 2021

kevinrushforth requested a review from aghaisas July 7, 2021 18:45

kevinrushforth reviewed Jul 7, 2021

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java Outdated Show resolved Hide resolved

calculated indentation in every method now which was previously done …

c869849

…via leftLabelPadding

kevinrushforth self-requested a review July 9, 2021 13:36

Merge branch 'master' of https://github.com/openjdk/jfx into 8231644-…

3776a37

…indentation � Conflicts: � modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java

aghaisas reviewed Sep 24, 2021

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableCellSkin.java Show resolved Hide resolved

aghaisas approved these changes Sep 27, 2021

View reviewed changes

kevinrushforth approved these changes Sep 29, 2021

View reviewed changes

openjdk bot added the ready Ready to be integrated label Sep 29, 2021

openjdk bot added the sponsor Ready to sponsor label Sep 29, 2021

openjdk bot closed this Sep 29, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Sep 29, 2021

Maran23 mentioned this pull request Oct 31, 2021

8274061: Tree-/TableRowSkin: misbehavior on switching skin #632

Closed

3 tasks

Maran23 deleted the 8231644-indentation branch November 9, 2021 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8231644: TreeTableView Regression: Indentation wrong using Label as column content type #568

8231644: TreeTableView Regression: Indentation wrong using Label as column content type #568

Maran23 commented Jul 7, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 7, 2021

mlbridge bot commented Jul 7, 2021 •

edited

Loading

kevinrushforth commented Jul 7, 2021

openjdk bot commented Jul 7, 2021

aghaisas commented Sep 24, 2021

Maran23 commented Sep 24, 2021

aghaisas commented Sep 27, 2021

kevinrushforth left a comment

openjdk bot commented Sep 29, 2021 •

edited

Loading

Maran23 commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

kevinrushforth commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

8231644: TreeTableView Regression: Indentation wrong using Label as column content type #568

8231644: TreeTableView Regression: Indentation wrong using Label as column content type #568

Conversation

Maran23 commented Jul 7, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 7, 2021

mlbridge bot commented Jul 7, 2021 • edited Loading

Webrevs

kevinrushforth commented Jul 7, 2021

openjdk bot commented Jul 7, 2021

aghaisas commented Sep 24, 2021

Maran23 commented Sep 24, 2021

aghaisas commented Sep 27, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 29, 2021 • edited Loading

Maran23 commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

kevinrushforth commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

Maran23 commented Jul 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 7, 2021 •

edited

Loading

openjdk bot commented Sep 29, 2021 •

edited

Loading