185707 - [Qt] Get printing working in Qt toolkit

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Roland Mainz]

Get printing working in Qt toolkit

Comment 1

[Roland Mainz]

Taking myself...

Comment 2

[Roland Mainz]

Attachment: Patch to get both Xprint and PostScript modules working again (no QPrinter support yet)

Comment 3

[Robert Kaiser]

Comment on attachment 109474 [details] [diff] [review]
Patch to get both Xprint and PostScript modules working again (no QPrinter support yet)

the patch does not apply cleanly to current trunk :(

Comment 4

[Roland Mainz]

Comment on attachment 109474 [details] [diff] [review]
Patch to get both Xprint and PostScript modules working again (no QPrinter support yet)

It applies to my trunk build cleanly... ;-/

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

note, timeless(@bemail.org) should review such patches, as he is the owner of
the qt port...

Comment 6

[timeless]

Comment on attachment 109474 [details] [diff] [review]
Patch to get both Xprint and PostScript modules working again (no QPrinter support yet)

+NS_IMETHODIMP nsDeviceContextSpecFactoryQT::CreateDeviceContextSpec(nsIWidget
*aWidget,
+								    
nsIPrintSettings* aPrintSettings,
+								    
nsIDeviceContextSpec *&aNewSpec,
+								     PRBool
aIsPrintPreview)
 {
+  nsresult rv;
+  static NS_DEFINE_CID(kDeviceContextSpecCID, NS_DEVICE_CONTEXT_SPEC_CID);
+  nsCOMPtr<nsIDeviceContextSpec> devSpec =
do_CreateInstance(kDeviceContextSpecCID, &rv);
i think this can be an early return:
+  if (NS_SUCCEEDED(rv))
-- if there's some reason not to early return, please explain :)
+  {
use NS_..._CAST: (you've used it elsewhere)
+    rv = ((nsDeviceContextSpecQT *)devSpec.get())->Init(aPrintSettings);

- * Portions created by the Initial Developer are Copyright (C) 1999
+ * Portions created by the Initial Developer are Copyright (C) 1998
this is just strange, please explain.

ah, well
-#ifndef nsDeviceContextSpecFactoryQT_h___
-#define nsDeviceContextSpecFactoryQT_h___
+#ifndef nsDeviceContextSpecFactoryX_h___
+#define nsDeviceContextSpecFactoryX_h___
please don't change the guard, it's confusing and wrong.

personally i don't see a reason to remove all of the debugging bits that we've
grown, we can remove them later.

-#include "nsIPrefBranch.h"
-#include "nsIPrefService.h"
+#include "nsIPref.h"
caillon will kill you. we're moving from nsIPref to nsIPrefBranch, don't undo
his work. if you need help with nsIPrefBranch you can contact caillon on irc.
note that i'd really like for the pref code you write to ctually use
prefbranches instead of just relying on the root branch, it will make the code
much cleaner.

+/* Ensure that the result is always equal to either PR_TRUE or PR_FALSE */
+#define MAKE_PR_BOOL(val) ((val)?(PR_TRUE):(PR_FALSE))
this is silly, and since i'm the module owner i can refuse to accept it. you
can use (PRBool)foo if you want to explicitly indicate that you're treating foo
as a boolean, and you should use if (foo) or if (!foo) to test the PRBool.
Actually, it doesn't even appear to be used...

+// then it will never be delete, so this class takes care of that.
delete=>deleted


instead of using NS_IMPL_ISUPPORTSx w/ lots of #ifdef, look into using the
INTERFACEMAP.

+NS_IMETHODIMP nsPrinterEnumeratorQT::EnumeratePrinters(PRUint32* aCount,
PRUnichar*** aResult)

+  NS_ENSURE_ARG(aCount);
+  NS_ENSURE_ARG_POINTER(aResult);
+
+  if (aCount) 
+    *aCount = 0;
+  else 
+    return NS_ERROR_NULL_POINTER;
change that first line to NS_ENSURE_ARG_POINTER and remove the else+return
(which weren't reachable anyway).
remove the else+return for aResult too.

+  nsresult rv = GlobalPrinters::GetInstance()->InitializeGlobalPrinters();
...
+  PRInt32 numPrinters = GlobalPrinters::GetInstance()->GetNumPrinters();
wouldn't it make some sense to cache GetInstance() function calls aren't free.

+  NS_ENSURE_ARG_POINTER(aPrinterName);
+  NS_ENSURE_ARG_POINTER(aPrintSettings);
+  
+  NS_ENSURE_TRUE(*aPrinterName, NS_ERROR_FAILURE);
+  NS_ENSURE_TRUE(aPrintSettings, NS_ERROR_FAILURE);
the second check of aPrintSettings is redundant (remove it)

+    const char *path;
+  
+    if (!(path = PR_GetEnv("PWD")))
+      path = PR_GetEnv("HOME");
PR_GetEnv is actually expensive on some platforms, please make path a static
and check to see if you've already picked one.

+    return NS_OK;    
+  }
+  else
+#endif /* USE_XPRINT */
you don't need the else.

+  PRBool allocate = (GlobalPrinters::GetInstance()->PrintersAreAllocated() ==
PR_FALSE);
do this instead:
+  PRBool allocate = !(GlobalPrinters::GetInstance()->PrintersAreAllocated());

I don't see particularly good reasons to use int instead of PRInt_.

timeless@boffo:~$ patch -p0 --dry-run <xpr
patching file mozilla/gfx/src/qt/nsDeviceContextQT.cpp
patching file mozilla/gfx/src/qt/nsDeviceContextSpecFactoryQT.cpp
Hunk #3 FAILED at 39.
1 out of 3 hunks FAILED -- saving rejects to file
mozilla/gfx/src/qt/nsDeviceContextSpecFactoryQT.cpp.rej
patching file mozilla/gfx/src/qt/nsDeviceContextSpecFactoryQT.h
patching file mozilla/gfx/src/qt/nsDeviceContextSpecQT.cpp
Hunk #3 FAILED at 36.
Hunk #4 succeeded at 324 (offset 4 lines).
1 out of 4 hunks FAILED -- saving rejects to file
mozilla/gfx/src/qt/nsDeviceContextSpecQT.cpp.rej
patching file mozilla/gfx/src/qt/nsDeviceContextSpecQT.h

Comment 7

[Robert Kaiser]

btw, I saw exactly the same chunks failing when I tried to apply the patch.

Roland, I guess you really have used an old version, which is missing the pref
changes as well :(

Comment 8

[Roland Mainz]

Attachment: Patch for 2002-12-16-08-trunk

Note that this patch is a 1:1 copy from the matching files from gfx/src/xlib/.
I am just copying the files and doing a s/Xlib/QT/ here. Any further change
must be ported to the other Unix toolkits to keep them in sync (which will
require r=/sr=/a=/moa=/dancing_dog=/goat= and whatever evil is out there, too).

Comment 9

[Roland Mainz]

Comment on attachment 109532 [details] [diff] [review]
Patch for 2002-12-16-08-trunk

timeless:
Can we get the current	patch "in" to get Qt printing glue in gfx/ _working_
and then cleanup it after we migrated all copies in gfx/src/gtk, gfx/src/xlib
and gfx/src/qt into gfx/src/unixshared (otherwise we would have to maintain
three copies of the same code), please ?

Comment 10

[timeless]

Comment on attachment 109532 [details] [diff] [review]
Patch for 2002-12-16-08-trunk

... waiting for something better doesn't help anyone

Comment 11

[timeless]

checked in .. please hurry up and unify this stuff

Comment 12

[Roland Mainz]

Marking bug as FIXED - this should be working now (code does not differ in
functionality from other platforms, we still have to verify this once menus are
working again...).