On Thu, 15 Jun 2023 19:38:00 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 .
>
> Removed earlier manual tester in favor of the monkey tester.
>
> 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 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...
> > 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.
>
> please elaborate, or point to a specific problem. It is entirely possible
> that a better algorithm might exist, but it might be out of scope _at the
> moment_, as this is a follow-up for a specific issue.
It's hard to point to a specific problem when most of the algorithm used would
be unnecessary if it used the initial conditions + current resize position as
its basis for calculating the column sizes. My problem with this
implementation is that it takes what is fundamentally a very simple algorithm
(columns have sizes X,Y,Z and Y is resized 10 pixels larger, what should the
layout be?) and turns it into a frame rate dependent, mouse movement dependent
delta calculation. The initial conditions are discarded, and so a single drag
resize of 10 pixels is NOT the same as a drag resize that captured several
individual steps (1 + 2 + 3 + 4 pixels), while it really should be...
On top of that, if indeed the algorithm is flawed, as I think it is, then there
is no way to really fix it apart from some cosmetic changes. This then would
be a lot of wasted effort. As I noted, there is no JUnit test for this code as
of yet, and for such a complicated algorithm to be verified to be correct, I
think it would need one to pass review. If we're willing to forego that, then
I suppose a casual fix is in the cards, but I can't really see whether or not
it would be correct (within its fundamental limitations) without extensive
manual testing.
> > 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).
>
> This is not my experience. Specifically, the difference in behavior between
> small changes (when the users resizes the columns slowly and we get small
> deltas of 1 pixel) and large changes (e.g. when initially resizing the table,
> or the user moves the mouse quickly) are significant.
Yes, it would be needed with this algorithm as it is dependent on mouse cursor
speed and frame rate as it has no idea of what the initial positions were and
how it arrived at the current state.
> For example, I tried to use your SpaceDistributor from #1111 and it suffered
> from the same problem as bypassing the small delta path (by setting
> SMALL_DELTA=0) - when the user resizes the columns slowly, the same column
> gets the pixel and grows wider than the rest.
It would not be sufficient to just replace `ResizeHelper` with something that
uses the space distributor as the information it needs would still be lost.
When writing algorithms that resize a UI, using the current size of the
elements in your UI + a resize amount will never result in a consistent looking
resize. It always must be based on their size constraints (which can be
min/pref/max based for controls that can't be individually resized) or the
actual sizes when the action started (for splitters or columns). Once the
action is finished, only then do the new sizes become the initial sizes for the
next action -- not at every mouse event or frame that elapses.
`HBox` for example doesn't use its **current** sizes to calculate its "new"
size -- it always goes back to the basic min/pref/max sizes, then looks at the
space available and computes new sizes based on only that information. If it
used the current sizes as its base, you would notice that in certain scenario's
the final sizes (if you slowly vs quickly resized a window) would not
necessarily always be the same for the same available width -- that's exactly
what I think is happening with the column sizes; it's not deterministic and
can't be because it lacks the information to do so and hence has to resort to
`SMALL_DELTA` tricks to differentiate between "fast" and "slow" resizes.
Relying on such things (which can vary with mouse hardware, current framerate,
CPU/GPU load, amount of visible `Node`s) makes for an inconsistent resize
experience, even when the column was resized by the exact same amount of pixels
when the resize ends.
This can I think be fixed relatively easily; all it really would take is to
track when the drag starts, store the sizes, use these sizes to calculate new
sizes each time, and when the drag ends, discard the stored sizes. The
`ResizeHelper` would then only need a single much simpler method that takes an
array of sizes, the resizing mode, the space available and which column was
resized and by how much -- or if you want, you can associate the `ResizeHelper`
with the drag resize start, and use the same helper while the resize is in
progress.
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 Comment: https://git.openjdk.org/jfx/pull/1156#issuecomment-1622392841
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
