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

[Andy Goryachev]

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

>> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review comments
>
> 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.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1253385294
PR Review Comment: https://git.openjdk.org/jfx/pull/1156#discussion_r1253382973