On Thu, 10 Sep 2026 23:31:27 GMT, Michael Strauß <[email protected]> wrote:
>> Marius Hanl has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> adjust copyright, remove import
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 2856:
>
>> 2854:
>> 2855: if (cell.getParent() != null) {
>> 2856: sheetChildren.remove(cell);
>
> Since is an `ObservableList`, this could potentially run arbitrary code in a
> loop. What do you think of adding the children to a temporary list, and then
> `removeAll` that list?
Very good question, thought about it as well, but we usually only remove one or
at a maximum two cells. So I think it is not worth it.
Tested with the reproducer in the ticket. Using a:
- `fixedCellSize`, we usually just remove one cell sometimes
- Without `fixedCellSize`, we also remove one cell, very rarely two
- With cells that may have a very different height, we usually still only
remove 1-2 cells here
- When resizing a `ListView` (-> bigger) to have more cells, then resizing it
down again will also remove 1-2 cells
The only test case where I could get many cell removals at a time is by
changing the `Window` to maximized, then again to the normal size (so both with
one click, not resizing the `Window` manually). In this case I got ~15 cells
removed at a time.
Rethinking now, I'm not 100% sure. What do you think?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2308#discussion_r3990055089
