palexdev / VirtualizedFX

GNU Lesser General Public License v3.0
46 stars 4 forks source link

Issues on cell size changes #11

Closed Alexander-Ploskin closed 4 months ago

Alexander-Ploskin commented 1 year ago

Changes:

  1. Fix exception, when after cell size changed, columns range shifts (for example, [2, 3] -> [3, 4]). On that line cells collection can't contain rIndex, because cells, which corresponds to any columns from the columns range was removed earlier.

  2. setPosition(0, 0) causes a lot of problems to me (redundant frame updates). I didn't find any purpose for this behavior. I removed this line and tested in my project, and it looks fine. Could you please describe why it actually needed?

palexdev commented 1 year ago

1) Yeah that must have been a type, thanks for the fix 2) I'm reluctant on changing that code fragment. If I wrote that, there must have been a reason. I don't actually remember why exactly, but what that code does is reposition the flow at [0, 0] if the cells' size changes and the layout was updated In other words, it should be a 'better safe than sorry' solution to avoid unwanted states for edge cases situations. In fact, the same happens for VirtualTable and their paginated versions

The thing is that you are also trying to implement a rather special case. A VirtualFlow must have fixed cell sizes at all times. When you are zooming, you are practically "cheating" on this rule. And because of this rule, my way of seeing cell sizes changing is: if size has changed, then it's not safe to keep the flow at the same position it was before because anything can happen.

palexdev commented 1 year ago

What we could try to do is to add a flag to avoid that code fragment from triggering. In other words, a way to disable that system is special cases like yours, or even simpler, just override the method since you can do it

On the other hand... It would be nice to not reset the position, but at least ensure that it is valid before doing anything. In pseudocode, something like this:

        if (getWidth() != 0.0 && getHeight() != 0.0) {
            if (!manager.init()) {
                requestViewportLayout();
            } else {
                // Check Positions
                               // Ensure both vPos and hPos are within the new estimated sizes
                              // This can be done with clamping
                             // After clamping set the positions
            }
        }

Mind testing this, maybe send me a "code preview"?