RFR: 8189354: ArrayIndexOutOfBoundsException when listening to selection changes on TreeTableView [v2]

[mstr2]

On Sat, 24 Apr 2021 18:29:47 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

> Also, if you could explain the logic changes a bit, that would be helpful.

Here's my guess what the original implementation tried to accomplish:


    ...
    while (endPos < removed.size()) {
        if (removed.get(startPos) == removed.get(endPos) - 1) {
            endPos++;
            continue;
        }
        ...
    }


`removed` is an ascending list of indices that should be removed from `selectedIndices`. The loop tries to find uninterrupted ascending ranges within that list (for example, `[2, 3], [5, 6, 7]`). However, the condition `removed.get(startPos) == removed.get(endPos) - 1` will always fail after just a single iteration, since `startPos` is not modified before `continue`, which means that the loop will never detect any range longer than 2 elements.

The second issue with the original code is at this line:

    selectedIndices._nextRemove(selectedIndices.indexOf(removed.get(startPos)), removed.subList(startPos, endPos));

The first parameter of `_nextRemove` is the index at which the removal happened, and this index must match the current state of the list.

Let's assume that `selectedIndices` contains `[1, 2, 3, 4, 5, 6, 7, 8]`, and `removed` contains `[2, 3, 5, 6, 7]`.
In this case, the first removed range is `[2, 3]`, which happens at index `1`. The second removal must account for the already-removed elements: `[5, 6, 7]` at index `2`.

Here's the new implementation (see the comments):

    for (int endPos = 0, max = removed.size(); endPos < max; ++endPos) {

        // This loop increments endPos as long as the next index is adjacent to the previous index
        while (endPos < max - 1 && removed.get(endPos) == removed.get(endPos + 1) - 1) {
            ++endPos;
        }

        // totalRemoved accounts for the elements which have already been removed, so that the
        // index will refer to the state of the list after all previous removal changes have been applied
        selectedIndices._nextRemove(
            selectedIndices.indexOf(removed.get(startPos)) - totalRemoved,
            removed.subList(startPos, endPos + 1));

        totalRemoved += endPos - startPos + 1;
        startPos = endPos + 1;
    }

-------------

PR: https://git.openjdk.java.net/jfx/pull/480