On Fri, 23 Jun 2023 15:45:21 GMT, Andy Goryachev <ango...@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
