On Fri, 22 Sep 2023 18:33:49 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>>> > yeah, I also don't quite understand why we have the rounding (I mean,
>>> > floor'ing) here. I would vote to remove the typecast as the OP proposes.
>>>
>>> Phil has some concerns that will need to be looked into.
>>
>> To follow-on, it seems that there are a few options.
>>
>> 1. Pass back the unmodified value. The concern that Phil raised is that this
>> could lead to floating-point error. Of course the current code that
>> truncates could also lead to errors. If the value ends up as, say, 10.9999
>> inches, we would want to return 11 rather than 10 even if we did want to
>> limit it to integers.
>> 2. Round the value to whole integers. This doesn't really solve the problem
>> that the PR is trying to solve
>> 3. Round to some number of decimal places, e.g., round to the nearest 0.01
>> or 0.001 inches.
>> 4. Something else.
>>
>> Option 3 seems like it might be a reasonable solution, but I'd want Phil to
>> weigh in.
>
> what kind of FP error could option 1 lead to?
>
> I think rounding belongs to the UI, to the panel which displays the sizes
> with precision given by the requirements. So this method, I think, should
> return the unmodified value.
>
> Looking at AWT Paper, I see no rounding there. Where did it come from to FX?
>
> PS. my other comments are indeed unrelated to this PR. sorry.
Some observations on the paper class:
/**
* Specifies the North American legal size, 8.5 inches by 14 inches.
*/
public static final Paper LEGAL = new Paper("Legal", 8.4, 14, INCH);
The comment and actual size contradict each other. 8.4 inches would not result
in an integer number of points (604.8) and looking up the legal paper size
confirms it should be 8.5 inches
/**
* Specifies the Monarch envelope size, 3.87 inch by 7.5 inch.
*/
public static final Paper MONARCH_ENVELOPE = new Paper("Monarch Envelope",
3.87, 7.5, INCH);
This seems incorrect, the size is 3-7/8” x 7-1/2”, which would be `3.875` x
`7.5` -- the difference in this case is small enough that it will be rounded to
the same value in points.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334926281
