Margin value isn't always saved as entered

VERIFIED FIXED in mozilla1.9alpha1

Status

()

P1
major
VERIFIED FIXED
16 years ago
12 years ago

People

(Reporter: kazhik, Assigned: masayuki)

Tracking

(Blocks: 1 bug, {intl, verified1.8.1})

Trunk
mozilla1.9alpha1
intl, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

16 years ago
Margin value in Page Setup dialog isn't always saved as entered.

Steps to reproduce:
(1) Enter "158" as right margin.
(2) Click OK button and open Page Setup dialog again.

Right margin is changed to "157.9".

Build: 2003010408-trunk/Win98

Updated

16 years ago
Priority: -- → P3
Target Milestone: --- → Future

Updated

16 years ago
Blocks: 125824

Comment 1

15 years ago
Is this related to bug 118954?

Comment 2

13 years ago
(In reply to comment #1)
> Is this related to bug 118954?

Yes, caused by small patch attachment 106157 [details] [diff] [review] which was not reviewed prior to check in.
Assignee: rods → printing
Blocks: 118954
OS: Windows 98 → All
Priority: P3 → --
QA Contact: sujay
Hardware: PC → All
taking this.

I think that this bug is very silly for i18n. We should fix this before next release. I'll attach the patch.
Assignee: printing → masayuki
Severity: normal → major
Flags: blocking1.8.1?
Keywords: intl
Priority: -- → P1
Target Milestone: Future → mozilla1.8.1beta1
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Created attachment 225132 [details] [diff] [review]
Patch rv1.0

I think that we should save the margin value by inputted value, directly. Because if we convert the inputted value to another unit(i.e., inch or twip), we have a problem of rounding. For escaping from it, we can use the way.

But if we keep to use current margin pref names, we cannot keep compatibility. Therefore, we should use new pref name for the saving. I named these prefs:
"print_inputted_margin_top", "print_inputted_margin_bottom", "print_inputted_margin_left", "print_inputted_margin_right"

If these new prefs doesn't exist in pref, we should use old pref for loading.

I have two question:

1. The margin attributes of nsIPrintSettings are changed the meaning. But the interface is doesn't changed. Can it go to 1.8 branch?

2. nsIDeviceContextSpecXp::GetTopMargin, nsIDeviceContextSpecXp::GetBottomMargin, nsIDeviceContextSpecXp::GetLeftMargin, nsIDeviceContextSpecXp::GetRightMargin are changed the meaning by my patch. But these methods are not used any codes. Is the meaning changing in nsIDeviceContextSpecXp allowed?
Attachment #225132 - Flags: superreview?(roc)
Attachment #225132 - Flags: review?(roc)
Attachment #225132 - Flags: approval-branch-1.8.1?(roc)

Comment 5

13 years ago
Not going to block 1.8.1 for this.
Flags: blocking1.8.1? → blocking1.8.1-
This isn't really intl, and it's not very serious. I think we should just fix it on trunk only.
Actually, embedders use this interface so probably we shouldn't change it this way even on trunk.

Why can't we keep these values in inches and get the rounding right? They are doubles, so if we round to say six decimal places for display only, we should have no trouble converting from millimetres to inches and then back to millimetres for display.
(In reply to comment #7)
> Why can't we keep these values in inches and get the rounding right? They are
> doubles, so if we round to say six decimal places for display only, we should
> have no trouble converting from millimetres to inches and then back to
> millimetres for display.

Ah, right. It's strange.

If the user inputs "15.0"mm in the page setup dialog, the pref saves as "0.590277791023254"in. 0.590277791023254in * 25.4 = 15mm. It is right.
But actual result is 14.9mm. 0.59027779in * 25.4 = 14.993055866mm.
I think that the double value is round to float before it. I'll create another approach patch.
(In reply to comment #8)
> 0.590277791023254in * 25.4 = 15mm. It is right.

Sorry, this is wrong. I recalculated by direct inputting, that returns "14.9930558919906516".
Created attachment 226129 [details] [diff] [review]
Patch rv2.0

Thank you, roc. This is simple bug on page setup dialog.
I'm changing reviewers, thanks.
Attachment #225132 - Attachment is obsolete: true
Attachment #226129 - Flags: review?(mconnor)
Attachment #225132 - Flags: superreview?(roc)
Attachment #225132 - Flags: review?(roc)
Attachment #225132 - Flags: approval-branch-1.8.1?(roc)
(Assignee)

Updated

13 years ago
Attachment #226129 - Flags: review?(neil)
(Assignee)

Updated

13 years ago
Attachment #226129 - Flags: approval-branch-1.8.1?(mconnor)

Comment 11

13 years ago
Comment on attachment 226129 [details] [diff] [review]
Patch rv2.0

> function getDoubleStr(val, dec)
> {
>+  val *= Math.pow(10, dec);
>+  val = Math.round(val);
>+  val /= Math.pow(10, dec);
>   var str = val.toString();
>   var inx = str.indexOf(".");
>+  if (inx == -1 && str != "")
>+    return str + ".0";
>   return str.substring(0, inx+dec+1);
> }
So, now that we're properly rounding val, we don't need to use substring to truncate the string at dec digits, right?
Also, return str + ".0"; only works when dec is 1, and so I'd say it's incorrect as a new behaviour.
(In reply to comment #11)
> So, now that we're properly rounding val, we don't need to use substring to
> truncate the string at dec digits, right?
> Also, return str + ".0"; only works when dec is 1, and so I'd say it's
> incorrect as a new behaviour.
> 

right. I'll attach new patch.
Created attachment 226176 [details] [diff] [review]
Patch rv2.1
Attachment #226129 - Attachment is obsolete: true
Attachment #226176 - Flags: review?(mconnor)
Attachment #226176 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #226129 - Flags: review?(neil)
Attachment #226129 - Flags: review?(mconnor)
Attachment #226129 - Flags: approval-branch-1.8.1?(mconnor)
(Assignee)

Updated

13 years ago
Attachment #226176 - Flags: superreview?(neil)
Attachment #226176 - Flags: review?(neil)

Updated

13 years ago
Attachment #226176 - Flags: approval-branch-1.8.1?(mconnor)

Comment 14

13 years ago
Comment on attachment 226176 [details] [diff] [review]
Patch rv2.1

OK, so Mike Shaver points out that print_margin_top.toFixed(1) does exactly the same as getDoubleStr(print_margin_top, 1) was supposed to do (and apparently two years earlier to boot, so no excuse for rods not to know!)
Attachment #226176 - Flags: superreview?(neil)
Attachment #226176 - Flags: superreview-
Attachment #226176 - Flags: review?(neil)
Created attachment 226820 [details] [diff] [review]
Patch rv3.0

Neil:

|getDoubleStr| is still used by scaling. But scaling value does not have a fraction usually. But I think that we should not use |toFixed(3)| for it. Is it OK?
Attachment #226176 - Attachment is obsolete: true
Attachment #226820 - Flags: superreview?(neil)
Attachment #226820 - Flags: review?(neil)
Attachment #226176 - Flags: review?(mconnor)

Comment 16

13 years ago
(In reply to comment #15)
>|getDoubleStr| is still used by scaling. But scaling value does not have a
>fraction usually. But I think that we should not use |toFixed(3)| for it.
WritePrefDouble seems to round the value to the nearest 0.01 so after multiplication by 100 we should always get an integer percentage.

Comment 17

13 years ago
Comment on attachment 226820 [details] [diff] [review]
Patch rv3.0

>-  gDialog.topInput.value    = getDoubleStr(print_margin_top, 1);
>-  gDialog.bottomInput.value = getDoubleStr(print_margin_bottom, 1);
>-  gDialog.leftInput.value   = getDoubleStr(print_margin_left, 1);
>-  gDialog.rightInput.value  = getDoubleStr(print_margin_right, 1);
>+  gDialog.topInput.value    = print_margin_top.toFixed(1);
>+  gDialog.bottomInput.value = print_margin_bottom.toFixed(1);
>+  gDialog.leftInput.value   = print_margin_left.toFixed(1);
>+  gDialog.rightInput.value  = print_margin_right.toFixed(1);
r=me on these changes, but we still need to fix scaling.
Attachment #226820 - Flags: superreview?(neil)
Attachment #226820 - Flags: superreview-
Attachment #226820 - Flags: review?(neil)
Attachment #226820 - Flags: review+
Created attachment 226922 [details] [diff] [review]
Patch rv3.1

>>|getDoubleStr| is still used by scaling. But scaling value does not have a
>>fraction usually. But I think that we should not use |toFixed(3)| for it.
> WritePrefDouble seems to round the value to the nearest 0.01 so after
> multiplication by 100 we should always get an integer percentage.

You're right. I confirmed it. Please sr it.
# If you approve it, I'll request additional review to mconnor for toolkit part.
Attachment #226820 - Attachment is obsolete: true
Attachment #226922 - Flags: superreview?(neil)
Attachment #226922 - Flags: review+

Comment 19

13 years ago
Comment on attachment 226922 [details] [diff] [review]
Patch rv3.1

>+  gDialog.scalingInput.value = (gPrintSettings.scaling * 100).toString();
This doesn't quite work right, I'm seeing rounding errors from the division (try 44% for example). sr=me if you change this to use toFixed(0) instead.
Attachment #226922 - Flags: superreview?(neil) → superreview+
Created attachment 227074 [details] [diff] [review]
Patch rv3.2

Thank you, Neil for your review.
Attachment #226922 - Attachment is obsolete: true
Attachment #227074 - Flags: superreview+
Attachment #227074 - Flags: review+
Comment on attachment 227074 [details] [diff] [review]
Patch rv3.2

Mike:

Please check the toolkit part.
Attachment #227074 - Flags: review?(mconnor)
Comment on attachment 227074 [details] [diff] [review]
Patch rv3.2

Wow, I didn't know that Neil is a reviewer of toolkit. This patch was already cleared necessary reviews.
I'll check-in this to trunk.
Attachment #227074 - Flags: review?(mconnor)
checked-in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8.1beta1 → mozilla1.9alpha
Comment on attachment 227074 [details] [diff] [review]
Patch rv3.2

This patch has low risk. And this patch makes happy many meter unit users.
Attachment #227074 - Flags: approval1.8.1?
Comment on attachment 227074 [details] [diff] [review]
Patch rv3.2

a=mconnor on behalf of drivers for 1.8 branch checkin
Attachment #227074 - Flags: approval1.8.1? → approval1.8.1+
checked-in to 1.8 branch too.
Keywords: fixed1.8.1
-> v. on trunk
Status: RESOLVED → VERIFIED
-> v. on 1.8 branch
Keywords: fixed1.8.1 → verified1.8.1
You need to log in before you can comment on or make changes to this bug.