On Mon, 29 Jul 2024 18:45:23 GMT, Marius Hanl <mh...@openjdk.org> wrote:
> This PR fixes a bug where a `TableView` inside a `TitledPane` may 'duplicate'
> the cells. This actually has nothing to do with the `TitledPane`, but it
> triggered the bug easily due to its nature to change the size of his content
> when collapsing.
>
> The expansion change of a `TitledPane` triggered an event where the
> underlying `VirtualFlow` was adding cells to the pile (for reuse) and later
> cleaning them all up without resetting the index to -1.
> This led to a bug where two cells had the same index and therefore received
> an edit event, although just one should (and is visible, the other cell has
> no parent -> orphan node).
>
> The fix is to always reset the index to -1. This was already done before,
> just not everywhere, for all cells.
> So before we clear the pile (or cells), we always reset the index to -1.
> Added a bunch of tests, some pass before and after.
This is a definitely the right fix.
The TitledPane reproducer fails without and works correctly with the change. I
don't see the duplicate startEdit() with the Dialog reproducer, please clarify
the steps to reproduce and what is expected/observed.
Left a couple of minor comments, will re-approve if you choose to make changes.
As with everything involving VirtualFlow, I'd like a second reviewer. Maybe
@johanvos ?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 808:
> 806: vertical = new BooleanPropertyBase(true) {
> 807: @Override protected void invalidated() {
> 808: resetIndex(cells);
minor: [cells|pile]clear() can be combined with resetIndex(), something like
private void clear(ArrayLinkedList<T> cells) {
for (T cell : cells) {
cell.updateIndex(-1);
}
cells.clear();
}
as these operations seem to be called in pairs
modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java
line 7448:
> 7446: treeTableView.getRoot().setExpanded(true);
> 7447: for (int i = 0; i < 4; i++) {
> 7448: TreeItem<String> parent = new TreeItem<>("item - " + i);
minor: this looks like an unrelated change
-------------
PR Review: https://git.openjdk.org/jfx/pull/1521#pullrequestreview-2206165807
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1521#pullrequestreview-2206180955
PR Review Comment: https://git.openjdk.org/jfx/pull/1521#discussion_r1696013498
PR Review Comment: https://git.openjdk.org/jfx/pull/1521#discussion_r1696009737
