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

RESOLVED FIXED in mozilla1.7beta

Status

()

Core
Printing: Output
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: rods (gone), Assigned: Kenneth Herron)

Tracking

Trunk
mozilla1.7beta
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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

16 years ago
Accepting bug...
Status: NEW → ASSIGNED
OS: Windows 2000 → Linux
Summary: Default page sizes all in inches (bad) → [ps] Default page sizes all in inches (bad)
Target Milestone: --- → mozilla1.3alpha

Comment 2

15 years ago
Duplicate of bug 118954, or other way around?
(Assignee)

Comment 3

15 years ago
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
Status: ASSIGNED → NEW
(Assignee)

Comment 4

15 years ago
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
Status: NEW → ASSIGNED
Target Milestone: mozilla1.3alpha → mozilla1.7beta
(Assignee)

Comment 5

14 years ago
Created attachment 141916 [details]
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.
(Assignee)

Comment 6

14 years ago
Created attachment 141918 [details]
New file gfx/src/ps/nsPaperPS.cpp

This is the implementation for the nsPaperPS class.
(Assignee)

Comment 7

14 years ago
Created attachment 141922 [details] [diff] [review]
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.
(Assignee)

Comment 8

14 years ago
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)

Comment 9

14 years ago
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

14 years ago
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+
(Assignee)

Comment 11

14 years ago
Created attachment 142866 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #141916 - Attachment is obsolete: true
Attachment #141918 - Attachment is obsolete: true
Attachment #141922 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
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....
(Assignee)

Comment 13

14 years ago
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+
(Assignee)

Comment 15

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Adding the nsPaperPS to gfx/src/Makefile.in broke the AIX tinderbox.

Comment 17

14 years ago
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.
(Assignee)

Comment 18

14 years ago
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).
(Assignee)

Comment 19

14 years ago
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".
(Assignee)

Comment 20

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

Comment 21

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

Comment 22

14 years ago
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).
(Assignee)

Comment 23

14 years ago
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.
(Assignee)

Comment 24

14 years ago
Reopening to correct build issues.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

14 years ago
Created attachment 143450 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
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
pavlov?
Attachment #143450 - Flags: review?(roc)

Comment 27

14 years ago
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+
(Assignee)

Comment 28

14 years ago
Created attachment 143465 [details] [diff] [review]
Makefile adjustments v2

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

Updated

14 years ago
Attachment #143450 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #143465 - Flags: superreview?(dbaron)
(Assignee)

Comment 29

14 years ago
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)
(Assignee)

Comment 30

14 years ago
Requesting approval to checkin the makefile fix. Building fails for a particular
build configuration, as detailed in comment 23.
Flags: blocking1.7b?
(Assignee)

Comment 31

14 years ago
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+
(Assignee)

Comment 32

14 years ago
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
(Assignee)

Comment 33

14 years ago
No complaints from the tinderboxes. Resolving FIXED.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Flags: blocking1.7b?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.