On Sun, 2 Jul 2023 12:11:56 GMT, John Hendrikx <[email protected]> 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
