On Fri, 11 Sep 2026 14:19:25 GMT, Marius Hanl <[email protected]> wrote:
>> 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?
I'm always suspicious of modifying anything that's observable in a live loop.
In my conservative estimation, JavaFX has about 5 million bugs because of that.
So even though in this case, we might only remove very few elements, I'd still
do a bulk notification at the end.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2308#discussion_r3990170563
