Transform a change by using an undeleted visible cell or by ignoring exact replacements

[JordanMartinez]

Coming from FXMisc/RichTextFX#390 and #39, the code did not account for the possibility of a deletion that occurs before a StartOffStart's itemIndex and continues into the viewport. Thus, the code before this PR would scroll to the first cell even if this was not desired. This PR fixes the issue.

However, in the situation where the removedSize == addedSize, one does not know whether the offset value should be adjusted to 0.0 or left as is so that the viewport appears not to scroll at all.
Using RichTextFX as an example, if one scrolled the viewport slightly so that the first visible line was only halfway visible before removing some regular text and replacing it with a tall image, should the resulting offset value be 0.0 or left as is? What might prevent undesirable scrolling in one situation may force the same in another.

[TomasMikula]

This doesn't make sense to me. First of all, the condition && itemIndex <= pos + removedSize is superfluous: the test above failed, so by this time we know that itemIndex < pos + removedSize. So it is effectively overriding the next else if branch.

[JordanMartinez]

Mm... true. I misread the code.

However, the StartOffStart object that is executing this method in this case is
StartOffStart(127, 0.0) and the parameters are pos = 0, removed = 200, and added = 200.

[JordanMartinez]

The present code has this kind of logic:

1. If the portion of cells that would be displayed above the viewport's anchor cell are deleted, then update the anchor cell's index to point to the viewport's original anchor cell.

   • by using the difference of the removedSize and addedSize, the index is either incremented by that difference, decremented by that amount, or an equivalent TargetPosition is returned, though the original could be returned in order to use less memory (extremely insignificant optimization).

2. If a deletion deletes the anchor cell, then set the anchor cell's index to be the start of the deletion.

   • this assumes that nothing was added in that same process, thus causing the bug in RTFX

3. Otherwise, a deletion occurred after the anchor cell, which does not change it in any way, so just return the original TargetPosition.

How should the second case be changed? If nothing is added, then the anchor cell's index should be the start of the deletion. This is what the code currently does. But what if something is also added? I see three possibilities (| represent the edges of the viewport and * represents the anchor cell's index):

// Possibility 1a: index is deleted, but addedSize < index; viewport is partially deleted
Before: 00|11*29|33
Delete: 00|11*2
Added:  44
Result: 44     9 33 // what should be * ?

Did you notice that the 9 was not deleted? To prevent as much change to the viewport as possible, wouldn't it be wiser to change the anchor cell to convert the TargetPosition into one that would use the 9 as its anchor cell and adjust the offset value, so that this new anchor cell appears not to change at all? Granted, if there are no cells before 9 after a deletion/replacement or if there are cells but not enough to entirely fill up the viewport, this would be a waste of time. However, if there are additional cells before 9 that can fill up the remaining space in the viewport, then only the cells before 9 would be rendered with the replaced content and 9 would appear unchanged.

// Possibility 1b: index is deleted, but addedSize < index; viewport is completely deleted
Before: 00|11*2|63
Delete: 00|11*2
Added:  44
Result: 44      63

If that is not the situation when this deletion occurs (i.e., there are no other visible cells that could be used as the new anchor cell index and offset), then this methodology should be used.
Let's break this change into two changes. The 00 is replaced by 44 and the 11*2 is deleted. After the first change, the index remains the same because the deletion did not affect the anchor cell's index. After the second change, we should resort to how the code works when this situation arises: set the index to the start of the deletion, which in this case is the third index. 6 would be the new anchor cell index. The offset should stay the same in case a change like the RTFX-bug occurs where the "height" of the deleted cells equal the "height" of the added cells. If this is not the case, this does not produce any bad side effects as the entire viewport is being re-rendered anyway.

// Possibility 2: index is deleted, but addedSize = index; viewport is completely deleted
Before: 00|11*2|33
Delete: 00|11*2
Added:  44|4454
Result: 44 4454 33

If only part of the viewport is deleted, we could use the same reasoning about using an undeleted currently-rendered cell as the new anchor cell. If that is not the present situation (as shown above), we'd use the same workaround as above.

// Possibility 3: index is deleted, but addedSize > index; viewport is completely deleted
Before: 00|11*29|33
Delete: 00|11*29|33
Added:  44|44544|44
Result: 44 44544 44

This situation would never allow the "use a different currently-displayed cell as the new anchor cell" approach. Thus, it would follow the same logic as the first and second possibilities.

As a side note, I think there should be a way to get the navigator's current position, so that one could use that to determine what to display after some change occurs (e.g. scroll from the current position or display an item 10 indexes higher, etc.) via VirtualFlow's show method. In order to support that functionality, there would need to be more view-related API methods that returned a TargetPosition of a specific cell.

[JordanMartinez]

In this new approach, the 2nd branch of the IF-block does 2 things before defaulting to the behavior before this PR:

• use the first or last visible cell as the new anchor cell if they weren't deleted
• don't change anything if an exact replacement occurs (removed == added)

[JordanMartinez]

Testing this out with synth3's example still scrolled a bit. So, I think this problem should be studied a bit more before merging.

[JordanMartinez]

The problem I encountered above occurred when I would scroll to the bottom of the area before clicking the reset button. I expected the last cell to be rendered at the bottom of the viewport. However, the viewport would render the second to last cell on the bottom and fill up the rest of the viewport from there.

After searching for quite a while, I narrowed down what was causing the problem. Turns out this current PR works just fine when the VirtualFlow is not wrapped in a VirtualizedScrollPane. It seems that, somewhere in the layout process, the length of the viewport is changed due to the scroll bars disappearing temporarily. Thus, distanceFromSky returns a positive value since the viewport's length is greater than the last cell's maxY value when they should be equal. Thinking that there is still space to fill, the viewport will render additional content from the "ground" up, causing the actual anchor cell to be pushed down in the viewport, causing the last cell to be pushed out of the viewport.

[JordanMartinez]

I decided not to use the first/last visible cell as the new anchor cell should the current one be deleted. When transforming the targetPosition across multiple modifications, the "first" and "last" visible cells aren't always the first/last ones through multiple transformations if they get deleted. Addressing this to account for that possibility is outside of the scope of this PR, which only seeks to fix #39. So, I'll not address it now.