317450 - Print dialog should use FSG OpenPrinting API (PAPI) for queue enumeration and submission when available

Status: RESOLVED INVALID

(Core :: Printing: Output, enhancement)

Description

[Norm Jacobs]

User-Agent:       Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8) Gecko/20051117 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8) Gecko/20051117 Firefox/1.5

When running the browser/mail client/... on Solaris and bringing up the print
dialog on Solaris, I find that the printers available on my system are not
listed.
Recent versions of Solaris include an implementation of the Free Standards Group
OpenPrinting API (PAPI).  This API provides a "standard" interface for performing various printing related tasks. eg. queue (printer) enumeration, queue query, job submission, cancelation, etc.  In the short term, this API should be used to enumerate the list of available print queues when populating the printer list pulldown in the dialog box when it's available.  The API should also be used to submit the job to the print service when the user hits the print button.

A longer term goal should be to make use of the printer query support to gather capabilities of the printer and modify the dialog to reflect capabilities that the user can select for job submission.  eg. duplex, print quality, media/tray,
etc.

Reproducible: Always

Steps to Reproduce:
1. open "file" -> "print"
2. pull down the "Printer" selection pulldown
3. run "/usr/bin/lpget list" from the shell.
4. compare the lists of printers from 2 and 3.  THey will be different

Actual Results:  
The Printer selection pulldown only lists "PostScript/default" as the
available printer and "lpget list" enumerates several printers that I
can print to from my system.

Expected Results:  
I expect the dialog to contain the same set of printers as is available
from the command line or other desktop applications.  If the PAPI was available
on the system, it should have tried to enumerate a list of available printers using the API and presented it to me.

Comment 1

[Norm Jacobs]

Attachment: compressed tar file with diffs and new files that implement a fix for this bug

Attached is a compressed tar file that contains two new files
  gfx/src/psshared/nsPAPIShim.{cpp|h}
and diffs (papi.diffs) to be applied to
  gfx/src/ps/nsPrintJobFactoryPS.cpp
  gfx/src/ps/nsPrintJobPS.cpp
  gfx/src/ps/nsPrintJobPS.h
  gfx/src/psshared/Makefile.in
  gfx/src/psshared/nsPSPrinters.cpp
  gfx/src/psshared/nsPSPrinters.h

These changes allow the print dialog to use the PAPI for print queue enumeration and job submission if the PAPI (libpapi.so) is found on the system.  At this point, I have only built and tested this on Solaris Nevada build 27a / Solaris Express

Comment 2

[Kenneth Herron]

Comment on attachment 203973 [details]
compressed tar file with diffs and new files that implement a fix for this bug

Norm, this is great. I was expecting to do an lp version of the patch from bug 135695, but this should produce much better results.

To start with, you should submit patches as an uncompressed text attachment instead of attaching a tarfile. If you "cvs add" the new files, then they'll be included in the patch, but this requires cvs write access. If you don't have that you can use "cvsdo" from <http://www.red-bean.com/cvsutils/> to add them just to your system. Alternately, some people attach new files as separate attachments.

Second, I don't think the copyright notices on the new files are accurate. I realize you probably copied those files from the CUPS shim code, but you should list yourself as the original author. See <http://www.mozilla.org/MPL/license-policy.html> for mozilla's policy and the copyright template. If you need to depart from that template--in particular, the statement "Copyright 2004 Sun Microsystems, Inc. All Rights Reserved. Use is subject to license terms." in nsPAPIShim.cpp, then I expect one of the mozilla drivers will have to get involved.

Third, you need to replace the tab characters with spaces and use consistent indention. Most mozilla code uses two-space indents. Personally I prefer something bigger which is why the CUPS shim code uses 4-space indents. nsPAPIShim.cpp uses a combination of 4-space and 8-space or tab indents. And ideally, the "mode" and "ex" lines at the top of the file should match whatever you settle on.

Also, could you add a little explanation of what PAPI is? A sentence or two and a link to the PAPI home page would be fine. The PAPI shim code could also contain a link to the PAPI documentation.

nsPAPIShim.cpp:

1) Line 47 still contains a reference to libcups.

2) In nsPAPIShim::PrintFile, is it mandatory to pass a number of copies to PAPI? With other printing backends, a copy count of 1 (from the print dialog) means don't specify a copy count to the printing backend, and let it use its default. See the description for SetNumCopies() in nsIPrintJobPS.h.

From the patch:

1) nsPrintJobFactoryPS::CreatePrintJob(), could you convert the code to use a switch instead of if/else if/else? If you look at the patch on bug 135695, you'll see what I mean.

2) In psshared/nsPSPrinters.cpp, you show a change within nsPSPrinterList::Enabled().  This change has already been committed to CVS.

3) In nsPrintJobPS.h, you define nsPrintJobPAPI as inheriting from nsPrintJobFilePS. I know several other classes do this, but I've come to think it's not a particularly good design; it's only done to get the mDestHandle and mDestination fields, which are used for different purposes in different classes. I'd like to avoid doing any more of that where it doesn't make sense. In this case, the PAPI code doesn't even use mDestination, so I think it'd be better to just inherit from nsIPrintJobPS and define whatever members you need.

4) In nsPrintJobPS.cpp nsPrintJobPAPI::FinishSubmission(), you call the functions to set the MOZ_PRINTER_NAME environment variable. Is this necessary? I was hoping we could leave that variable as an "implementation detail" of the old run-an-opaque-command printing backend and not extend it to other methods.

5) In nsPSPrinters.h, you include nsPAPIShim.h and also forward-declare the nsPAPIShim class. I suspect both of these aren't necessary. (I see the existing code does the same for nsCUPSShim; could you fix that?)

Comment 3

[Norm Jacobs]

Attachment: updated proposed patch

This attachment obsoletes the previous fix contained in the compressed tar file.  It should address all of the code review items from [e-mail]

Comment 4

[Norm Jacobs]

Attachment: updated patch with sorted papi printers list

After using my copy of Firefox with this fix applied in a network with a large
number of printers, I decided that the list of PAPI generated list of printers should be sorted to make it easier to search through.

Comment 5

[Kenneth Herron]

Comment on attachment 204457 [details] [diff] [review]
updated patch with sorted papi printers list

Hi, Norm,

>+nsPrintJobPAPI::FinishSubmission()
>+{
>+ [...]
>+    if (NS_SUCCEEDED(rv)) {
>+        /* Set up the environment. */
>+        if (PR_SUCCESS != EnvLock())
>+            return NS_ERROR_OUT_OF_MEMORY;
>+
>+        char *title = mJobTitle.IsVoid() ?
>+                        "Untitled Document" : mJobTitle.get();
>+
>+        int result = (mPAPI.PrintFile)((char *)mPrinterName.get(),
>+                                       (char *)printFileName.get(),
>+                                       mNumCopies, title);
>+
>+        /* Clean up */
>+        EnvClear();

EnvLock() and EnvClear() are used by the print-to-pipe class because that class manipulates the environment while printing and two simultaneous jobs would interfere with each other. You're not setting any environment variables here, so I'm not sure why you're calling EnvLock() unless the PAPI library needs it.

Also, do you need the parentheses around mPAPI.PrintFile? The analogous code in the CUPS backend uses that syntax because it's calling through a function pointer, but PrintFile is just a regular member function, isn't it?


>Index: ps/nsPrintJobPS.h
>+class nsPrintJobPAPI : public nsIPrintJobPS {
>+    public:
>+        nsresult StartSubmission(FILE **aHandle);
>+        nsresult FinishSubmission();
>+        nsresult SetNumCopies(int aNumCopies);
>+        void SetJobTitle(const PRUnichar *aTitle);
>+
>+    protected:
>+        nsresult Init(nsIDeviceContextSpecPS *);
>+        void SetDestHandle(FILE *aHandle) { mDestHandle = aHandle; }
>+        FILE *GetDestHandle() { return mDestHandle; }

Do you really need SetDestHandle() and GetDestHandle() here? I doubt anyone is ever going to inherit from this. The implementation should just access mDestHandle directly.


>Index: psshared/nsPAPIShim.cpp
>+int
>+nsPAPIShim::PrintFile(char *printer, char *file, int copies, char *title)
>+{

Mozilla uses a coding standard that function arguments are named with a leading 'a' followed by a capital: aPrinter, aCopies, etc.

>+    if (copies < 0)
>+        copies = 1;
>+
>+    mPapiAttributeListAddString(&attributes, PAPI_ATTR_EXCL, "job-name", title);
>+    if (copies != 0) 
>+        mPapiAttributeListAddInteger(&attributes, PAPI_ATTR_EXCL,
>+                "copies", copies);

Can you avoid setting this attribute when the copy count is 1? NsPrintJobPAPI::SetNumCopies() will always be called with the copy count from the print dialog. A value of 1 is interpreted as using the default for the print queue, rather than forcing one copy. Your code in NsPrintJobPAPI initializes the copy count to 0, but the SetNumCopies() function will always change it to some other value so the copies argument here will never be 0.


>Index: psshared/nsPSPrinters.h
>@@ -77,26 +78,28 @@ class NS_PSSHARED nsPSPrinterList {
>          *              There should always be at least one entry. The
>          *              first entry is the default print destination.
>          */
>         void GetPrinterList(nsCStringArray& aList);
> 
>         enum PrinterType {
>             kTypeUnknown,         // Not actually handled by the PS module
>             kTypePS,              // Generic postscript module printer
>-            kTypeCUPS             // CUPS printer
>+            kTypeCUPS,             // CUPS printer
>+            kTypePAPI             // PAPI printer
>         };

Nit: Could you fix the kTypeCUPS comment to line up with the others?

Comment 6

[Norm Jacobs]

Attachment: updated patch incorporating code review comments

This is the latest version of the patch, which incorporates changes due to code review comments.  The changes are as follows:
   * The unnecessary use of EnvLock/Clear() has been removed.
   * Get/SetDestHandle() have been been removed from the interface and replaced
     with direct access to mDestHandle.
   * copies has been initialized to 0 and the "copies" job attribute is only
     set if (copies > 1)
   * nsPAPIShim::PrintFile() arguments have been renamed to a(Argument) form
     to match the mozilla coding standard.
   * parenthesis around mPAPI.PrintFile call have been removed in
     nsPrintJobPAPI::FinishSubmission()
   * fixed comment alignment in nsPSPrinters.h

As I state above, I modified the code dealing with "copies", so that a value
of 1 (or less) means, use the service default.  This may be the way the rest of the code that interacts with print services works in Mozilla, I haven't looked into it further.  The only real issue with doing it this way is that there is
no longer a way to specify that you want only 1 copy when the service default
is some other value.  It may be more trouble than it's worth, but the dialog should probably initialize the "copies" field to the print service default and
not overloading the value of 1.

Comment 7

[Leon Sha]

Attachment: Patch v5

This patch is base on Norm's patch. The only thing that I changed is put the sort to another place. Norm agreed that I can work on this base on his work.

Comment 8

[Leon Sha]

Comment on attachment 226772 [details] [diff] [review]
Patch v5

There is no respnose from KH for a long time. Change the reviewer to ROC.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

kherron was just around on IRC today. Would you mind emailing him and asking him for another review here?

One thing is, I would like a configure option to disable building PAPI. For distributions that depend on CUPS, there is no need to build this code.

Comment 10

[Leon Sha]

Attachment: patch_v6

Add a build option.

Comment 11

[Kenneth Herron]

Comment on attachment 266872 [details] [diff] [review]
patch_v6

>Index: ps/nsPrintJobFactoryPS.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/ps/nsPrintJobFactoryPS.cpp,v
>retrieving revision 1.4
>diff -u -p -8 -r1.4 nsPrintJobFactoryPS.cpp
>--- ps/nsPrintJobFactoryPS.cpp	7 Sep 2004 17:53:15 -0000	1.4
>+++ ps/nsPrintJobFactoryPS.cpp	31 May 2007 09:11:13 -0000

Leon, is this patch incomplete? It looks like it was made from the gfx/src directory, and only contains changes to gfx/src/ps and gfx/src/psshared. The code in gfx/src/ps is obsolete; I wasn't aware you could even still build mozilla to use it.

Some of the code in gfx/src/psshared is still being used, but it should be moved somewhere else so that both of these directories can be removed. I've opened bug 383159 about that, and marked it as blocking this bug, because it would impact the files you're adding to the psshared directory.

Comment 12

[Leon Sha]

My mistake.
This patch is doing the similar thing as cups did.
For current trunk only gfx/src/psshared is needed.
Since there will be some changes on that directory, I'll do another patch after that.
Thank you for your help.
(In reply to comment #11)
> (From update of attachment 266872 [details] [diff] [review])
> >Index: ps/nsPrintJobFactoryPS.cpp
> >===================================================================
> >RCS file: /cvsroot/mozilla/gfx/src/ps/nsPrintJobFactoryPS.cpp,v
> >retrieving revision 1.4
> >diff -u -p -8 -r1.4 nsPrintJobFactoryPS.cpp
> >--- ps/nsPrintJobFactoryPS.cpp	7 Sep 2004 17:53:15 -0000	1.4
> >+++ ps/nsPrintJobFactoryPS.cpp	31 May 2007 09:11:13 -0000
> 
> Leon, is this patch incomplete? It looks like it was made from the gfx/src
> directory, and only contains changes to gfx/src/ps and gfx/src/psshared. The
> code in gfx/src/ps is obsolete; I wasn't aware you could even still build
> mozilla to use it.
> 
> Some of the code in gfx/src/psshared is still being used, but it should be
> moved somewhere else so that both of these directories can be removed. I've
> opened bug 383159 about that, and marked it as blocking this bug, because it
> would impact the files you're adding to the psshared directory.
> 

Comment 13

[Leon Sha]

Since the gnome print dialog is used, this bug is no longer valid.