RFR: 8299753: Tree/TableView: Column Resizing With Fractional Scale

[Andy Goryachev]

On Sun, 2 Jul 2023 12:53:05 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

> 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. 



> 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). 

convenience - it does have some data, and it's easier to keep code along with the data.


> 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. 

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.

>> > 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...

Thank you for a detailed write up, @hjohn .

One thing to note: the new code is a modification of the earlier work on constrained column resize policies, that logic is unaffected.  The only difference is to recognize the fact that in an over-constrained situation sone constraints must be relaxed, and that eliminates multiple passes.

Another consideration is that we are not re-writing Tree/TableView skin column resizing code.  The fact that the current public APIs do not provide us with the initial condition and instead invoke the policy resize methods giving small deltas is a limitation this PR is not going to address.

> 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`.

Here i would disagree.  I specifically do not want to replicate the snap code in the Region, since the Region publishes its snapping API.  If there is a change in the snapping APIs in the Region, that change would need to be replicated again here, which I think is a bad idea.

> 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`.

For some reason, I've decided to leave that as is, but since you pointed it out, I think it ought to be fixed. 

I am going to implement proper snapping logic in ResizeFeaturesBase.setColumnWidth() using private APIs, with the intent to eventually convert them to public APIs with https://bugs.openjdk.org/browse/JDK-8311527

> 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.

theoretically, may be.  I could not achieve it with the algorithm that distributes space for large deltas, as it breaks down with small deltas, see my previous comment.

snapRound(SMALL_DELTA) is indeed unnecessary.

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

PR Comment: https://git.openjdk.org/jfx/pull/1156#issuecomment-1622127738
PR Comment: https://git.openjdk.org/jfx/pull/1156#issuecomment-1622420462
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1253385294
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1253382973
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1253390837