307404 - respect the paper size setting

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

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].

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: proposed fix

Chosing reviewer based on reviewers of recent checkins to nsPostScriptObj.cpp,
hope that's correct.

Comment 2

[Kenneth Herron]

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?

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: reworked patch

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.

Comment 4

[Kenneth Herron]

Comment on attachment 195647 [details] [diff] [review]
reworked patch

This looks good. Thanks for the update.

Comment 5

[Joachim]

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

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

(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 .

Comment 7

[Kenneth Herron]

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!

Comment 8

[Kenneth Herron]

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.

Comment 9

[Asa Dotzler [:asa]]

can we get a trunk verification on this before we consider it for the branch?
Thanks.

Comment 10

[Andreas Lange]

(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

Comment 11

[Kenneth Herron]

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.

Comment 12

[José Jeria]

Did this cause bug 315687?

Comment 13

[Ian Neal]

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.

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 195647 [details] [diff] [review]
reworked patch

We need to sort out the regressions before taking this on any branch.