RFR: 8320232: Cells duplicated when table collapsed and expanded

[Andy Goryachev]

On Mon, 29 Jul 2024 18:45:23 GMT, Marius Hanl <mhanl at 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