78548 - Xprint, 2nd revamp...

Status: RESOLVED FIXED

(Core Graveyard :: Printing: Xprint, defect)

Description

[Roland Mainz]

More or less as a loose followup to RFE/bug 72087:
2nd revamp of Mozilla's Xprint module:
Goals:
- Get both Xlib-toolkit and Xprint code in sync to get newer code (mainly better
font and image handling) - maybe make some classes in Xprint module a subclass
of Xlib-toolkit classes. Note that some code should be a 1:1 copy from
Xlib-toolkit - copied code should only include a minimum of changes (incl.
<hint_for_shaver>XPCOM</hint_for_shaver> stuff !!!!!!).
- Better error handling
- Address all portability issues requires to "land" changes in bug 49947
- Fix some bugs which are still "open" from bug 72087
- Fix XPCOM issues to make shaver happy - except those stuff which is copied 1:!
from Xlib-toolkit (see comment above...)

Comment 1

[Roland Mainz]

Sorry for SPAM... swapping QA and owner...

Comment 2

[Roland Mainz]

Accepting bug, marking dependicy to bug 49947...

Comment 3

[Roland Mainz]

Oups... mistake... this bug blocks bug 49947...

More work (both silly but usefull for LXR search...):
- Rename module to a name which does not interfer with XPCOM stuff
- Rename classes from *XP* to *Xp* (lowercase 'p' !) for the same reason

Comment 4

[Roland Mainz]

More bugs: 
- Current Xprint module uses fork()-version instead of thread version for
print-to-file consumer
- <wait.h> required for waitid(2) in xprintutil_printtofile.c not available in
HP-UX (see bug 49947). I need an overview which OS need it and which not
(Solaris requires to include this header, HP-UX not... but what about
AIX/Linux/IRIX/SCO and so on...) ?
- Implement support for landscape/portrait printing (and return error if printer
doesn't support that (Xprint is smart and can figure that out... :-)
- Implement currect selection of page size (and return error if printer doesn't
support that (Xprint is smart and can figure that out... :-)
- Update nsGfxfactoryXP.cpp - we are not the PostScript print module... =:-) -
and we need a better module name which significantly differs from XP* stuff

Questions:
What happens when I simply make some classes in mozilla/gfx/src/xprint/ a
subclass of the matching classes in mozilla/gfx/src/xlib/ ? Are there any
problems with this (static vars for example) ? Are there any side-effects ?

Comment 5

[Roland Mainz]

Attachment: Patch for 2001-05-01-08-trunk

Comment 6

[Roland Mainz]

Attachment: New patch for 2001-05-05-08-trunk

Comment 7

[Roland Mainz]

Attachment: http://de.movies.yahoo.com/mvs.html printed via patches Xprint module (bad font quality is issue of DPS in Xsun + 400% scaling)

Comment 8

[Roland Mainz]

Attached new patch to fix some other bugs - image printing is now working as
expected (not in grayscale mode yet) - incl. fix for bug 54092. I still see some
minor GIF background artifacts - but I assume this are bugs in GIF transparent
image handling (please correct me if I am wrong).
Text/image scaling is still an issue (I've reduced text scaling factor from 2.4
down to 2.0 for now) - I am going to address this in another bug (not _now_)...

Want r=, sr= and checkin, please...

Comment 9

[Roland Mainz]

Attachment: New patch for 2001-05-05-08-trunk, now taking care of timeless's comments and adds two (new !!) missing files (nsPrintOptionsXlib.*) in mozilla/gfx/src/xlib/

Comment 10

[Chris Waterson]

Could you also attach diffs with the ``-wu'' option set? (To filter out 
whitespace changes.)

Comment 11

[Roland Mainz]

Attachment: Diff for review (same as attach_id=33372) but with -w -i to ignore whitespace/tab conversions and case changes (like s/XP/Xp/) REVIEW ONLY!!

Comment 12

[Roland Mainz]

The patches have a minor flaw... they create a new file
"mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej" which accidently slipped into
the patch... ;-(

Comment 13

[Chris Waterson]

I really think rods and blizzard should r= this code, but here are the comments 
that I have. (Comments appear _after_ the code in question.)

General style nits:

1. Are arguments separated from parens with whitespace or not? I see both
   styles in this patch. Please choose one.

2. Does |if| have a space after it or not?

I see all of these styles in this patch, sometimes mixed in the same line:

  if(foo)
  if( foo )
  if (foo)
  if ( foo )
  if ( foo)

Specific comments below.

+          // convert Unicode strings to cstrings
+          nsAutoString printFileStr;
+          printFileStr  = printfile;
+          char *pPrintFileStr = printFileStr.ToNewCString();
 
+          strcpy( mPrData.path, pPrintFileStr);
+          nsMemory::Free(pPrintFileStr);

The above can (and should) be done much more simply as:

  strcpy(mPrData.Path, NS_ConvertUCS2toUTF8(printfile).get());

Of course, you may very well be screwed if mPrData.Path is not expecting UTF-8 
encoded data. You probably want to convert to whatever the native character set 
is that's in use by the filesystem. I'm really _not_ sure how to do that, but at 
worst leave an ``XXX'' comment that says something to that effect.

Fix the other string munging (e.g., pCmdStr) as well.

+ifdef MOZ_ENABLE_XPRINT
+DEFINES         += -DUSE_XPRINT
+endif
+

Can't we use MOZ_ENABLE_XPRINT as the C-level #define, too? In other words, I'd 
prefer it if you'd replace ``#ifdef USE_XPRINT'' with ``#ifdef 
MOZ_ENABLE_XPRINT''.

+static NS_DEFINE_IID(kIDeviceContextSpecIID, NS_IDEVICE_CONTEXT_SPEC_IID);
+static NS_DEFINE_IID(kIDeviceContextSpecPSIID, NS_IDEVICE_CONTEXT_SPEC_PS_IID);
+#ifdef USE_XPRINT
+static NS_DEFINE_IID(kIDeviceContextSpecXPIID, NS_IDEVICE_CONTEXT_SPEC_XP_IID);
+#endif
+

Can't you use NS_GET_IID() for these?

-    *aInstancePtr = (void *) (nsIDeviceContextSpecXP *) this;
+    nsIDeviceContextSpecXp *tmp = this;
+    *aInstancePtr = (void*) tmp;

This seems unnecessary.

+  // if there is a current selection then enable the "Selection" radio button
+  if (NS_SUCCEEDED(rv) && printService) {
+    PRBool isOn;
+    
printService->GetPrintOptions(nsIPrintOptions::kPrintOptionsEnableSelectionRB, 
&isOn);
+    nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv);
+    if (NS_SUCCEEDED(rv) && pPrefs) {
+      (void) pPrefs->SetBoolPref("print.selection_radio_enabled", isOn);
+    }
+  }

Why are we mangling prefs from back-end code? This seems quite evil.

+        if (command != nsnull) {
+          // convert Unicode strings to cstrings
+          nsAutoString cmdStr;
+          cmdStr        = command;
+          char *pCmdStr = cmdStr.ToNewCString();
+
+          strcpy( mPrData.command, pCmdStr );
+          nsMemory::Free(pCmdStr);
+        }
+        if (printfile != nsnull) {
+          // convert Unicode strings to cstrings
+          nsAutoString printFileStr;
+          printFileStr  = printfile;
+          char *pPrintFileStr = printFileStr.ToNewCString();
+
+          strcpy( mPrData.path, pPrintFileStr);
+          nsMemory::Free(pPrintFileStr);
+        }

More C-string conversion badness.

+  int method=-1;
+  nsDeviceContextSpecXlib *spec = NS_STATIC_CAST(nsDeviceContextSpecXlib *, 
aDevice);
+  spec->GetPrintMethod(method);
+
+  if (method == 1) { // XPRINT

Use an enum.

+    dcxp->InitDeviceContextXP((nsIDeviceContext*)aContext,
+                              (nsIDeviceContext*)this);

NS_STATIC_CAST, but why are you allowed to cast an nsIDeviceContextSpec to an 
nsIDeviceContext at all? Shouldn't you QI()?

+#if 0 /* this does not work for an unknown reason */
+    rv = CallQueryInterface(dcxp, aContext);
+#else
+    rv = dcxp->QueryInterface(NS_GET_IID(nsIDeviceContext),
+                              (void **)&aContext);
+#endif    

Probably because |aContext| is an |nsIDeviceContext*&|. What is the specific 
problem here? Could you please get with scc to figure out what's going on?

+  { "Print Options",
+     NS_PRINTOPTIONS_CID,
+     //    "@mozilla.org/gfx/printoptions;1",
+     "@mozilla.org/gfx/printoptions;1",
+     nsPrintOptionsXlibConstructor },      

Why the // comment?

 MODULE         = layout
-LIBRARY_NAME   = gfxxprt
+LIBRARY_NAME   = gfxxprint

I suspect this module stuff is wrong. You may want to talk with dbaron or cls to 
figure out how to get it set correctly.

+  if (mPrintContext) 
+    delete mPrintContext; // we cannot reuse that...
+  
      mPrintContext = new nsXPrintContext();

Funny spaces

-  /* BUG: this does - unfortunately - not work due missing DISPLAY ptr
-   * Additionally this is the _wrong_ time to make any assumtions 
-   * about fonts - this info+the correct information is _only_ 
-   * available _after_ XpSetContext() - therefore any assumtions 
-   * made here are _wrong_ until we have a vaoid XPContext 
-   * (X11 print context)
-   */
-#if 0
   return nsFontMetricsXP::FamilyExists(aFontName);
-#else  

Why is this fixed all of a sudden? Because it just works in the xprint DC?

-#define nsDeviceContextXP_h___
+#define nsDeviceContextXp_h___ 1

No.

+
+static PRLogModuleInfo * FontMetricsXpLM = PR_NewLogModule("FontMetricsXp");
+

This needs to be under |#ifdef PR_LOGGING|.

+// Global variables
+// XXX many of these statics need to be freed at shutdown time
+

Uh, yeah! Do you?

I am not qualified to r= the changes here in gfx/src/xprint/nsFontMetricsXP.cpp, 
please get blizzard to look at them. I guess I'm a little bit concerned that 
we're hard-coding all this junk, but maybe that's just how it's done.

-          if ((bounds->ascent) || (bounds->descent)) {
+          if ((!bounds->ascent) && (!bounds->descent)) {
             SET_REPRESENTABLE(map, (row << 8) | cell);

Is this really right?

-#define nsFontMetricsXP_h__
+#define nsFontMetricsXp_h__ 1

No.

+  nsCString          *mGeneric;

Is this an array of |nsCString| objects? If so, use nsCStringArray. If not, just 
use an nsCString, inline.

+  PRUint8            mTriedAllGenerics;

PRBool? Or PRPackedBool if the space is important? From looking at surrounding 
members, you're screwed by alignment anyway.

+  nsCAutoString      mUserDefined;

Why nsCAutoString member? This has a huge inline buffer!

diff -r -u -N -w -i mozilla_original/gfx/src/xprint/nsGfxFactoryXP.cpp.rej 
mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej
--- mozilla_original/gfx/src/xprint/nsGfxFactoryXP.cpp.rej      Thu Jan  1 
01:00:00 1970
+++ mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej       Mon May  7 11:19:47 2001

Why are these reject files included in the patch file?

 
+static PRLogModuleInfo * RenderingContextXpLM = 
PR_NewLogModule("nsRenderingContextXp");
+
+

This should be under |#ifdef PR_LOGGING|

Comment 14

[Roland Mainz]

> I really think rods and blizzard should r= this code, but here are the
comments 
> that I have. (Comments appear _after_ the code in question.)
> 
> General style nits:
> 
> 1. Are arguments separated from parens with whitespace or not? I see both
>    styles in this patch. Please choose one.
> 
> 2. Does |if| have a space after it or not?
> 
> I see all of these styles in this patch, sometimes mixed in the same line:
> 
>   if(foo)
>   if( foo )
>   if (foo)
>   if ( foo )
>   if ( foo)

Code was originally written by various people which causes this whitespace "fun"
- I try to fix this when I am editing the code parts - but I cannot simply
reformat all the code.

As I said in IRC #mozilla - the  and nsFonsMetricsXP.h are both _copies_ from
Xlib-toolkit. I really do not like to change both sources (unless it is
_mandatory) _because_ it is a 1:1-copy+s/Xlib/Xp/ except minor API adaptions.
The reason is that I'd like to make the classes in nsFonsMetricsXP.* subclasses
of Xlib-toolkit classes - and this is the test for this (OK... and I get far
better fontmetrics code into Xprint code... :-) 
Any changes in that code except copying it may result in loss of these
changes... - I'd like to avoid that...

> Specific comments below.
> 
> +          // convert Unicode strings to cstrings
> +          nsAutoString printFileStr;
> +          printFileStr  = printfile;
> +          char *pPrintFileStr = printFileStr.ToNewCString();
>  
> +          strcpy( mPrData.path, pPrintFileStr);
> +          nsMemory::Free(pPrintFileStr);
> 
> The above can (and should) be done much more simply as:
> 
>   strcpy(mPrData.Path, NS_ConvertUCS2toUTF8(printfile).get());
> 
> Of course, you may very well be screwed if mPrData.Path is not expecting UTF-8 
> encoded data. 

I tested that in another piece of Xprint code - it does not work for ISO-Latin-1
chars like german "umlauts" (äöÜß and so on...) or swedish/dutch special
chars...

This is covered by bug 73446 ("Need to know how to convert between local
encoding and UCS2"...).
I'll change that if the neccesary API is available...

> You probably want to convert to whatever the native character set 
> is that's in use by the filesystem. I'm really _not_ sure how to do that, but
at 
> worst leave an ``XXX'' comment that says something to that effect.

See above...

> Fix the other string munging (e.g., pCmdStr) as well.
> 
> +ifdef MOZ_ENABLE_XPRINT
> +DEFINES         += -DUSE_XPRINT
> +endif
> +
> 
> Can't we use MOZ_ENABLE_XPRINT as the C-level #define, too? In other words,
I'd 
> prefer it if you'd replace ``#ifdef USE_XPRINT'' with ``#ifdef 
> MOZ_ENABLE_XPRINT''.

Unknown... I never tried it. "USE_XPRINT" was the symbol used in C/C++ code and
I never changed it...
If I going to change that I would have to touch tons of other sources, too.
Later, please...

> +static NS_DEFINE_IID(kIDeviceContextSpecIID, NS_IDEVICE_CONTEXT_SPEC_IID);
> +static NS_DEFINE_IID(kIDeviceContextSpecPSIID,
NS_IDEVICE_CONTEXT_SPEC_PS_IID);
> +#ifdef USE_XPRINT
> +static NS_DEFINE_IID(kIDeviceContextSpecXPIID,
NS_IDEVICE_CONTEXT_SPEC_XP_IID);
> +#endif
> +
> 
> Can't you use NS_GET_IID() for these?

Good question - I didn't wrote that code - I just copied it from GTK+ toolkit...
- I don't even know exacely what NS_DEFINE_IID() does... ;-(

> -    *aInstancePtr = (void *) (nsIDeviceContextSpecXP *) this;
> +    nsIDeviceContextSpecXp *tmp = this;
> +    *aInstancePtr = (void*) tmp;
> 
> This seems unnecessary.

This is only to make the code more consistent. Other code above/below that point
uses the "tmp" var... now I am usuing it, too - just to make the whole thing
more consistent... :-)

> +  // if there is a current selection then enable the "Selection" radio button
> +  if (NS_SUCCEEDED(rv) && printService) {
> +    PRBool isOn;
> +    
> printService->GetPrintOptions(nsIPrintOptions::kPrintOptionsEnableSelectionRB, 
> &isOn);
> +    nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv);
> +    if (NS_SUCCEEDED(rv) && pPrefs) {
> +      (void) pPrefs->SetBoolPref("print.selection_radio_enabled", isOn);
> +    }
> +  }
> 
> Why are we mangling prefs from back-end code? This seems quite evil.

It is - but see the comment at the head of the function - this is a exact 1:1
copy from GTK+-toolkit.

> +        if (command != nsnull) {
> +          // convert Unicode strings to cstrings
> +          nsAutoString cmdStr;
> +          cmdStr        = command;
> +          char *pCmdStr = cmdStr.ToNewCString();
> +
> +          strcpy( mPrData.command, pCmdStr );
> +          nsMemory::Free(pCmdStr);
> +        }
> +        if (printfile != nsnull) {
> +          // convert Unicode strings to cstrings
> +          nsAutoString printFileStr;
> +          printFileStr  = printfile;
> +          char *pPrintFileStr = printFileStr.ToNewCString();
> +
> +          strcpy( mPrData.path, pPrintFileStr);
> +          nsMemory::Free(pPrintFileStr);
> +        }
> 
> More C-string conversion badness.

Yup... It's _bad_. I fully agree here - but I do not know exactly how to fix
this without killing other functionality nor breaking non-UTF-8 locales...

> +  int method=-1;
> +  nsDeviceContextSpecXlib *spec = NS_STATIC_CAST(nsDeviceContextSpecXlib *, 
> aDevice);
> +  spec->GetPrintMethod(method);
> +
> +  if (method == 1) { // XPRINT
> 
> Use an enum.

Planned. This is #define-madness from original code... ;-(

> +    dcxp->InitDeviceContextXP((nsIDeviceContext*)aContext,
> +                              (nsIDeviceContext*)this);
> 
> NS_STATIC_CAST, but why are you allowed to cast an nsIDeviceContextSpec to an 
> nsIDeviceContext at all? Shouldn't you QI()?

What is QI() !?

> +#if 0 /* this does not work for an unknown reason */
> +    rv = CallQueryInterface(dcxp, aContext);
> +#else
> +    rv = dcxp->QueryInterface(NS_GET_IID(nsIDeviceContext),
> +                              (void **)&aContext);
> +#endif    
> 
> Probably because |aContext| is an |nsIDeviceContext*&|. What is the specific 
> problem here?

Both Sun Workshop and gcc reject that construct with tons of errors...

> Could you please get with scc to figure out what's going on?

I'll email him...

> +  { "Print Options",
> +     NS_PRINTOPTIONS_CID,
> +     //    "@mozilla.org/gfx/printoptions;1",
> +     "@mozilla.org/gfx/printoptions;1",
> +     nsPrintOptionsXlibConstructor },      
> 
> Why the // comment?

Copy from GTK+ toolkit... I simply didn't saw it...

>  MODULE         = layout
> -LIBRARY_NAME   = gfxxprt
> +LIBRARY_NAME   = gfxxprint
> 
> I suspect this module stuff is wrong. You may want to talk with dbaron or cls
to 
> figure out how to get it set correctly.

Uhm... _what_ is wrong here ?
I only changed the name s/gfxxprt/gfxxprint/ because "Xprt" is the X print
server - the _server_side_ - naming the client-side "xprt" a horrible misleading
name...

> +  if (mPrintContext) 
> +    delete mPrintContext; // we cannot reuse that...
> +  
>       mPrintContext = new nsXPrintContext();
> 
> Funny spaces

Funny _dead_ spaces... =:-)
Going to fix that...

> -  /* BUG: this does - unfortunately - not work due missing DISPLAY ptr
> -   * Additionally this is the _wrong_ time to make any assumtions 
> -   * about fonts - this info+the correct information is _only_ 
> -   * available _after_ XpSetContext() - therefore any assumtions 
> -   * made here are _wrong_ until we have a vaoid XPContext 
> -   * (X11 print context)
> -   */
> -#if 0
>    return nsFontMetricsXP::FamilyExists(aFontName);
> -#else  
> 
> Why is this fixed all of a sudden? Because it just works in the xprint DC?

No... Xprint fontmetrics stuff was _outdated_ stuff stolen from ancient
Xlib-toolkit code. I copied-over the newest nsFontMetricsXlib.cpp from
Xlib-toolkit which kills _tons_ of font issues in Xprint land...

> -#define nsDeviceContextXP_h___
> +#define nsDeviceContextXp_h___ 1
> 
> No.

Why ? "#define x" is AFAIK equal to "#define x 1"... (this was an accident -
sometimes #define x 1 is required for portabiliy...).


> +static PRLogModuleInfo * FontMetricsXpLM = PR_NewLogModule("FontMetricsXp");
> +
> 
> This needs to be under |#ifdef PR_LOGGING|.

OK... going to fix that.

> +// Global variables
> +// XXX many of these statics need to be freed at shutdown time
> +
> 
> Uh, yeah! Do you?
> 
> I am not qualified to r= the changes here in
gfx/src/xprint/nsFontMetricsXP.cpp, 
> please get blizzard to look at them. I guess I'm a little bit concerned that 
> we're hard-coding all this junk, but maybe that's just how it's done.

See my comment about nsFontMetricsXp.* above. This is a _copy_ - I didn#t wrote
that code - I just copied it...

> -          if ((bounds->ascent) || (bounds->descent)) {
> +          if ((!bounds->ascent) && (!bounds->descent)) {
>              SET_REPRESENTABLE(map, (row << 8) | cell);
> 
> Is this really right?
> 
> -#define nsFontMetricsXP_h__
> +#define nsFontMetricsXp_h__ 1
> 
> No.

See above.

> +  nsCString          *mGeneric;
> 
> Is this an array of |nsCString| objects? If so, use nsCStringArray. If not,
just 
> use an nsCString, inline.
> 
> +  PRUint8            mTriedAllGenerics;
> 
> PRBool? Or PRPackedBool if the space is important? From looking at surrounding 
> members, you're screwed by alignment anyway.
> 
> +  nsCAutoString      mUserDefined;
> 
> Why nsCAutoString member? This has a huge inline buffer!
> 
> diff -r -u -N -w -i mozilla_original/gfx/src/xprint/nsGfxFactoryXP.cpp.rej 
> mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej
> --- mozilla_original/gfx/src/xprint/nsGfxFactoryXP.cpp.rej      Thu Jan  1 
> 01:00:00 1970
> +++ mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej       Mon May  7 11:19:47
2001
> 
> Why are these reject files included in the patch file?

See my comment in this bug... small glich... I saw it some hours after making
that patch... ;-(

> +static PRLogModuleInfo * RenderingContextXpLM = 
> PR_NewLogModule("nsRenderingContextXp");
> +
> +
> 
> This should be under |#ifdef PR_LOGGIN

Going to fix that...

Comment 15

[Roland Mainz]

Attachment: Patch for 2001-05-07-08-trunk fixing some 1st review-cycle issues

Comment 16

[Roland Mainz]

Attachment: Diff for review (same as attach_id=33587) but with -w -i to ignore whitespace/tab conversions and case changes (like s/XP/Xp/) REVIEW ONLY!!

Comment 17

[Roland Mainz]

> +          // convert Unicode strings to cstrings
> +          nsAutoString printFileStr;
> +          printFileStr  = printfile;
> +          char *pPrintFileStr = printFileStr.ToNewCString();
>  
> +          strcpy( mPrData.path, pPrintFileStr);
> +          nsMemory::Free(pPrintFileStr);
> 
> The above can (and should) be done much more simply as:
> 
>   strcpy(mPrData.Path, NS_ConvertUCS2toUTF8(printfile).get());

Fixed.

> Of course, you may very well be screwed if mPrData.Path is not expecting UTF-8 
> encoded data. You probably want to convert to whatever the native character
set 
> is that's in use by the filesystem. I'm really _not_ sure how to do that, but
at 
> worst leave an ``XXX'' comment that says something to that effect.

Fixed - added comment.

> Fix the other string munging (e.g., pCmdStr) as well.

Fixed.

> +ifdef MOZ_ENABLE_XPRINT
> +DEFINES         += -DUSE_XPRINT
> +endif
> +
> 
> Can't we use MOZ_ENABLE_XPRINT as the C-level #define, too? In other words,
I'd 
> prefer it if you'd replace ``#ifdef USE_XPRINT'' with ``#ifdef 
> MOZ_ENABLE_XPRINT''.

I still don't know _why_ the original author did that... I assume there must be
some reason for this...
Not fixed for now...

[snip] 
> +  // if there is a current selection then enable the "Selection" radio button
> +  if (NS_SUCCEEDED(rv) && printService) {
> +    PRBool isOn;
> +    
> printService->GetPrintOptions(nsIPrintOptions::kPrintOptionsEnableSelectionRB, 
> &isOn);
> +    nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv);
> +    if (NS_SUCCEEDED(rv) && pPrefs) {
> +      (void) pPrefs->SetBoolPref("print.selection_radio_enabled", isOn);
> +    }
> +  }
> 
> Why are we mangling prefs from back-end code? This seems quite evil.

The reason for this code is simple: Prefs API is misused for communication
between back-end code and prefs dialog "code"(=XUL/JavaScript)...
AFAIK there's no easy fix for this... this is definately dcone's land...
Not fixed.

> +        if (command != nsnull) {
> +          // convert Unicode strings to cstrings
> +          nsAutoString cmdStr;
> +          cmdStr        = command;
> +          char *pCmdStr = cmdStr.ToNewCString();
> +
> +          strcpy( mPrData.command, pCmdStr );
> +          nsMemory::Free(pCmdStr);
> +        }
> +        if (printfile != nsnull) {
> +          // convert Unicode strings to cstrings
> +          nsAutoString printFileStr;
> +          printFileStr  = printfile;
> +          char *pPrintFileStr = printFileStr.ToNewCString();
> +
> +          strcpy( mPrData.path, pPrintFileStr);
> +          nsMemory::Free(pPrintFileStr);
> +        }
> 
> More C-string conversion badness.

Fixed.

> +  int method=-1;
> +  nsDeviceContextSpecXlib *spec = NS_STATIC_CAST(nsDeviceContextSpecXlib *, 
> aDevice);
> +  spec->GetPrintMethod(method);
> +
> +  if (method == 1) { // XPRINT
> 
> Use an enum.

Going to fix that later (please).

[snip]
> +  if (mPrintContext) 
> +    delete mPrintContext; // we cannot reuse that...
> +  
>       mPrintContext = new nsXPrintContext();
> 
> Funny spaces

Fixed.

[snip]
> -#define nsDeviceContextXP_h___
> +#define nsDeviceContextXp_h___ 1
> 
> No.

Fixed.

> +static PRLogModuleInfo * FontMetricsXpLM = PR_NewLogModule("FontMetricsXp");
> +
> 
> This needs to be under |#ifdef PR_LOGGING|.

Fixed.
Fixed in mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp, too.

[snip]
> diff -r -u -N -w -i mozilla_original/gfx/src/xprint/nsGfxFactoryXP.cpp.rej 
> mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej
> --- mozilla_original/gfx/src/xprint/nsGfxFactoryXP.cpp.rej      Thu Jan  1 
> 01:00:00 1970
> +++ mozilla/gfx/src/xprint/nsGfxFactoryXP.cpp.rej       Mon May  7 11:19:47
2001
> 
> Why are these reject files included in the patch file?

Fixed. That "new" file is gone... :-)

> +static PRLogModuleInfo * RenderingContextXpLM = 
> PR_NewLogModule("nsRenderingContextXp");
> +
> +
> 
> This should be under |#ifdef PR_LOGGING|

Fixed.

Comment 18

[Roland Mainz]

Attachment: http://www.mozilla.org/ - printed via todays newest patch and Xprt PS DDX from X11R6.6 - PS2GIF via sdtimage

Comment 19

[Roland Mainz]

I am going to file a new patch in a few secs.
Changes:
- Changed nsDeviceContextXp::InitDeviceContextXP() to fix bug 71669, bug 72216,
bug 57820 (partial fix; scaling images does not work due broken image scaling
code) and some other bugs...
- Removed unneccesary code in class nsDeviceContextXp which isn't needed anymore
due the change above.
- Fixed a GC leak in nsXPrintContext::DrawImage()
- Fixed patch bustage caused by checkin of patches for bug 79132
- Moved patches from bug 79132 over to nsFontmetricsXP.cpp to keep both sources
in sync

Comment 20

[Roland Mainz]

Attachment: Patch for 2001-05-08-08-trunk

Comment 21

[Roland Mainz]

Attachment: Diff for review (same as attach_id=33681) but with -w -i to ignore whitespace/tab conversions and case changes (like s/XP/Xp/) REVIEW ONLY!!

Comment 22

[Roland Mainz]

Attachment: Patch for 2001-05-09-08-trunk to fix bustage caused by other checkins...

Comment 23

[Roland Mainz]

Attachment: Diff for review (same as attach_id=34053) but with -w -i to ignore whitespace/tab conversions and case changes (like s/XP/Xp/) REVIEW ONLY!!

Comment 24

[Roland Mainz]

I filed a new patch to fix bustage caused by other checkins.

Small note for reviewers: Nearly 200k of that patch (4/5 == 80%) is the code
mover which copies nsFontmetricsXlib.cpp and nsFontmetricsXlib.h to
nsFontmetricsXP.cpp and nsFontmetricsXlib.h. Another large amount is the change
s/XP/Xp/ (e.g. case change) - the real changes are less that 18kb...

Comment 25

[timeless]

All files i don't mention are fine (r=timeless)

[all trivial]
mozilla/gfx/src/xlib/nsPrintOptionsXlib.cpp 
mozilla/gfx/src/xlib/nsPrintOptionsXlib.h
MPL!!
remove? + *   Travis Bogard <[e-mail]>

mozilla/gfx/src/gtk/nsDeviceContextSpecG.cpp
near @@ -243,10 +243,11 @@
+        if (!(path = PR_GetEnv("PWD")) && !(path = PR_GetEnv("HOME")))
+        if (path)

mozilla/gfx/src/xlib/nsDeviceContextSpecXlib.cpp
+      (void) pPrefs->SetBoolPref("print.selection_radio_enabled", isOn);
no need to (void). 

+        printService->GetMarginTop(&dtop);
+        printService->GetMarginLeft(&dleft);
+        printService->GetMarginBottom(&dbottom);
+        printService->GetMarginRight(&dright);
? perhaps a GetMargins call might be in order? [dcone?]

Furture Bug, 
+  PRUnichar *command        = nsnull;
+  PRUnichar *printfile      = nsnull;
should be converted to something that doesn't need nsMemory::Free()

mozilla/gfx/src/xprint/nsDeviceContextXP.cpp
@@ -49,9 +56,11 @@
use initializers?

please use:
classname::methodname [not: classname :: methodname]
if (cond) [not if(cond) or if ( cond )]

//you might consider this ugly if ()
nsFontXp*
nsFontMetricsXp::FindFont(PRUnichar aChar)
{
  nsFontXp* font;
#ifdef NS_FONT_DEBUG_CALL_TRACE
  if (gDebug & NS_FONT_DEBUG_CALL_TRACE) {
    printf("FindFont(%04X)[", aChar);
     for (PRInt32 i = 0; i < mFonts.Count(); i++) {
       printf("%s, ", mFonts.CStringAt(i)->get());
    }
    printf("]\nreturns ");
  }
#endif
  if (
     (font = FindUserDefinedFont(aChar))
  || (font = FindLocalFont(aChar))
  || (font = FindGenericFont(aChar))
  || (font = FindGlobalFont(aChar))
  || (font = FindSubstituteFont(aChar))
  )
#if NS_FONT_DEBUG_CALL_TRACE
  if (gDebug & NS_FONT_DEBUG_CALL_TRACE)
  {
    printf("%s\n", font->mName ? font->mName : "(substitute)");
  } else {
    printf("NULL\n");
  }
#else
  {};
#endif
  return font;
}

Comment 26

[Roland Mainz]

blizzard:
Wanna sr= that patch, please ? Thanks !

Comment 27

[Christopher Blizzard (:blizzard)]

+#if 0 /* this does not work for an unknown reason */
+    rv = CallQueryInterface(dcxp, aContext);
+#else

Why doesn't it work?  Please don't leave #if 0 code in if you can avoid it.

+    nsIDeviceContextSpecXp *tmp = this;

Need a cast there?

-  if (PR_FALSE == aQuiet ) {

Why are you removing that from the gtk code?  That's needed in case someone
doesn't want the dialog.  I'm pretty sure that's wrong.

+protected:
   virtual ~nsDeviceContextSpecXlib();

Why is that protected now?

+#undef USER_DEFINED
+#define USER_DEFINED "x-user-def"
+#define NOISY_FONTS 1

Are you sure you want that on by default?

Comment 28

[Roland Mainz]

> +#if 0 /* this does not work for an unknown reason */
> +    rv = CallQueryInterface(dcxp, aContext);
> +#else
> 
> Why doesn't it work?  Please don't leave #if 0 code in if you can avoid it.

Removed for now. For some reason the compiler did not accept that... ,-((
Fixed.

> +    nsIDeviceContextSpecXp *tmp = this;
> 
> Need a cast there?

In theory yes. But the code above and below used that "tmp" var, too. I don't
know _why_ that code uses that... but I thought it may be usefull to be
consistent with the other code above/below...

> -  if (PR_FALSE == aQuiet ) {
> 
> Why are you removing that from the gtk code?  That's needed in case someone
> doesn't want the dialog.  I'm pretty sure that's wrong.

xx!!@@@!!xx... the h*ll may know why this code got lost... sorry... 
Fixed.
 
> +protected:
>    virtual ~nsDeviceContextSpecXlib();
> 
> Why is that protected now?

Accident!?...
Fixed.

> +#undef USER_DEFINED
> +#define USER_DEFINED "x-user-def"
> +#define NOISY_FONTS 1
> 
> Are you sure you want that on by default?

Mhhh... AFAIK I didn't touch that code. Answer for know: YES - I have more work
to do in nsFontMetrics*.cpp - I'll kill that in one of my next patches. Should
be gone at the end of this month... :-)

And I fixed the license boilerplates of nsPrintOptionsXlib.* (new files) to make
timeless happy...

Going to file final patches...

Comment 29

[Roland Mainz]

Attachment: (Final) patch for 2001-05-14-latest-trunk

Comment 30

[Roland Mainz]

Attachment: Diff for review (same as attach_id=34774) but with -w -i to ignore whitespace/tab conversions and case changes (like s/XP/Xp/) REVIEW ONLY!!

Comment 31

[Roland Mainz]

blizzard:
Wanna sr= that patch, please ? Thanks !

Comment 32

[Roland Mainz]

Attachment: PostScript from Solaris 7 Xprt+todays patch printing http://www.zdnet.de/

Comment 33

[Roland Mainz]

I attached a DIN A4 PostScript file grabbed from print queue printed via Xprint
using todays patches (images are disabled because the scaling of images doesn't
work yet... yes... I am _working_ on that...)...

Any comments ?

Comment 34

[Christopher Blizzard (:blizzard)]

Original code:

-      if (command != nsnull && printfile != nsnull) {

New code:


+      if (command != nsnull) {
+        // ToDo: Use LocalEncoding instead of UTF-8 (see bug 73446)
+        strcpy(mPrData.command, NS_ConvertUCS2toUTF8(command).get());
+      }
+      if (printfile != nsnull) {
+        // ToDo: Use LocalEncoding instead of UTF-8 (see bug 73446)        
+        strcpy(mPrData.path, NS_ConvertUCS2toUTF8(printfile).get());


Please put the check back in for both since both the command and the print file
are needed.

Other than that sr=blizzard.

Comment 35

[Roland Mainz]

Attachment: Final final patch for 2001-05-16-08-trunk

Comment 36

[Roland Mainz]

Filed final final patch for checkin.

mkaply, wanna checkin this monster and mark the bug as "fixed" after checkin,
please ?
Note that the patch has two new files for Xlib-toolkit...

THANKS !!

Comment 37

[Mike Kaply [:mkaply]]

Fix checked in.