Closed Bug 307404 Opened 19 years ago Closed 19 years ago

respect the paper size setting

Categories

(Core :: Printing: Output, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: chpe, Assigned: chpe)

References

Details

Attachments

(1 file, 1 obsolete file)

I'm trying to use nsIPrintSettings' PaperWidth and PaperHeight properties to set
the paper size to use for the print output. But nsPostScriptObj doesn't use
those settings; instead it uses the *paper name* setting to look the paper size
up [see
http://lxr.mozilla.org/mozilla/source/gfx/src/ps/nsPostScriptObj.cpp#302] in its
extremely limited set of only 6 paper sizes
[http://lxr.mozilla.org/mozilla/source/gfx/src/psshared/nsPaperPS.cpp#46] !

The attached patch fixes that by getting the paper width/height properties from
the nsIDeviceContextSpecPS only if the paper name wasn't found in the list. I
chose this approach because it doesn't change anything if you only set the paper
name and expect the size to be chosen automatically (which may well be what
existing apps and embedders could rely on). 

Not the same as bug 306653, but could serve as backend for that bug's frontend
changes, instead of the nsPostScriptObj.cpp change in that bug's attachment 194624 [details] [diff] [review].
Attached patch proposed fix (obsolete) — Splinter Review
Chosing reviewer based on reviewers of recent checkins to nsPostScriptObj.cpp,
hope that's correct.
Assignee: printing → chpe
Status: NEW → ASSIGNED
Attachment #195164 - Flags: review?(kherron+mozilla)
Comment on attachment 195164 [details] [diff] [review]
proposed fix

I like this, but I'd like to go further. nsPostScriptObj uses the paper name to
look up a size because it's always worked that way, not out of necessity. If
the device context spec has a call to return the paper size, it'd be cleaner to
use that every time rather than consult the paper size list. This would
probably help with bug 272670 as well.

Christian, would you be able to rework this patch?
Attachment #195164 - Flags: review?(kherron+mozilla) → review-
Attached patch reworked patchSplinter Review
I removed the paper size lookup by name and get the size from the print
settings instead. (I still get the paper name though, since it's written into
the PS output.)

I've checked that both the toolkit and xpfe print dialogues set the paper
width/height, see
http://lxr.mozilla.org/seamonkey/source/toolkit/components/printing/content/printjoboptions.js#916
and
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/unix/printjoboptions.js#914
.

It looks like this might indeed fix bug 272670 too.
Attachment #195164 - Attachment is obsolete: true
Attachment #195647 - Flags: review?(kherron+mozilla)
Comment on attachment 195647 [details] [diff] [review]
reworked patch

This looks good. Thanks for the update.
Attachment #195647 - Flags: superreview?(roc)
Attachment #195647 - Flags: review?(kherron+mozilla)
Attachment #195647 - Flags: review+
Blocks: 272670
Be aware, that you should not use the paper size itself, but the printible area.
Most laser/ink printers have unprintible areas, where the source/page number
etc. is printed on at now
(In reply to comment #5)
> Be aware, that you should not use the paper size itself, but the printible area.
> Most laser/ink printers have unprintible areas, where the source/page number
> etc. is printed on at now

nsPaper (what the code is using before this patch to get the paper size) does
not take that into account either, see
http://lxr.mozilla.org/seamonkey/source/gfx/src/psshared/nsPaperPS.cpp#49 .
Attachment #195647 - Flags: superreview?(roc) → superreview+
Checked in:

/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.125; previous revision: 1.124

Thanks for the patch, Christian!
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 195647 [details] [diff] [review]
reworked patch

Requesting approval to check on the 1.8 branch. This patch fixes the symptoms
of bug 272670 and bug 308895.
Attachment #195647 - Flags: approval1.8b5?
can we get a trunk verification on this before we consider it for the branch?
Thanks.
Blocks: 309988
(In reply to comment #9)
> can we get a trunk verification on this before we consider it for the branch?
> Thanks.

https://bugzilla.mozilla.org/show_bug.cgi?id=308895#c10
https://bugzilla.mozilla.org/show_bug.cgi?id=272670#c17
Status: RESOLVED → VERIFIED
Comment on attachment 195647 [details] [diff] [review]
reworked patch

I don't think this patch is bad per se, but we it's apparent we have a problem
with the print dialog returning bad data bug 272670 and bug 309988) and this
patch in conjunction with that seems to be causing print failures.
Attachment #195647 - Flags: approval1.8b5?
Did this cause bug 315687?
Comment on attachment 195647 [details] [diff] [review]
reworked patch

I think we need to get this onto at least the 1.8 branch if not the 1.8.0 branch - probably affects non-letter size users more than letter size users.
Attachment #195647 - Flags: approval1.8.1?
Attachment #195647 - Flags: approval1.8.0.1?
Comment on attachment 195647 [details] [diff] [review]
reworked patch

too late for 1.8.0.1, minusing.

I'm not going to nominate for 1.8.0.2 because I suspect it would just get rejected -- comments 11 and 12 imply unfixed regressions and those issues would have to be addressed.
Attachment #195647 - Flags: approval1.8.0.1? → approval1.8.0.1-
Attachment #195647 - Flags: approval1.8.1? → branch-1.8.1?(roc)
Comment on attachment 195647 [details] [diff] [review]
reworked patch

We need to sort out the regressions before taking this on any branch.
Attachment #195647 - Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1-
You need to log in before you can comment on or make changes to this bug.