On Mon, 29 Mar 2021 11:40:40 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
>> This PR fixes an issue, where table cells are not removed from the table row
>> when the corresponding table column got removed. This will lead to empty
>> "ghost" cells laying around in the table.
>> This bug only occurs, when a fixed cell size is set to the table.
>>
>> I also added 3 more tests beside the mandatory test, which tests my fix.
>> 1 alternative test, which tests the same with no fixed cell size set.
>> The other 2 tests are testing the same, but the table columns are set
>> invisible instead.
>>
>> -> Either the removal or setVisible(false) of a column should both do the
>> same: Remove the corresponding cells, so that there are no empty cells.
>> Therefore, the additional tests make sure, that the other use cases (still)
>> works and won't break in future (at least, without red tests ;)).
>> See also: TableRowSkinBase#updateCells(boolean)
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableRowSkinBase.java
> line 556:
>
>> 554: }
>> 555: getChildren().removeAll(toRemove);
>> 556: } else if (resetChildren || cellsEmpty) {
>
> just curious : any idea why fixedCellSize was special-cased here? Not to
> clean them up sounds (and is) very much wrong, so there must have been a
> reason?
The '!fixedCellSizeEnabled' check is not needed, as we already check for
fixedCellSizeEnabled in the main if condition.
`if (fixedCellSizeEnabled) {...}`
So before, it was like:
`if (fixedCellSizeEnabled) {...} `
`else if (!fixedCellSizeEnabled && (resetChildren || cellsEmpty)) {...}`
So we only execute the else condition, if fixedCellSizeEnabled is not true ->
!fixedCellSizeEnabled. -> The check is not necessary, as fixedCellSizeEnabled
must be false, when we come to the else condition.
The variable fixedCellSizeEnabled is also a primitive boolean, so it can't be
null, making this check obsolete.
-------------
PR: https://git.openjdk.java.net/jfx/pull/444
