RFR: JDK-8316590: Rendering artifact after JDK-8311983

[Johan Vos]

On Thu, 21 Sep 2023 14:27:59 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

> Fixes the regression by basically reverting one change introduced in https://bugs.openjdk.org/browse/JDK-8311983.
> The problem is that it is actually required to get the size from a cell with the index -1, which technically does not exist (the accumCell is used then).
> 
> Furthermore, unlike the name suggests, the call to `addLeadingCells()` is always needed, even if there are none.
> This is because the method does much more than what you would think first. 
> 
> In future, it would probably be good to revisit this code. Also performance wise, since this looks like something that can be optimized. But that is another story, not related to this fix.

The PR indeed fixes a regression caused by the fix for PR-8311983. Calling addLeadingCells with a wrong start index (the index of the cell before the top-one) and a wrong offset causes the issue you observed. I agree that the name "addLeadingCells" is a bit confusing, as is the name "index". 
As for the test: I understand it will detect issues with a new cell being reassigned to content that should still be owned by its original cell. However, that doesn't have to be theoretically wrong. There is no guarantee in the VirtualFlow API about order and shuffles in the cells it is using internally, so a change in internal implementation might cause this test to fail.
It might be better to check for updateIndex() to be called on the Cell implementation, and fail if unexpectedly an item is assigned to a cell that is drawn at a wrong position.

The latter is what you can observe with the current issue: 
inside `addLeadingCells`, the first `while` loop will be executed once (if you pass the wrong index), leading to this:

    cell = getAvailableCell(index);
    setCellIndex(cell, index);
...
    positionCell(cell, offset);


Hence, the result is that the item at "index" 0 is positioned at the wrong offset, which is something you can detect with a test.

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

PR Comment: https://git.openjdk.org/jfx/pull/1246#issuecomment-1730206988