[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8218826: TableRowSkinBase: horizontal layout broken if row has padding
From:       Marius Hanl <mhanl () openjdk ! java ! net>
Date:       2022-05-24 21:31:48
Message-ID: IE44Rb1_kllCW35A3RcTcacIznqUEB-Q-cmTvL10PIk=.b07c6939-ba94-4844-bdc2-0d43b06eb984 () github ! com
[Download RAW message or body]

On Tue, 24 May 2022 21:25:23 GMT, Marius Hanl <mhanl@openjdk.org> wrote:

> This PR fixes a problem, where the layout is broken when a `(Tree)TableRow` has \
> padding. As also mentioned in the ticket, the `layoutChildren` method in \
> `TableRowSkinBase` is implemented wrong. 
> The `layoutChildren` method is responsible for layouting all the \
> `(Tree)TableCells`.  When the row has padding, it is subtracted on every \
> `(Tree)TableCell` - which is wrong. 
> Instead the `x` and `y` from `layoutChildren` should be used. (E.g. if `x` is 10, \
> then only the first cell should start at 10 and the other cells follow as usual) \
> Also the `compute...` methods needs to add the padding as well. 
> **Example:**
> _Row padding left right 0:_ 
> [Cell1][Cell2][Cell3]
> _Row padding left right 10:_ 
> [    10    ][Cell1][Cell2][Cell3][    10    ]
> _Same for top bottom._
> 
> When a `fixedCellSize` is set, the padding is currently ignored (also after this \
> PR). Therefore, `y` in the `layoutChildren` method is set to 0 for `fixedCellSize`.
> 
> This may can be discussed in the mailing list (Could be a follow up). To support \
> padding also when a `fixedCellSize` is set, the `compute...` methods needs to also \
> add the padding when a `fixedCellSize` is set (in the `if` clauses) and the \
> `VirtualFlow` needs to add the padding to every row instead of using the \
> `fixedCellSize` directly.

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableRowSkinTest.java \
line 255:

> 253:     private void removedColumnsShouldRemoveCorrespondingCellsInRowImpl() {
> 254:         // Remove the last 2 columns.
> 255:         tableView.getColumns().remove(tableView.getColumns().size() - 2, \
> tableView.getColumns().size());

Note: I added this test in PR: https://github.com/openjdk/jfx/pull/444. While testing \
this PR I found out that this removes just one column, therefore I fixed it right \
away.

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

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


[prev in list] [next in list] [prev in thread] [next in thread]