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.

Accepting bug...

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

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.

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

This is the implementation for the nsPaperPS class.

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

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

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.

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?

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.

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

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

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.

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 ".deps/src/ps/nsPaperPS.pp".

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

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

Reopening to correct build issues.

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 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 on attachment 143450 [details] [diff] [review] Makefile adjustments You neeed to add the copied file to GARBAGE as well. Fix that && r=cls

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

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.

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

Comment on attachment 143465 [details] [diff] [review] Makefile adjustments v2 Oops, set the flag on the patch, not the bug...

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

No complaints from the tinderboxes. Resolving FIXED.