FXMisc / Flowless

Efficient VirtualFlow for JavaFX
BSD 2-Clause "Simplified" License
185 stars 38 forks source link

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

Closed JordanMartinez closed 7 years ago

JordanMartinez commented 7 years ago

Coming from TomasMikula/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.

JordanMartinez commented 7 years ago

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

JordanMartinez commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.