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

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[rods (gone)]

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

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.

Comment 1

[Roland Mainz]

Accepting bug...

Comment 2

[Andrew Hagen]

Duplicate of bug 118954, or other way around?

Comment 3

[Kenneth Herron]

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

Comment 4

[Kenneth Herron]

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.

Comment 5

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsPaperPS.h

I've moved the paper size list out of nsPostScriptObj.h into a class of its
own. This is the header for that class.

Comment 6

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsPaperPS.cpp

This is the implementation for the nsPaperPS class.

Comment 7

[Kenneth Herron]

Attachment: Patch to use nsPaperPS

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 libgfxps.so and libgfx_gtk.so (or ..._xlib.so).
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 8

[Kenneth Herron]

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)?

Comment 9

[Roland Mainz]

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... :)

kherron:
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 10

[tor]

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

Comment 11

[Kenneth Herron]

Attachment: Add assertions to nsPaperSizePS methods

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.

Comment 12

[Boris Zbarsky [: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 13

[Kenneth Herron]

Comment on attachment 142866 [details] [diff] [review]
Add assertions to nsPaperSizePS methods

Oh, well. roc, any chance you could sr this?

Comment 14

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

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.

Comment 15

[Kenneth Herron]

Checked in with adjusted member names:

Checking in Makefile.in;
/cvsroot/mozilla/gfx/src/Makefile.in,v  <--  Makefile.in
new revision: 1.129; previous revision: 1.128
done
Checking in gtk/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsDeviceContextSpecG.cpp,v  <-- 
nsDeviceContextSpecG.cpp
new revision: 1.59; previous revision: 1.58
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPaperPS.cpp,v
done
Checking in ps/nsPaperPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPaperPS.cpp,v  <--  nsPaperPS.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPaperPS.h,v
done
Checking in ps/nsPaperPS.h;
/cvsroot/mozilla/gfx/src/ps/nsPaperPS.h,v  <--  nsPaperPS.h
initial revision: 1.1
done
Checking in ps/nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.110; previous revision: 1.109
done
Checking in ps/nsPostScriptObj.h;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.h,v  <--  nsPostScriptObj.h
new revision: 1.42; previous revision: 1.41
done
Checking in xlib/nsDeviceContextSpecXlib.cpp;
/cvsroot/mozilla/gfx/src/xlib/nsDeviceContextSpecXlib.cpp,v  <-- 
nsDeviceContextSpecXlib.cpp
new revision: 1.41; previous revision: 1.40
done

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Adding the nsPaperPS to gfx/src/Makefile.in broke the AIX tinderbox.

Comment 17

[Philip K. Warren]

Something in this patch is causing the AIX Tinderbox to fail:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1078721820.21916.gz&fulltext=1

Is there a reason why nsPaperPS.cpp and nsPaperPS.h were added to
gfx/src/Makefile.in instead of gfx/src/ps/Makefile.in? That is the only thing I
can think of at this point.

Comment 18

[Kenneth Herron]

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

Comment 19

[Kenneth Herron]

A further comment: On the solaris and linux systems I have access to, the
nsPaperPS dependencies file is ".deps/src/nsPaperPS.pp" rather than
".deps/src/ps/nsPaperPS.pp".

Comment 20

[Kenneth Herron]

Darnit, make that ".../src/.deps/nsPaperPS.pp". The point is there's no "ps"
subdirectory.

Comment 21

[Philip K. Warren]

Ok I'll look and see if I can see what is happening.

Comment 22

[Philip K. Warren]

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

Comment 23

[Kenneth Herron]

When .pp files are built by the compiler, the .pp filename comes from the source
filename with any directory component removed. See configure.in around line
4716, <http://lxr.mozilla.org/seamonkey/source/configure.in#4716>.

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

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/rules.mk lines 1543 - 1571, 
<http://lxr.mozilla.org/seamonkey/source/config/rules.mk#1543>. The rule:

$(MDDEPDIR)/%.pp: %.cpp
        $(REPORT_BUILD)
        @$(MAKE_DEPS_NOAUTO)

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.

Comment 24

[Kenneth Herron]

Reopening to correct build issues.

Comment 25

[Kenneth Herron]

Attachment: Makefile adjustments

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/libgkgfx.so, we could symlink the
source file into gfx/src/gtk and then build the object into both
gfx/src/gtk/libgfx_gtk.so and gfx/src/ps/libgfxps.so. 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

Comment 26

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

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
pavlov?

Comment 27

[cls]

Comment on attachment 143450 [details] [diff] [review]
Makefile adjustments

You neeed to add the copied file to GARBAGE as well.  Fix that && r=cls

Comment 28

[Kenneth Herron]

Attachment: Makefile adjustments v2

This adds the copy of nsPaperPS.cpp to GARBAGE. I've verified that the copy
gets deleted during "make clean".

Comment 29

[Kenneth Herron]

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.

Comment 30

[Kenneth Herron]

Requesting approval to checkin the makefile fix. Building fails for a particular
build configuration, as detailed in comment 23.

Comment 31

[Kenneth Herron]

Comment on attachment 143465 [details] [diff] [review]
Makefile adjustments v2

Oops, set the flag on the patch, not the bug...

Comment 32

[Kenneth Herron]

Makefile change checked in:

Checking in Makefile.in;
/cvsroot/mozilla/gfx/src/Makefile.in,v  <--  Makefile.in
new revision: 1.130; previous revision: 1.129
done
Checking in ps/Makefile.in;
/cvsroot/mozilla/gfx/src/ps/Makefile.in,v  <--  Makefile.in
new revision: 1.50; previous revision: 1.49
done

Comment 33

[Kenneth Herron]

No complaints from the tinderboxes. Resolving FIXED.