RFR: 8299753: Tree/TableView: Column Resizing With Fractional Scale [v2]

[John Hendrikx]

On Fri, 23 Jun 2023 15:45:21 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Modified the resize algorithm to work well with fractional scale, thanks for deeper understanding of the problem thanks to  @hjohn and @mstr2 .
>> 
>> It is important to note that even though the constraints are given by the user in unsnapped coordinates, they are converted to snapped values, since the snapped values correspond to the actual pixels on the display.  This means the tests that validate honoring constraints should, in all the cases where (scale != 1.0), assume possibly error not exceeding (1.0 / scale) (I think).
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comments

I only reviewed this partially.

My observation is that this algorithm seems unable to provide a proper user resizing experience as it seems to discard important information it would need to do so.  The `ResizeHelper` is short-lived, and created/recreated many times during a resize operation. I'm not even sure why it is being instantiated at all (it literally is created and discarded in a single method).  The `SMALL_DELTA` constant that changes behavior on how large a "drag" was registered is a red flag; this shouldn't matter.  The sizes should always be based on what they were initially (at the start of the drag), and where the cursor is currently, regardless of what path the cursor took to get there (ie. there should be no memory effects, the algorithm should only need the initial sizes + current position).

I'd expect:

1. User starts a resize
2. All sizes and positions are stored -- this may be a good time to create something like the `ResizeHelper`.  This `ResizeHelper` should never modify the initial sizes, as they're needed for each new calculation until the drag ends.
3. User drags the cursor all over the place
4. Depending on where the cursor is + the initial stored values, a new set of column sizes is calculated and displayed; the algorithm should not have "memory" effects of non-final column sizes of positions where the cursor was before
5. When the drag ends, the final position is exactly what would have happened if the drag was a single instant move, in other words, moving the cursor from `0 -> 100` should be the same as if it was moved from `0 -> -10 -> 90 -> 200 -> 100`

I haven't checked completely how the column resizing works, but I don't see how this could possible be a good algorithm currently. So I'm left wondering what value this change then brings as I think these changes would more than likely all be discarded once the UX is implemented correctly.

A direct JUnit test on this complicated code that verifies all the edge cases would be something I'd like to see added as a minimum, but looking at the code, I don't see this being a good algorithm at all...

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 58:

> 56:                         ConstrainedColumnResize.ResizeMode mode) {
> 57:         this.rf = rf;
> 58:         this.snap = (rf.getTableControl().isSnapToPixel() ? rf.getTableControl() : null);

Obtaining the `Region` here is a bit hacky, this may be out of scope, but I would say `snap` should be a boolean, and the `snapXXX` helper methods in this class should call `ScaledMath`.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 77:

> 75:                 double cmin = snapCeil(c.getMinWidth()); // always honor min width!
> 76:                 double cmax = snapFloor(c.getMaxWidth()); // TableColumnBase.doSetWidth() clamps to (min,max) range
> 77:                 min[i] = cmin;

Looking at `doSetWidth` I see that it clamps using unsnapped values, so the column can still be given an unsnapped size.  When scale is 1.0, and the column for example has its min/max width set to 20.1 and 20.9, then snapCeil is 21 and snapFloor is 20 (so maximum is less than minimum, which may already be a bit dubious).  When `doSetWidth` is called it will be clamped, resulting in `20.1` or `20.9`.

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ResizeHelper.java line 125:

> 123:     private void distribute(double delta, double[] desired) {
> 124:         double threshold = snapRound(SMALL_DELTA);
> 125:         if (Math.abs(delta) > threshold) {

The threshold is pretty arbitrary, I don't think it benefits from snap rounding here.  I'd be more inclined to eliminate the difference between these two functions; I would expect that if a user drags a column resizer for 100 pixels, that it should make no difference whether that was a slow drag or a fast one, and the end visuals and column sizes should be exactly the same regardless.

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

PR Review: https://git.openjdk.org/jfx/pull/1156#pullrequestreview-1509558936
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1249477980
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1249466719
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1249493706