279298 - Support for CUPS printer instances

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Mårten Svantesson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041113 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041113 Firefox/1.0

Opening the printer dialog while having CUPS support, you can choose from a list
of you printers. But some printerns are duplicated. I realized that the extra
duplacates were the configured instances of the printers. But the instance names
are not shown.

Reproducible: Always

Steps to Reproduce:
1.Run Firefox with Postscript prinitng and CUPS installed.
2.Open Print dialog


Actual Results:  
Printers with specified instances are duplicated in printer menu, but instance
names are not shown.

Expected Results:  
Instance names are shown in printer menu and when choosing an instance you will
print to it correctly.

Looking at the code the simplest option would be to no include the instances in
the menu. But on the other hand it would only take about 10 rows of code to
support printing to instances.

The code for adding CUPS printers to the printer menu lives in
gfx/src/psshared/nsPSPrinters.cpp
and the code for the actual printing is in gfx/src/ps/nsPrintJobPS.cpp

Comment 1

[Mårten Svantesson]

Attachment: Draft patch

It seems to me that this should work, but I haven't tested it yet.

Comment 2

[Mårten Svantesson]

Attachment: Tested and fixed

Now the patch is tested and fixed. (No, it didn't work directly...)

I don't really like the efficiency of CupsGetDests beeing called in both
nsPSPrinterList::GetPrinterList and nsPrintJobCUPS::FinishSubmission, but I
don't see where you should store the dests in between. I don't want to model
after how xprint is doing it. (It seems redicously cumbersome to maintain and
it's ugly.)

I used '/' as the name/instance delimiter since though it's not officially
safe, it is used in the lpr/lp interface to CUPS.

Comment 3

[Kenneth Herron]

Comment on attachment 172586 [details] [diff] [review]
Tested and fixed

>I don't really like the efficiency of CupsGetDests beeing called in
>both nsPSPrinterList::GetPrinterList and nsPrintJobCUPS::FinishSubmission, 
>but I don't see where you should store the dests in between.

Not to mention the fact we're loading the CUPS library for the print dialog,
unloading it, then reloading it to print :-).

>@@ -356,18 +356,21 @@ nsPrintJobCUPS::Init(nsIDeviceContextSpe
> 
>     NS_ENSURE_TRUE(mCups.Init(), NS_ERROR_NOT_INITIALIZED);
> 
>     /* Printer name */
>     const char *printerName = nsnull;
>     aSpec->GetPrinterName(&printerName);
>     NS_ENSURE_TRUE(printerName, NS_ERROR_GFX_PRINTER_NAME_NOT_FOUND);
> 
>-    const char *slash = strchr(printerName, '/');
>-    mPrinterName = slash ? slash + 1 : printerName;
>+    if (strncmp(NS_CUPS_DRIVER_NAME, printerName, 
>+                NS_CUPS_DRIVER_NAME_LEN) == 0) {
>+        mPrinterName = printerName + NS_CUPS_DRIVER_NAME_LEN;
>+    }

Is this change necessary? It seems to me the old code is fine here, and if you
don't reference NS_CUPS_* here there's no reason to move them into
nsIDeviceContextSpecPS.h. But see the next comment...

>     return NS_OK;
> }
> 
> nsresult
> nsPrintJobCUPS::StartSubmission(FILE **aHandle)
> {
>     NS_ENSURE_TRUE(mCups.IsInitialized(), NS_ERROR_NOT_INITIALIZED);
> 
>@@ -394,20 +397,35 @@ nsresult
> nsPrintJobCUPS::FinishSubmission()
> {
>     NS_ENSURE_TRUE(mCups.IsInitialized(), NS_ERROR_NOT_INITIALIZED);
>     NS_PRECONDITION(GetDestHandle(), "No destination file handle");
>     NS_PRECONDITION(!GetDestination().IsEmpty(), "No destination");
> 
>     fclose(GetDestHandle());
>     SetDestHandle(nsnull);
>-
>-    int result = (mCups.mCupsPrintFile)(mPrinterName.get(),
>-            GetDestination().get(), "Mozilla print job", 0, nsnull);
>+    
>+    char* printerName = strdup(mPrinterName.get());
>+    char *slash = strchr(printerName, '/');
>+    char* instance = NULL;

You can call BeginWriting() here instead of get() to gain writable access to
mPrinterName. Another way to do this would be to replace mPrinterName with an
nsCStringArray, and use nsCStringArray::ParseString() during Init() to split
the string into driver, destination, and instance all at once.


>+    cups_dest_t *dest = (mCups.mCupsGetDest)(printerName, instance, num_dests, dests);
>+    int result = (mCups.mCupsPrintFile)(printerName,
>+                                    GetDestination().get(), "Mozilla print job", 
>+                                    dest->num_options, dest->options);

You need to allow for the possibility that cupsGetDest() might return NULL if
the printer isn't in the list.


>+++ psshared/nsCUPSShim.cpp	27 Jan 2005 19:08:20 -0000
>@@ -44,16 +44,17 @@
> // List of symbols to find in libcups. Must match symAddr[] defined in Init().
> // Making this an array of arrays instead of pointers allows storing the
> // whole thing in read-only memory.
> static const char gSymName[][sizeof("cupsPrintFile")] = {
>     { "cupsGetDests" },
>     { "cupsFreeDests" },
>     { "cupsPrintFile" },
>     { "cupsTempFd" },
>+    { "cupsGetDest" },
> };

This is nitpicky, but here and in the other places where you're adding the new
symbol, could you keep them in alphabetical order?


>+++ psshared/nsPSPrinters.cpp	27 Jan 2005 19:08:20 -0000
>@@ -46,20 +46,16 @@
> #include "nsPrintfCString.h"
> #include "nsPSPrinters.h"
> #include "nsReadableUtils.h"        // StringBeginsWith()
> 
> #include "prlink.h"
> #include "prenv.h"
> #include "plstr.h"
> 
>-#define NS_CUPS_PRINTER "CUPS/"
>-#define NS_CUPS_PRINTER_LEN (sizeof(NS_CUPS_PRINTER) - 1)

Assuming these macros remain private to this file, I'd prefer to avoid renaming
them, just to reduce the number of lines being changed.

Comment 4

[Kenneth Herron]

Mårten, this feature would be a nice improvement, but we should try to get it in
before the next beta (in mid-march) if it's going to make 1.8. Do you plan to
submit a new patch, or should someone else give it a try?

Comment 5

[Mårten Svantesson]

(In reply to comment #4)
> Mårten, this feature would be a nice improvement, but we should try to get it in
> before the next beta (in mid-march) if it's going to make 1.8. Do you plan to
> submit a new patch, or should someone else give it a try?

I've been trying to work on a new patch, but after updating my tree from CVS and
rebuilding firefox wont start. :( Not even when downloading a fresh tarball,
updating that and building.  (Entered bug 283130 about it.)

Anyway I collected some comments about your review that I intended to send in
with patch. Here they are:

(In reply to comment #3)
> (From update of attachment 172586 [details] [diff] [review] [edit])
> >I don't really like the efficiency of CupsGetDests beeing called in
> >both nsPSPrinterList::GetPrinterList and nsPrintJobCUPS::FinishSubmission, 
> >but I don't see where you should store the dests in between.
> 
> Not to mention the fact we're loading the CUPS library for the print dialog,
> unloading it, then reloading it to print :-).

Gah, missed that one. Not that it is in the scope of this bug, but is
there any coherent work on fixing things like this? I've seen comments
scattered around bugzilla, but it might be suitable to start
scribbling somewhere at wiki.mozilla.org. Somebody has to do it
though... On to the issues at hand:

> >@@ -356,18 +356,21 @@ nsPrintJobCUPS::Init(nsIDeviceContextSpe
> > 
> >     NS_ENSURE_TRUE(mCups.Init(), NS_ERROR_NOT_INITIALIZED);
> > 
> >     /* Printer name */
> >     const char *printerName = nsnull;
> >     aSpec->GetPrinterName(&printerName);
> >     NS_ENSURE_TRUE(printerName, NS_ERROR_GFX_PRINTER_NAME_NOT_FOUND);
> > 
> >-    const char *slash = strchr(printerName, '/');
> >-    mPrinterName = slash ? slash + 1 : printerName;
> >+    if (strncmp(NS_CUPS_DRIVER_NAME, printerName, 
> >+                NS_CUPS_DRIVER_NAME_LEN) == 0) {
> >+        mPrinterName = printerName + NS_CUPS_DRIVER_NAME_LEN;
> >+    }
> 
> Is this change necessary? It seems to me the old code is fine here, and if you
> don't reference NS_CUPS_* here there's no reason to move them into
> nsIDeviceContextSpecPS.h. 

It depends on the interpretation of the old code. I interpreted it as
"The printername might have a driver name prepended, in that case get
rid of it.". With the possibility of an instance name appended as well
the old code wouldn't mean that anymore (but instead "Get rid of the
prepended driver name if it exists, otherwise get rid of the printer
name."). On the other as the other code looks now the change isn't
neccessary, no.

The prepended driver name seems like a kludge, that's why I
interpreted the code as I did.

> >     return NS_OK;
> > }
> > 
> > nsresult
> > nsPrintJobCUPS::StartSubmission(FILE **aHandle)
> > {
> >     NS_ENSURE_TRUE(mCups.IsInitialized(), NS_ERROR_NOT_INITIALIZED);
> > 
> >@@ -394,20 +397,35 @@ nsresult
> > nsPrintJobCUPS::FinishSubmission()
> > {
> >     NS_ENSURE_TRUE(mCups.IsInitialized(), NS_ERROR_NOT_INITIALIZED);
> >     NS_PRECONDITION(GetDestHandle(), "No destination file handle");
> >     NS_PRECONDITION(!GetDestination().IsEmpty(), "No destination");
> > 
> >     fclose(GetDestHandle());
> >     SetDestHandle(nsnull);
> >-
> >-    int result = (mCups.mCupsPrintFile)(mPrinterName.get(),
> >-            GetDestination().get(), "Mozilla print job", 0, nsnull);
> >+    
> >+    char* printerName = strdup(mPrinterName.get());
> >+    char *slash = strchr(printerName, '/');
> >+    char* instance = NULL;
> 
> You can call BeginWriting() here instead of get() to gain writable access to
> mPrinterName. Another way to do this would be to replace mPrinterName with an
> nsCStringArray, and use nsCStringArray::ParseString() during Init() to split
> the string into driver, destination, and instance all at once.

Replacing mPrinterName for CUPS but keeping it for the other drivers
seems a bit unclean. I stay with a minimal patch if that's fine with you.

> >+    cups_dest_t *dest = (mCups.mCupsGetDest)(printerName, instance,
num_dests, dests);
> >+    int result = (mCups.mCupsPrintFile)(printerName,
> >+                                    GetDestination().get(), "Mozilla print
job", 
> >+                                    dest->num_options, dest->options);
> 
> You need to allow for the possibility that cupsGetDest() might return NULL if
> the printer isn't in the list.
> 
> 
> >+++ psshared/nsCUPSShim.cpp	27 Jan 2005 19:08:20 -0000
> >@@ -44,16 +44,17 @@
> > // List of symbols to find in libcups. Must match symAddr[] defined in Init().
> > // Making this an array of arrays instead of pointers allows storing the
> > // whole thing in read-only memory.
> > static const char gSymName[][sizeof("cupsPrintFile")] = {
> >     { "cupsGetDests" },
> >     { "cupsFreeDests" },
> >     { "cupsPrintFile" },
> >     { "cupsTempFd" },
> >+    { "cupsGetDest" },
> > };
> 
> This is nitpicky, but here and in the other places where you're adding the new
> symbol, could you keep them in alphabetical order?

Not per se, since they are not in alphabetical order now. I can fix that though.

> >+++ psshared/nsPSPrinters.cpp	27 Jan 2005 19:08:20 -0000
> >@@ -46,20 +46,16 @@
> > #include "nsPrintfCString.h"
> > #include "nsPSPrinters.h"
> > #include "nsReadableUtils.h"        // StringBeginsWith()
> > 
> > #include "prlink.h"
> > #include "prenv.h"
> > #include "plstr.h"
> > 
> >-#define NS_CUPS_PRINTER "CUPS/"
> >-#define NS_CUPS_PRINTER_LEN (sizeof(NS_CUPS_PRINTER) - 1)
> 
> Assuming these macros remain private to this file, I'd prefer to avoid renaming
> them, just to reduce the number of lines being changed.

Certainly. 

Comment 6

[Mårten Svantesson]

Attachment: Patch fixed according to review

I've reduced the size of the patch and also changed from C string.h functions
to Mozilla string functions. The order of functions in nsCUPSShim.* were
changed to be "more" alphabetical, without increasing number of rows in patch.

I noticed that the a non OK return value of FinishSubmission never is
propageted to the user, Mozilla just fails silently. But that isn't in the
scope of this bug I believe.

Comment 7

[Mårten Svantesson]

Comment on attachment 175952 [details] [diff] [review]
Patch fixed according to review

I realised that my comment
> Replacing mPrinterName for CUPS but keeping it for
> the other drivers
seems a bit unclean.

is nonsense. On the other hand this patch is tested and working, so I see no
reason to change it now.

Comment 8

[Kenneth Herron]

Comment on attachment 175952 [details] [diff] [review]
Patch fixed according to review

Sorry about that "alphabetical order" bit. For some reason I was reading
"mCupsGetDests" and thinking "D"...

>--- ps/nsPrintJobPS.cpp	7 Sep 2004 17:53:15 -0000	1.4
>+++ ps/nsPrintJobPS.cpp	1 Mar 2005 18:19:46 -0000
>@@ -400,14 +400,37 @@
>     fclose(GetDestHandle());
>     SetDestHandle(nsnull);
> 
>-    int result = (mCups.mCupsPrintFile)(mPrinterName.get(),
>-            GetDestination().get(), "Mozilla print job", 0, nsnull);
>+    nsCStringArray* printer = new nsCStringArray;
>+    printer->ParseString(mPrinterName.get(),"/");

It looks like "printer" could just be a stack variable, rather than being
dynamically allocated. Also, nsCStringArray has a ctor which lets you specify
the expected size:

    nsCStringArray printer(3);

Other than that, this looks really nice. Thanks for submitting this.

Comment 9

[Kenneth Herron]

Checked in:

Checking in ps/nsPrintJobPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPrintJobPS.cpp,v  <--  nsPrintJobPS.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in psshared/nsCUPSShim.cpp;
/cvsroot/mozilla/gfx/src/psshared/nsCUPSShim.cpp,v  <--  nsCUPSShim.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in psshared/nsCUPSShim.h;
/cvsroot/mozilla/gfx/src/psshared/nsCUPSShim.h,v  <--  nsCUPSShim.h
new revision: 1.3; previous revision: 1.2
done
Checking in psshared/nsPSPrinters.cpp;
/cvsroot/mozilla/gfx/src/psshared/nsPSPrinters.cpp,v  <--  nsPSPrinters.cpp
new revision: 1.3; previous revision: 1.2
done

Resolving FIXED. Thanks for the patch, Mårten.

Comment 10

[timeless]

*** Bug 290168 has been marked as a duplicate of this bug. ***