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

[Kevin Rushforth]

On Wed, 15 Jan 2025 15:41:23 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 .
>> 
>> 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).
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 39 commits:
> 
>  - Merge branch 'master' into 8299753.resize
>  - Merge remote-tracking branch 'origin/master' into 8299753.resize
>  - Merge remote-tracking branch 'origin/master' into 8299753.resize
>  - in case of hitting min max
>  - Merge branch 'master' into 8299753.resize
>  - Merge branch 'master' into 8299753.resize
>  - Merge remote-tracking branch 'origin/master' into 8299753.resize
>  - Merge branch 'master' into 8299753.resize
>  - Merge remote-tracking branch 'origin/master' into 8299753.resize
>  - Merge remote-tracking branch 'origin/master' into 8299753.resize
>  - ... and 29 more: https://git.openjdk.org/jfx/compare/a95151e1...f365f3dd

I reread the discussion on this, and have a few thoughts.

1. [JDK-8299753](https://bugs.openjdk.org/browse/JDK-8299753) is a pretty noticeable bug when dealing with non-integer render scales (especially 125%) in the default snap-to-pixel case.

2. I agree with @hjohn and @mstr2 that the existing `TableView` resize-on-mouse-drag algorithm is flawed. The way that successive delta values are accumulated, which leads to different results for slow vs fast mouse moves, highlights this problem (SMALL_DELTA is a partial workaround, but it's only needed in the first place because the algorithm is flawed). I haven't looked closely enough to see whether it would be possible to fix this without public API changes, but I can imagine that it might not be. Either way it would be a large effort to fix the algorithm with a lot of testing needed. Long term, it might be the right thing to do, but it's out of scope for this bug fix.

Given that rewriting the algorithm is out of scope here, the question becomes whether this bug is worth the effort to fix (I think it is) and whether this particular change is sound, given the existing algorithm. The main thing I would want to ensure is that we don't regress in the case of an integer screen scale.

My testing of it so far looks good, but I want to review the code changes to satisfy myself that the risk of regression is small. I'm still a bit skeptical of the need for the (not-public) snapInnerSpace method, but I'll take a closer look.

We'll need to testing with snap-to-pixel disabled as well (I spotted what I think is a bug in that case, but that was just something I happened to notice -- I still  have not done a thorough review of the code).

If we are going to get this in, doing so early in the jfx25 release would be good so it gets more testing time.

modules/javafx.controls/src/main/java/javafx/scene/control/ResizeFeaturesBase.java line 104:

> 102:       if (c.isSnapToPixel()) {
> 103:           double min = c.snapSizeX(col.getMinWidth());
> 104:           double max = RegionHelper.snapInnerSpaceX(c, col.getMaxWidth());

I'm still not convinced that min and max should be snapped differently (floor vs ceiling).

modules/javafx.controls/src/main/java/javafx/scene/control/ResizeFeaturesBase.java line 118:

> 116:       }
> 117:       // can set width directly because all constraints have been checked
> 118:       col.setWidth(width);

The constraints have been checked only in the default snap-to-pixel case. This will bypass clamping if snap-to-pixel is false. I recommend moving the `setWidth` call into the if statement, and calling `doSetWidth` in an `else` block.

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

PR Review: https://git.openjdk.org/jfx/pull/1156#pullrequestreview-2573442011
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1929153111
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1929153564