Closed Bug 558742 Opened 10 years ago Closed 8 years ago

Comma doesn't work in print range

Categories

(Toolkit :: Printing, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: vicky.sun, Assigned: stransky)

Details

Attachments

(1 file, 3 obsolete files)

Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9.1.9) Gecko/20100406 Firefox/3.5.9

It can also be reproduced FF3.5.6 on Ubuntu 9.10, FF3.6 on OpenSolaris

Steps to reproduce:
1. Launch FF and load a webpage, e.g. www.mozillazine.org
2. Go to File -> Print -> Range -> Pages, enter 1,3,5, then click Print button

Actual result: Page 1 to 5 are printed
Expected result: Page 1,3 and 5 are printed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 499640
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
Any reason why this would be the same on each platform?
Attached patch printing ranges implementation (obsolete) — Splinter Review
Comment on attachment 577277 [details] [diff] [review]
printing ranges implementation

I wonder who is the best reviewer for it, please reassign if you're the wrong one.
Attachment #577277 - Flags: review?(roc)
Comment on attachment 577277 [details] [diff] [review]
printing ranges implementation

Review of attachment 577277 [details] [diff] [review]:
-----------------------------------------------------------------

Also, rev UUID on nsIPrintSettings.idl.

::: widget/public/nsIPrintSettings.idl
@@ +333,5 @@
>    [noscript] void GetUnwriteableMarginInTwips(in nsNativeIntMarginRef aEdge);
> +  
> +  /**
> +   * Get more accurate printing ranges
> +   */

Please document this! Need to at least mention that the array is a list of (start, end) pairs where the endpoints are inclusive. Also need to specify that ranges can't overlap.
Attachment #577277 - Flags: review?(roc) → review+
Patch in commit format, new UUID for nsIPrintSettings, updated doc. Carry on r+ from roc. Is sr needed for this one?
Attachment #577277 - Attachment is obsolete: true
Attachment #577912 - Flags: review+
Comment on attachment 577912 [details] [diff] [review]
printing ranges implementation, v2

According to http://www.mozilla.org/hacking/reviewers.html is sr requested (API change, cross-module). Boris, can you look at it please?
Attachment #577912 - Flags: superreview?(bzbarsky)
Comment on attachment 577912 [details] [diff] [review]
printing ranges implementation, v2

> +      // Page ranges are couples

Maybe "pairs"?  And use "%" to make it clearer what's being tested?

>+  [noscript] void GetPageRanges(out IntegerArray aPages);

This isn't right.  The contract for that IDL is that GetPageRanges allocates an array and puts it at the provided pointer.

What you want to do is something more like what GetUnwriteableMarginInTwips does: declare a ref-to-nsTArray type and use an in parameter of that type.
Attachment #577912 - Flags: superreview?(bzbarsky) → superreview-
Thanks Boris, this one should fix it.
Attachment #579052 - Flags: superreview?(bzbarsky)
Comment on attachment 579052 [details] [diff] [review]
printing ranges implementation, v3

"length % 2 == 0" is probably clearer than using !.  With that change, sr=me.  Thanks!
Attachment #579052 - Flags: superreview?(bzbarsky) → superreview+
Attached patch v4Splinter Review
Thanks, there's the "length % 2 == 0" version here.
Attachment #577912 - Attachment is obsolete: true
Attachment #579052 - Attachment is obsolete: true
Attachment #579058 - Flags: checkin?
Keywords: checkin-needed
Assignee: nobody → stransky
Attachment #579058 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/845129faeace
Status: NEW → RESOLVED
Closed: 10 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.