Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 17 Oct 2023 09:23:16 GMT, Florian Kirmaier wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > It would be nice if something could be decided here. > > Another option would be, to rename the current method to something like > getWidthInPoints() which then can be used internally. But the old - now > correct methods - can be exposed to applications, returning correct values. > > But I would still prefer to just fix it, returning the correct values. And if > it turns out, at some part we need it in points, then we just have to convert > it to this location. I still don't see how the current state doesn't lead to > various incorrect behaviors. @FlorianKirmaier This PR is ready for you to `/integrate` - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1830847681
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
On Tue, 31 Oct 2023 11:46:57 GMT, Florian Kirmaier wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8316518 > small change based on codereview Sorry about the very lengthy delay here. I've gone back over what little I have left from 11 years ago and I can't find a specific reason for the rounding. My best guess is that it was about "UI", meaning ideally a user would not see values like 611.9985349 If that becomes a problem we can revert this i fwe can't handle it elsewhere. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1244#pullrequestreview-1745523007
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
On Tue, 31 Oct 2023 11:46:57 GMT, Florian Kirmaier wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8316518 > small change based on codereview (testing jcheck fix) - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1789234370
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 22:26:31 GMT, Phil Race wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > FX uses doubles as a rule and it doesn't preclude that the actual values may > have no fractional part. > NA paper is measured in INCHes and if we have some FP error and end up with > 792.0001 pts then the rounding would help. > So I am not sure about removing it completely. > What exact problem does it cause you ? Paper matching to A4 etc should be > working SFAIK. > Perhaps we can tweak the MM case only. Not rounding to whole values but a > close FP representation. please wait before integrating this until @prrace has a chance to take another look at it. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1787960540
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
On Tue, 31 Oct 2023 11:46:57 GMT, Florian Kirmaier wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8316518 > small change based on codereview LGTM - Marked as reviewed by jhendrikx (Committer). PR Review: https://git.openjdk.org/jfx/pull/1244#pullrequestreview-1707154735
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
On Tue, 31 Oct 2023 11:46:57 GMT, Florian Kirmaier wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8316518 > small change based on codereview looks good to me I think the fix is the right one. - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1244#pullrequestreview-1706953801 PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1787716551
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
On Tue, 31 Oct 2023 11:46:57 GMT, Florian Kirmaier wrote: >> The Method javafx.print.getWidth and getHeight returns a double of points. >> But the values are always rounded to full integers. This causes especially >> problems, >> with mm based Papers. >> >> I asked in the mailing list, with the conclusion (me and John Hendrix) that >> is best to just fix this behavior by removing the rounding. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8316518 > small change based on codereview Marked as reviewed by mstrauss (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1244#pullrequestreview-1706879548
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
On Thu, 26 Oct 2023 01:51:52 GMT, Michael Strauß wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8316518
>> small change based on codereview
>
> modules/javafx.graphics/src/main/java/javafx/print/Paper.java line 85:
>
>> 83: case MM: return (dim * 72) / 25.4;
>> 84: }
>> 85: return dim;
>
> Minor: you can use an enhanced `switch` expression here, which also ensures
> exhaustiveness:
>
> return switch (units) {
> case POINT -> dim;
> case INCH -> dim * 72;
> case MM -> (dim * 72) / 25.4;
> };
done
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1377475424
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors. [v2]
> The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: JDK-8316518 small change based on codereview - Changes: - all: https://git.openjdk.org/jfx/pull/1244/files - new: https://git.openjdk.org/jfx/pull/1244/files/e84c14a7..cc3ba365 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1244=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1244=00-01 Stats: 6 lines in 1 file changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1244.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1244/head:pull/1244 PR: https://git.openjdk.org/jfx/pull/1244
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier
wrote:
> The Method javafx.print.getWidth and getHeight returns a double of points.
> But the values are always rounded to full integers. This causes especially
> problems,
> with mm based Papers.
>
> I asked in the mailing list, with the conclusion (me and John Hendrix) that
> is best to just fix this behavior by removing the rounding.
modules/javafx.graphics/src/main/java/javafx/print/Paper.java line 85:
> 83: case MM: return (dim * 72) / 25.4;
> 84: }
> 85: return dim;
Minor: you can use an enhanced `switch` expression here, which also ensures
exhaustiveness:
return switch (units) {
case POINT -> dim;
case INCH -> dim * 72;
case MM -> (dim * 72) / 25.4;
};
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1372483292
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. I agree with John. I can't think of the use case for the rounding. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1766557869
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. It would be nice if something could be decided here. Another option would be, to rename the current method to something like getWidthInPoints() which then can be used internally. But the old - now correct methods - can be exposed to applications, returning correct values. But I would still prefer to just fix it, returning the correct values. And if it turns out, at some part we need it in points, then we just have to convert it to this location. I still don't see how the current state doesn't lead to various incorrect behaviors. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1766021174
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. Relating to the wrong Paper sizes - I have this strange suspicion, that sizes were wrong - to get the right roundings at some point in time. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1740938751
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. I'm also open to making the original width/height and unit accessible. I think, the UI which displays the size, must do the rounding itself. But it's important that these values are correct because they are. used to layout the Content, which should be rendered. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1735041109
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. I did a fix for the incorrect legal paper size here: https://github.com/openjdk/jfx/pull/1248 - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1732394588
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Sat, 23 Sep 2023 06:02:14 GMT, John Hendrikx wrote:
>> About the rounding; all these values are fixed values and could have been
>> entered in points as constants directly.
>>
>> Also realize that all papers measured in inches can be expressed exactly in
>> points (after the above errors are fixed) so no rounding is needed for those
>> at all; the same is not true for the papers specified in millimeters.
>>
>> Since the papers in inches never should need any rounding, the rounding can
>> therefore be removed for those. If there is a worry that the multiplication
>> (* 72) may cause floating point errors, then I suggest to specify all papers
>> in points immediately, like this:
>>
>> new Paper("Letter", 8.5 * 72, 11 * 72, POINT);
>>
>> As for the papers specified in millimeters, these can't be expressed in
>> whole points, as a millimeter is equivalent to `2,83465` points. Rounding
>> them to points will require some guess work to find out what the actual size
>> in millimeters was, for example, when it says 3 points it must have been 1
>> mm, while 1, 2, 4 or 5 points has no direct mm equivalent.
>
> It is also very odd that `Paper` would expose the name (like `A4`), but not
> the unit (`MM`) and the exact sizes for those. Representing these in a UI
> therefore becomes terribly complicated; if I want to show the user, I can
> show them the name, but then when I want to display its size, I have to show
> it in points (a useless measurement, for users, for both papers specific in
> inches and mm).
Typically paper sizes are displayed in whatever units are appropriate for the
locale. On my Mac all paper sizes are displayed in whole millimeters if the
locale is United Kingdom and inches (to 1/100 of an inch) if the locale is
United States.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1335047374
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Sat, 23 Sep 2023 05:39:12 GMT, John Hendrikx wrote:
>> 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.
About the rounding; all these values are fixed values and could have been
entered in points as constants directly.
Also realize that all papers measured in inches can be expressed exactly in
points (after the above errors are fixed) so no rounding is needed for those at
all; the same is not true for the papers specified in millimeters.
Since the papers in inches never should need any rounding, the rounding can
therefore be removed for those. If there is a worry that the multiplication (*
72) may cause floating point errors, then I suggest to specify all papers in
points immediately, like this:
new Paper("Letter", 8.5 * 72, 11 * 72, POINT);
As for the papers specified in millimeters, these can't be expressed in whole
points, as a millimeter is equivalent to `2,83465` points. Rounding them to
points will require some guess work to find out what the actual size in
millimeters was, for example, when it says 3 points it must have been 1 mm,
while 1, 2, 4 or 5 points has no direct mm equivalent.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334928344
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Sat, 23 Sep 2023 06:00:40 GMT, John Hendrikx wrote:
>> 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.
>
> About the rounding; all these values are fixed values and could have been
> entered in points as constants directly.
>
> Also realize that all papers measured in inches can be expressed exactly in
> points (after the above errors are fixed) so no rounding is needed for those
> at all; the same is not true for the papers specified in millimeters.
>
> Since the papers in inches never should need any rounding, the rounding can
> therefore be removed for those. If there is a worry that the multiplication
> (* 72) may cause floating point errors, then I suggest to specify all papers
> in points immediately, like this:
>
> new Paper("Letter", 8.5 * 72, 11 * 72, POINT);
>
> As for the papers specified in millimeters, these can't be expressed in whole
> points, as a millimeter is equivalent to `2,83465` points. Rounding them to
> points will require some guess work to find out what the actual size in
> millimeters was, for example, when it says 3 points it must have been 1 mm,
> while 1, 2, 4 or 5 points has no direct mm equivalent.
It is also very odd that `Paper` would expose the name (like `A4`), but not the
unit (`MM`) and the exact sizes for those. Representing these in a UI
therefore becomes terribly complicated; if I want to show the user, I can show
them the name, but then when I want to display its size, I have to show it in
points (a useless measurement, for users, for both papers specific in inches
and mm).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334928528
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Fri, 22 Sep 2023 18:33:49 GMT, Andy Goryachev 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.
>> 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
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Fri, 22 Sep 2023 18:21:56 GMT, Kevin Rushforth 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.
>>
>>> But there is more:
>>>
>>> 1. why in the world the constructor is not public? what if I need to print
>>> a Commercial 8-5/8 envelope?
>>
>> That's the wrong question to ask, and this PR is the wrong place to ask it.
>> If I could reformulate your question, I might rather state it as "would it
>> be a useful enhancement to allow Paper to be customizable?". Maybe (or maybe
>> not), but as with any other new feature, it needs proper motivation, etc.
>>
>>> 2. what is going on in hashCode??
>>>
>>> ```
>>> public final int hashCode() {
>>> return (int)width+((int)height<<16)+units.hashCode();
>>> }
>>> ```
>>
>> This is also unrelated to this PR. It may or may not be a great `hashCode`
>> implementation, but It meets the contract of hasCode w.r.t. equals. And if
>> it didn't, it would need a separate bug.
>
>> > 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. 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.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334708237
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Fri, 22 Sep 2023 18:07:50 GMT, Kevin Rushforth 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. 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. - PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334698234
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Fri, 22 Sep 2023 17:34:45 GMT, Andy Goryachev 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.
> But there is more:
>
> 1. why in the world the constructor is not public? what if I need to print a
> Commercial 8-5/8 envelope?
That's the wrong question to ask, and this PR is the wrong place to ask it. If
I could reformulate your question, I might rather state it as "would it be a
useful enhancement to allow Paper to be customizable?". Maybe (or maybe not),
but as with any other new feature, it needs proper motivation, etc.
> 2. what is going on in hashCode??
>
> ```
> public final int hashCode() {
> return (int)width+((int)height<<16)+units.hashCode();
> }
> ```
This is also unrelated to this PR. It may or may not be a great `hashCode`
implementation, but It meets the contract of hasCode w.r.t. equals. And if it
didn't, it would need a separate bug.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334687579
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier
wrote:
> The Method javafx.print.getWidth and getHeight returns a double of points.
> But the values are always rounded to full integers. This causes especially
> problems,
> with mm based Papers.
>
> I asked in the mailing list, with the conclusion (me and John Hendrix) that
> is best to just fix this behavior by removing the rounding.
modules/javafx.graphics/src/main/java/javafx/print/Paper.java line 83:
> 81: case POINT : return dim;
> 82: case INCH : return dim * 72;
> 83: case MM: return (dim * 72) / 25.4;
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.
But there is more:
1. why in the world the constructor is not public? what if I need to print a
Commercial 8-5/8 envelope?
see https://www.papersizes.org/us-envelope-sizes.htm
2. what is going on in hashCode??
public final int hashCode() {
return (int)width+((int)height<<16)+units.hashCode();
}
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1244#discussion_r1334659707
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. Many of the Papers in the Paper Class are measured in MM, and the Paper Class simply returns wrong values. A rounding like `.1` should cause less problems than `.465` (A4). A4 Paper simply returns wrong values. If the developer uses these values, then the computations are wrong. By using the correct values, we get visible more correct results. In the mailing list, I also suggested 2 other solutions: **Solution2:** We could add new methods getExactWidth() / getExactHeight() , which returns the exact value without a rounding problem anywhere noticeable. **Solution3:** We could provide an API to access the width/height based on a given Unit. like: getWidth(Unit.MM). We already have implemented this, in our application code, so it wouldn’t be much effort. But it would require moving com.sun.javafx.print.Units back to javafx.print.Paper.Units. (basically reverting JDK-8093699) I could also implement one of these solutions, if that's preferred. But i still don't see, how this rounding doesn't cause many unknown problems. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1729039960
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. FX uses doubles as a rule and it doesn't preclude that the actual values may have no fractional part. NA paper is measured in INCHes and if we have some FP error and end up with 792.0001 pts then the rounding would help. So I am not sure about removing it completely. What exact problem does it cause you ? Paper matching to A4 etc should be working SFAIK. Perhaps we can tweak the MM case only. Not rounding to whole values but a close FP representation. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1726623600
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. @prrace needs to review this change. - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1725381832
Re: RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
On Tue, 19 Sep 2023 11:56:39 GMT, Florian Kirmaier wrote: > The Method javafx.print.getWidth and getHeight returns a double of points. > But the values are always rounded to full integers. This causes especially > problems, > with mm based Papers. > > I asked in the mailing list, with the conclusion (me and John Hendrix) that > is best to just fix this behavior by removing the rounding. According to my test - I'm still able to print. I also don't see anything in the code, which implies that the rounding was necessary. Is there something specific that should be tested on changes in printing? - PR Comment: https://git.openjdk.org/jfx/pull/1244#issuecomment-1725363048
RFR: 8316518 javafx.print.Paper getWidth / getHeight rounds values, causing errors.
The Method javafx.print.getWidth and getHeight returns a double of points. But the values are always rounded to full integers. This causes especially problems, with mm based Papers. I asked in the mailing list, with the conclusion (me and John Hendrix) that is best to just fix this behavior by removing the rounding. - Commit messages: - JDK-8316518 Changes: https://git.openjdk.org/jfx/pull/1244/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1244=00 Issue: https://bugs.openjdk.org/browse/JDK-8316518 Stats: 10 lines in 2 files changed: 1 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1244.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1244/head:pull/1244 PR: https://git.openjdk.org/jfx/pull/1244
