Closed Bug 175879 Opened 22 years ago Closed 20 years ago

[ps] Default page sizes all in inches (bad)


(Core :: Printing: Output, defect)

Not set





(Reporter: rods, Assigned: kherron+mozilla)




(2 files, 4 obsolete files)

In nsPostScriptObj the following array
PSPaperSizeRec postscript_module_paper_sizes[] =

defines all the paper sizes in inches. Is shouldn't. It should have an
additional attr for defining whether it is inches or not and then the code that
writes this array out needs to write out the width/height as inches or as
millimeters and makes sure inches/mm is set right in the prefs.js file for each

Then when a user selects a paper size the PS gets the correct value as to
whether the paper size is inches or mm.

This is all important, because right now the Page Setup is designed to have it
group box title change depending on whether the paper size unit is inches or mm,
but the remaining bug is for the PageSetup dialog to convert the margins to
inches or to mm depending on whether the paper size is in inches or mm.

I have the beginnings of that patch working now.
Accepting bug...
OS: Windows 2000 → Linux
Summary: Default page sizes all in inches (bad) → [ps] Default page sizes all in inches (bad)
Target Milestone: --- → mozilla1.3alpha
Duplicate of bug 118954, or other way around?
Taking. I have a patch almost ready.

(If anyone wants some paper sizes added to the list of sizes supported by the PS
module, this would be a good time to speak up.)
Assignee: Roland.Mainz → kjh-5727
I have the changes to fix this bug ready, but I'm waiting for some other patches
to go through review and checkin first. If I were to upload the patch now, it'd
just have to be updated once the other patches were checked in.

I've check-tested bug 118954 with the changes in place. When a "metric" paper
size is selected in File->Print->Properties->Page Size, the margins displayed in
File->Page Setup->Margins are in millimeters as expected. I believe that bug can
be closed once this bug is fixed.

I have not received any suggestions for additional standard paper sizes.
Blocks: 118954
Target Milestone: mozilla1.3alpha → mozilla1.7beta
Attached file New file gfx/src/ps/nsPaperPS.h (obsolete) —
I've moved the paper size list out of nsPostScriptObj.h into a class of its
own. This is the header for that class.
Attached file New file gfx/src/ps/nsPaperPS.cpp (obsolete) —
This is the implementation for the nsPaperPS class.
Attached patch Patch to use nsPaperPS (obsolete) — Splinter Review
This patch removes the old paper size and orientation tables from
nsPostScriptObj.h. The new class is used for paper sizes. Because the old
arrays were defined in a header file, mozilla was ending up with separate
copies of the arrays in and (or
Defining the array in an implementation file avoids this.

For the orientation array, I've just replaced it with hardcoded calls for
portrait and landscape orientations. There's little chance this list will
change, and I couldn't find anything that used the paper orientation records
anyway (far as I could tell the page setup dialog just uses the
can-change-orientation flag).

The old paper size table specified a 1/4 inch margin for all paper sizes. This
was additive with the margins from page setup and from printer properties, and
reduces the useful height & width by 1/2 in. This logic was apparently added to
address bug 130075, but I don't think a third, hardcoded margin is the right
way to address that bug. It wastes paper, it causes print preview to
misrepresent the page layout, and It makes the printer properties margin UI
pointless. We also have a bug report about this extra margin; see bug 229268. I
haven't retained this margin in the new paper size table.

The page setup and printer properties margins are handled by layout, so
removing the paper size table means the postscript module doesn't have to
concern itself with margins or the printable portion of the page. The patch
removes the margin-related code from nsPostScriptObj. I've also removed a few
unused member fields and one empty function (annotate_page).

With this patch in place, the page setup margins dialog uses millimeters if a
european paper size is selected in printer properties. I've tested the patch
with both gtk and xlib.
Comment on attachment 141922 [details] [diff] [review]
Patch to use nsPaperPS

tor, I think you asked me about this patch a couple of times. Would you be
interested in reviewing it (along with the two new files)?
Attachment #141922 - Flags: review?(tor)
The patch looks good in general.

... I don't like the name "nsPaperPS.*" ... the files could hold more than the
"page size values" in the future. What about nsPSCapabilities.* (sp?) ... or
something better... I am not very good in naming such stuff... :)

You don't need to attach new files seperately. There is a tool called "cvsdo"
which can do a "CVS add" without contacting the CVS server (and therefore even
without having cvs write. permission). You can simply run "cvsdo add
newPaperPS.*" and then diff the tree...
Comment on attachment 141922 [details] [diff] [review]
Patch to use nsPaperPS

Looks good.  One minor change that would be nice is to add
an assertion to nsPaperPS if someone does a next() past the
end.  Oh, and the copyright of the new files should probably
read 2004.  r=tor
Attachment #141922 - Flags: review?(tor) → review+
The new files are included in the patch this time. I've added precondition
checks to the nsPaperSizePS methods where appropriate, and updated the
copyright dates.
Attachment #141916 - Attachment is obsolete: true
Attachment #141918 - Attachment is obsolete: true
Attachment #141922 - Attachment is obsolete: true
Attachment #142866 - Flags: superreview?(bzbarsky)
I'm afraid I'm not likely to get to this any time soon (certainly not before
this Saturday, and more likely not till next Saturday, at least)... Sorry....
Comment on attachment 142866 [details] [diff] [review]
Add assertions to nsPaperSizePS methods

Oh, well. roc, any chance you could sr this?
Attachment #142866 - Flags: superreview?(bzbarsky) → superreview?(roc)
Comment on attachment 142866 [details] [diff] [review]
Add assertions to nsPaperSizePS methods

I think the nsPaperPS method names should conform to our conventions:
InterCaps. If you don't mind. Other than that, looks great.
Attachment #142866 - Flags: superreview?(roc) → superreview+
Checked in with adjusted member names:

Checking in;
/cvsroot/mozilla/gfx/src/,v  <--
new revision: 1.129; previous revision: 1.128
Checking in gtk/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsDeviceContextSpecG.cpp,v  <-- 
new revision: 1.59; previous revision: 1.58
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPaperPS.cpp,v
Checking in ps/nsPaperPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPaperPS.cpp,v  <--  nsPaperPS.cpp
initial revision: 1.1
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPaperPS.h,v
Checking in ps/nsPaperPS.h;
/cvsroot/mozilla/gfx/src/ps/nsPaperPS.h,v  <--  nsPaperPS.h
initial revision: 1.1
Checking in ps/nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.110; previous revision: 1.109
Checking in ps/nsPostScriptObj.h;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.h,v  <--  nsPostScriptObj.h
new revision: 1.42; previous revision: 1.41
Checking in xlib/nsDeviceContextSpecXlib.cpp;
/cvsroot/mozilla/gfx/src/xlib/nsDeviceContextSpecXlib.cpp,v  <-- 
new revision: 1.41; previous revision: 1.40
Closed: 20 years ago
Resolution: --- → FIXED
Adding the nsPaperPS to gfx/src/ broke the AIX tinderbox.
Something in this patch is causing the AIX Tinderbox to fail:

Is there a reason why nsPaperPS.cpp and nsPaperPS.h were added to
gfx/src/ instead of gfx/src/ps/ That is the only thing I
can think of at this point.
The nsPaperSizePS class has to be accessible to nsDeviceContextSpecG in gtk and
nsDeviceContextSpecXlib in xlib (depending on the toolkit being used). There was
a problem, either at link-time or run-time, with linking the class into the ps
library (I originally wrote this code back in december so I've forgotten which).
A further comment: On the solaris and linux systems I have access to, the
nsPaperPS dependencies file is ".deps/src/nsPaperPS.pp" rather than
Darnit, make that ".../src/.deps/nsPaperPS.pp". The point is there's no "ps"
Ok I'll look and see if I can see what is happening.
For now I have changed the default build target for the AIX tinderbox 
from 'build_all_depend' to 'build'. Since it is a clobber build, I don't think 
we need to be generating the dependency information anyway.

With gmake 3.79.1 and 3.80, I get the same results on AIX when building 
dependencies for nsPaperPS.cpp (it tries to create 
gfx/src/.deps/ps/nsPaperPS.pp instead of gfx/src/.deps/nsPaperPS.pp).
When .pp files are built by the compiler, the .pp filename comes from the source
filename with any directory component removed. See around line
4716, <>.

When --enable-auto-deps is set (the default) and the compiler isn't used to
build .pp files, the relevant section is config/ lines 1052 - 1081; see
<>. Again, any
directory component in the source filename is removed (note the macro for

Laredo doesn't use gcc, and is (was) building with --disable-auto-deps. In this
case, .pp files are built from dependency rules which preserve directory
components from the source file; see config/ lines 1543 - 1571, 
<>. The rule:

$(MDDEPDIR)/%.pp: %.cpp

sets a target of ".deps/ps/nsPaperPS.pp" from "ps/nsPaperPS.cpp". So this build
configuration behaves differently from the others.

I've gone through the gmake manual and tried a few experiments, but I haven't
found a way to make the .pp dependency rule strip out the "ps" portion. In
particular, something like "$(MDDEPDIR)/$(notdir %.pp): %.cpp" doesn't work.
Assuming we want to continue supporting this build configuration, I suppose the
simplest solution is to move nsPaperPS.cpp into the base directory for whatever
library it's linked into.
Reopening to correct build issues.
Resolution: FIXED → ---
Attached patch Makefile adjustments (obsolete) — Splinter Review
This symlinks nsPaperPS.cpp into gfx/src, instead of referring to the file in
its original location. nsPaperPS.h is exported from the gfx/src/ps makefile
instead of the gfx/src makefile. It turns out nsPostScriptObj.h no longer needs
to be exported, so I've removed that from the gfx/src/ps export list. I've
tested this in an objdir build on linux.

Instead of building nsPaperPS.o into gfx/src/, we could symlink the
source file into gfx/src/gtk and then build the object into both
gfx/src/gtk/ and gfx/src/ps/ We'd end up with two
copies of the object, one in each library, but it's a small object, and it
looks like there are other objects within gfx/src handled that way.

> size nsPaperPS.o	  # on linux
   text    data     bss     dec     hex filename
    121      96       0     217      d9 nsPaperPS.o
Attachment #143450 - Flags: review?(roc)
Comment on attachment 143450 [details] [diff] [review]
Makefile adjustments

Sorry, but I'm not really confident about build issues and whether doing this
file copy/symlink is the right thing. Can you get review from blizzard or even
Attachment #143450 - Flags: review?(roc)
Comment on attachment 143450 [details] [diff] [review]
Makefile adjustments

You neeed to add the copied file to GARBAGE as well.  Fix that && r=cls
Attachment #143450 - Flags: review+
This adds the copy of nsPaperPS.cpp to GARBAGE. I've verified that the copy
gets deleted during "make clean".
Attachment #143450 - Attachment is obsolete: true
Attachment #143465 - Flags: superreview?(dbaron)
Comment on attachment 143465 [details] [diff] [review]
Makefile adjustments v2

Cancelling sr request. I'm told it's not necessary for	a makefile-only patch.
Attachment #143465 - Flags: superreview?(dbaron)
Requesting approval to checkin the makefile fix. Building fails for a particular
build configuration, as detailed in comment 23.
Flags: blocking1.7b?
Comment on attachment 143465 [details] [diff] [review]
Makefile adjustments v2

Oops, set the flag on the patch, not the bug...
Attachment #143465 - Flags: approval1.7b?
Attachment #143465 - Flags: approval1.7b? → approval1.7b+
Makefile change checked in:

Checking in;
/cvsroot/mozilla/gfx/src/,v  <--
new revision: 1.130; previous revision: 1.129
Checking in ps/;
/cvsroot/mozilla/gfx/src/ps/,v  <--
new revision: 1.50; previous revision: 1.49
No complaints from the tinderboxes. Resolving FIXED.
Closed: 20 years ago20 years ago
Flags: blocking1.7b?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.