Closed
Bug 175879
Opened 22 years ago
Closed 21 years ago
[ps] Default page sizes all in inches (bad)
Categories
(Core :: Printing: Output, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.7beta
People
(Reporter: rods, Assigned: kherron+mozilla)
References
Details
Attachments
(2 files, 4 obsolete files)
40.60 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
1.84 KB,
patch
|
dbaron
:
approval1.7b+
|
Details | Diff | Splinter Review |
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•22 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•22 years ago
|
||
Duplicate of bug 118954, or other way around?
Assignee | ||
Comment 3•21 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•21 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.
Assignee | ||
Comment 5•21 years ago
|
||
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•21 years ago
|
||
This is the implementation for the nsPaperPS class.
Assignee | ||
Comment 7•21 years ago
|
||
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•21 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•21 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•21 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•21 years ago
|
||
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•21 years ago
|
Attachment #141916 -
Attachment is obsolete: true
Attachment #141918 -
Attachment is obsolete: true
Attachment #141922 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #142866 -
Flags: superreview?(bzbarsky)
Comment 12•21 years ago
|
||
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•21 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•21 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
Closed: 21 years ago
Resolution: --- → FIXED
Adding the nsPaperPS to gfx/src/Makefile.in broke the AIX tinderbox.
Comment 17•21 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•21 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•21 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•21 years ago
|
||
Darnit, make that ".../src/.deps/nsPaperPS.pp". The point is there's no "ps"
subdirectory.
Comment 21•21 years ago
|
||
Ok I'll look and see if I can see what is happening.
Comment 22•21 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•21 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•21 years ago
|
||
Reopening to correct build issues.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 25•21 years ago
|
||
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•21 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•21 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•21 years ago
|
||
This adds the copy of nsPaperPS.cpp to GARBAGE. I've verified that the copy
gets deleted during "make clean".
Assignee | ||
Updated•21 years ago
|
Attachment #143450 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #143465 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 29•21 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•21 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•21 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•21 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•21 years ago
|
||
No complaints from the tinderboxes. Resolving FIXED.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Flags: blocking1.7b?
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•