User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060119 Firefox/1.5 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060119 Firefox/1.5 Only standard paper sizes are available in the print dialog, which can prevent printing entirely on printers (e.g. label printers) without any of these paper sizes. When using CUPS, the actual paper sizes for each printer can be obtained, e.g. in the nsPaperPS object, and displayed for the user. I'll attach 2 patches that attempt to do 4 things (sorry!) 1a) Add CUPS paper sizes for GTK and PostScript (tested). 1b) It seems that some of the preference usage in for instance nsDeviceContextSpecG.cpp is not in sync with current preference settings; I've tried to remedy that, too. 1c) When interacting directly with CUPS (in nsPrintJobCUPS), the page size is transmitted (print command is not used in this case, and can thus NOT transfer information on media size!) 2) Add CUPS paper sizes for Qt (UNTESTED!) Reproducible: Always Steps to Reproduce: 1. Using CUPS, open print dialog 2. Click "Properties..." 3. Click "Paper Size:" Actual Results: Only the 6 standard paper sizes Expected Results: All paper sizes supported by the current printer, as specified by CUPS

Patch 1a) adding support for importing paper sizes from CUPS into GTK and PostScript printing 1b) fixing some preference names that seemed to be out of sync with present-day preference usage 1c) adding support for sending the paper size directly to CUPS when printing

Untested patch adding support for importing paper sizes from CUPS into Qt. Note that it depends on patch 209020.

Comment on attachment 209020 [details] [diff] [review] Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing I have some quick comments on this patch. >--- gfx/src/gtk/nsDeviceContextGTK.cpp 23 Aug 2005 02:11:31 -0000 1.132 >+++ gfx/src/gtk/nsDeviceContextGTK.cpp 19 Jan 2006 22:14:03 -0000 >@@ -573,18 +573,45 @@ NS_IMETHODIMP nsDeviceContextGTK::GetDev > // default/PS > static NS_DEFINE_CID(kCDeviceContextPS, NS_DEVICECONTEXTPS_CID); > > // Create a Postscript device context > nsCOMPtr<nsIDeviceContextPS> dcps(do_CreateInstance(kCDeviceContextPS, &rv)); > NS_ASSERTION(NS_SUCCEEDED(rv), "Couldn't create PS Device context."); > if (NS_FAILED(rv)) > return NS_ERROR_GFX_COULD_NOT_LOAD_PRINT_MODULE; >+ >+ const char* aPrinterName; >+ rv = spec->GetPrinterName(&aPrinterName); >+ if (NS_FAILED(rv)) >+ rv = dcps->SetSpec(aDevice); >+ else { >+ nsXPIDLCString >+ fullPrinterName, /* Full name of printer incl. driver-specific prefix */ >+ printerName; /* "Stripped" name of printer */ >+ fullPrinterName.Assign(aPrinterName); >+ printerName.Assign(aPrinterName); > >- rv = dcps->SetSpec(aDevice); >+ PrintMethod type = pmInvalid; >+ rv = nsDeviceContextSpecGTK::GetPrintMethod(printerName, type); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ /* "Demangle" postscript printer name */ >+ if (type == pmPostScript) { >+ /* Strip the printing method name from the printer, >+ * e.g. turn "PostScript/foobar" to "foobar" */ >+ PRInt32 slash = printerName.FindChar('/'); >+ if (kNotFound != slash) >+ printerName.Cut(0, slash + 1); >+ } >+ >+ rv = dcps->SetSpec(aDevice, fullPrinterName.get (), printerName.get ()); >+ } >+ None of this should be necessary. First of all, the format of postscript-module printer names is a private implementation detail of the PS module. It shouldn't be necessary to embed PS name-parsing rules into other classes. (It might be nice to hide them in the print dialog, but that's not the issue here.) Second, |spec| here (from which you get the printer name) is an alias for |aDevice| which is already passed to the PS device context. The PS device context can extract and parse the printer name from the argument it's already getting; you don't need to do that here. Third, it doesn't look like you're doing anything useful with fullPrinterName and printerName anyway. You're just passing them to nsDeviceContextPS, which just passes them to nsPostScriptObj, which just ignores them. >diff -U8 -p -r1.68 nsDeviceContextSpecG.cpp >--- gfx/src/gtk/nsDeviceContextSpecG.cpp 19 Oct 2005 16:37:28 -0000 1.68 >+++ gfx/src/gtk/nsDeviceContextSpecG.cpp 19 Jan 2006 22:14:04 -0000 >@@ -682,36 +682,36 @@ NS_IMETHODIMP nsDeviceContextSpecGTK::Cl > /* Get prefs for printer > * Search order: > * - Get prefs per printer name and module name > * - Get prefs per printer name > * - Get prefs per module name > * - Get prefs > */ > static >-nsresult CopyPrinterCharPref(nsIPref *pref, const char *modulename, const char *printername, const char *prefname, char **return_buf) >+nsresult CopyPrinterCharPref(nsIPref *pref, const char *modulename, const char* prefix, const char *printername, const char *prefname, char **return_buf) It looks like the changes to |CopyPrinterCharPref()| are to let you look up prefs starting with "print.tmp.printerfeatures" as well as "print.printer_". As I understand it, the "print.tmp.printerfeatures" pref branch is actually created by the device context spec to communicate settings to the print dialog. It shouldn't be necessary to read any of those prefs while setting up for the print dialog. >--- gfx/src/ps/nsDeviceContextPS.cpp 23 Aug 2005 02:11:33 -0000 1.74 >+++ gfx/src/ps/nsDeviceContextPS.cpp 19 Jan 2006 22:14:04 -0000 >@@ -129,17 +129,19 @@ nsDeviceContextPS::~nsDeviceContextPS() > [...] >+ const char* paperName = nsnull; >+ psSpec->GetPaperName(&paperName); >+ if (paperName) mPrintJob->SetPaperName(paperName); This shouldn't be necessary either. the psSpec argument is passed to the print job class factory, which passes it to the print job class during initialization. If the print job class needs the paper name, it can get it then.

Comment on attachment 209022 [details] [diff] [review] Untested patch adding support for importing paper sizes from CUPS into Qt zack isn't doing reviews. Let's defer this one until the first one is landed.

Comment on attachment 209020 [details] [diff] [review] Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing Please address kherron's comments and then resubmit. When you do resubmit, ask for review from kherron and sr from me. Thanks!

(In reply to comment #3) Thanks for your excellent comments, Kenneth---I entirely agree. This new patch is substantially smaller, but achieves the same goal---which is great, of course. You'll find that I've also made a change to nsPrinterEnumeratorGTK::GetDefaultPrinterName, because I found that it gets called (by nsGlobalWindow::Print) in the sense "tell me which printer the user thinks mozilla will print to." Originally, it simply returned the first printer in the list, which is the system default printer. As far as I can gather, this means that the user must change the default printer OUTSIDE mozilla to print preview on a different printer than the last one used INSIDE mozilla. The change I suggest is to use the value of print.print_printer, i.e., the last used printer inside mozilla. If you agree on this interpretation, you might ask whether the change has been made in the correct place?

Comment on attachment 209263 [details] [diff] [review] Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing >--- gfx/src/nsPrintOptionsImpl.cpp 2 Sep 2005 09:28:49 -0000 1.78 >+++ gfx/src/nsPrintOptionsImpl.cpp 22 Jan 2006 09:44:12 -0000 >@@ -1095,29 +1095,31 @@ nsPrintOptions::InitPrintSettingsFromPre > aPS->GetIsInitializedFromPrefs(&isInitialized); > > if (isInitialized) > return NS_OK; > > nsAutoString prtName; > // read any non printer specific prefs > // with empty printer name >- nsresult rv = ReadPrefs(aPS, prtName, aFlags); >+ nsresult rv = ReadPrefs(aPS, prtName, >+ aFlags & ~nsIPrintSettings::kInitSavePrinterName); We have other bug reports about the issue you're trying to fix here, and as it happens I'm working one of them. See bug 324072. Even if that weren't the case, this change is unrelated to the bug at hand, it's to a file belonging to a different module (so it's under someone else's control), and it has implications for other platforms (windows & mac). I'm afraid it's not appropriate to include it in this patch. >--- gfx/src/gtk/nsDeviceContextSpecG.cpp 19 Oct 2005 16:37:28 -0000 1.68 >+++ gfx/src/gtk/nsDeviceContextSpecG.cpp 22 Jan 2006 09:44:21 -0000 >@@ -800,19 +855,23 @@ NS_IMETHODIMP nsPrinterEnumeratorGTK::En > return NS_OK; > } > > /* readonly attribute wstring defaultPrinterName; */ > NS_IMETHODIMP nsPrinterEnumeratorGTK::GetDefaultPrinterName(PRUnichar **aDefaultPrinterName) > { > DO_PR_DEBUG_LOG(("nsPrinterEnumeratorGTK::GetDefaultPrinterName()\n")); > NS_ENSURE_ARG_POINTER(aDefaultPrinterName); >- >- GlobalPrinters::GetInstance()->GetDefaultPrinterName(aDefaultPrinterName); >- >+ >+ nsresult rv; >+ nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv); >+ if (!NS_FAILED(rv)) >+ rv = pPrefs->CopyUnicharPref ("print.print_printer", aDefaultPrinterName); >+ if (NS_FAILED (rv)) >+ GlobalPrinters::GetInstance()->GetDefaultPrinterName(aDefaultPrinterName); This change isn't appropriate to this bug either. Aside from the the fact it's out of scope, you're not verifying that the printer from print.print_printer is still valid (see bug 323781). >@@ -1148,28 +1207,29 @@ NS_IMETHODIMP nsPrinterEnumeratorGTK::In > printerFeatures.SetSupportsResolutionNameChange(PR_FALSE); > printerFeatures.SetSupportsColorspaceChange(PR_FALSE); > #endif /* SET_PRINTER_FEATURES_VIA_PREFS */ > > #ifdef SET_PRINTER_FEATURES_VIA_PREFS > printerFeatures.SetCanChangeOrientation(PR_TRUE); > #endif /* SET_PRINTER_FEATURES_VIA_PREFS */ > >- nsXPIDLCString orientation; >- if (NS_SUCCEEDED(CopyPrinterCharPref(pPrefs, "postscript", printerName, "orientation", getter_Copies(orientation)))) { >- if (!PL_strcasecmp(orientation, "portrait")) { >+ int orientationIdx; >+ if (NS_SUCCEEDED(GetPrinterIntPref(pPrefs, "postscript", fullPrinterName, "print_orientation", &orientationIdx))) { >+ switch (orientationIdx) { >+ case 0: Once again, this is out of scope for the bug at hand. Considering the new |GetPrinterIntPref| this change adds quite a bit of code to this patch. I see that other parts of the printing code refer to a "print_orientation" pref so there may be a real issue here, but I'd rather see it handled in a bug specific to that subject. >+ PRBool hasSpoolerCmd; >+ switch (nsPSPrinterList::GetPrinterType(fullPrinterName)) { >+ case nsPSPrinterList::kTypePS: >+ case nsPSPrinterList::kTypeCUPS: >+ hasSpoolerCmd = true; >+ break; >+ default: hasSpoolerCmd = false; > } >- >- PRBool hasSpoolerCmd = (nsPSPrinterList::kTypePS == >- nsPSPrinterList::GetPrinterType(fullPrinterName)); Aside from being out of scope this is just wrong. CUPS printers don't have a spooler command. >--- gfx/src/ps/nsPrintJobPS.cpp 21 May 2005 15:33:09 -0000 1.7 >+++ gfx/src/ps/nsPrintJobPS.cpp 22 Jan 2006 09:44:22 -0000 >@@ -359,16 +359,20 @@ nsPrintJobCUPS::Init(nsIDeviceContextSpe > /* 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; > mJobTitle.SetIsVoid(PR_TRUE); >+ /* Paper name */ >+ const char* paperName = nsnull; >+ aSpec->GetPaperName (&paperName); Could you lose the space after the function name here? It's not the style followed in this file. >--- gfx/src/psshared/nsCUPSShim.cpp 8 May 2005 15:01:20 -0000 1.3 >+++ gfx/src/psshared/nsCUPSShim.cpp 22 Jan 2006 09:44:22 -0000 >@@ -40,23 +40,26 @@ > #include "nsString.h" > #include "nsCUPSShim.h" > #include "prlink.h" > > > // 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")] = { >+static const char gSymName[][sizeof("cupsPpdOpenFile")] = { This is a mistake? "cupsPpdOpenFile" isn't in the array and "cupsPrintFile" is longer than "ppdOpenFile". > { "cupsAddOption" }, > { "cupsFreeDests" }, > { "cupsGetDest" }, > { "cupsGetDests" }, > { "cupsPrintFile" }, > { "cupsTempFd" }, >+ { "cupsGetPPD" }, >+ { "ppdOpenFile" }, >+ { "ppdClose" }, >--- gfx/src/psshared/nsPaperPS.cpp 7 Sep 2004 17:51:50 -0000 1.1 >+++ gfx/src/psshared/nsPaperPS.cpp 22 Jan 2006 09:44:22 -0000 > [...] >+#if 0 >+nsPaperSizePS::nsPaperSizePS() { >+ DO_PR_DEBUG_LOG(("nsPaperSizePS::nsPaperSizePS()\n")); >+ mList = mDefaultList; >+ mCount = COUNTOF(mDefaultList); >+ mDynamicList = nsnull; >+ mDynamicCount = 0; >+ mCurrent = 0; >+} >+#endif /* 0 */ This doesn't need to be in here at all, does it? >+nsPaperSizePS::nsPaperSizePS (const char* fullPrinterName, >+ const char* printerName) { You're adding a lot of complexity here by trying to make one class that can handle everything. Let me suggest you structure this as follows: a base nsPaperSizePS class for non-CUPS printers, a subclass of that for CUPS printers, and a factory function. The factory function would take a printer name as an argument and return an instance of the correct paper size class. If you'd like to see an example of this kind of thing, see gfx/src/ps/nsPrintJobFactoryPS.* and gfx/src/ps/nsPrintJobPS.*. >+ mPrefSvc = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv); >+ if (!NS_SUCCEEDED(rv)) return; >+ rv = mPrefSvc->GetBranch("print.", getter_AddRefs(mPref)); >+ if (!NS_SUCCEEDED(rv)) return; >+ // Should we try cups? >+ PRBool useCups = PR_TRUE; >+ mPref->GetBoolPref("postscript.cups.enabled", &useCups); You don't need to check this. You can assume that if you were passed a CUPS name then CUPS is available. Even if you did have to check this, it looks like mPrefSvc and mPref are only used in this function so they don't need to be member variables, do they? >+ mCups.Init(); Initializing a CUPS shim is expensive. We need to try very hard to avoid creating & destroying one for every CUPS printer. The device context spec could pass one to the factory function, for example. >+ mDynamicCount = ppd->num_sizes; >+ mDynamicList = >+ (nsPaperSizePS_*) calloc (sizeof (nsPaperSizePS_), mDynamicCount); If this were a CUPS-specific subclass, you might consider holding on to the original ppd object for the life of the paper size object and serving size requests directly from that, instead of building an array of nsPaperSizePS_ structs. If not, you should allocate the nsPaperSizePS_ array using new[] instead of calloc(). >+ mDynamicList [j].width_mm = rintf (size.width * 0.35277); You need to define this magic number 0.35277 in some symbolic way. And rintf() is apparently new to C99 so it's not portable enough for mozilla. See <http://docs.hp.com/en/B9106-90010/rint.3M.html> for example, where it's only supported on their 64-bit platform.

(In reply to comment #3) Thanks again, Kenneth, for useful and precise comments. I entirely agree. This attachment attempts to implement what you suggest, getting rid of the intermediate data structure for holding paper sizes, serving requests directly from the PPD structure.

out of curiosity I've tested the latest patch but it doesn't seem to make a difference. The paper size is always letter although my cups printers are A4 printers. With NSPR_LOG_MODULES=PaperSizePS:5 I only get output like this: 1446704384[895beb0]: nsPaperSizeCUPS::nsPaperSizeCUPS('CUPS/gb_2_1_19', 'gb_2_1_19') when switching printers but no hint about the paper size to be available.

(In reply to comment #11) A little more debug info can hopefully be emitted by letting NSPR_LOG_MODULES=PaperSizePS:5,DeviceContextSpecGTK:5 It would also be interesting to know which paper sizes are listed when you click File->Print...->Properties...->Paper Size. Compare the list of paper sizes with your system's CUPS setup (use a printer configurator, or some CUPS aware program like OpenOffice or GIMP); if you don't get the CUPS paper sizes, but only the six mozilla default paper sizes, something's not as expected.

(In reply to comment #12) > It would also be interesting to know which paper sizes are listed when you > click File->Print...->Properties...->Paper Size. OK, my list is longer than 6 entries but even if I use a CUPS printer which has no "Letter" format and I see in the log that A4 is chosen correctly the UI still shows "Letter" as paper-format as set value. Could it be that the default "Letter" is always avaiable in the UI because of some reason? > Compare the list of paper sizes with your system's CUPS setup (use a printer > configurator, or some CUPS aware program like OpenOffice or GIMP); if you > don't get the CUPS paper sizes, but only the six mozilla default paper > sizes, something's not as expected. I see more values so the list seems to be gathered correctly from CUPS. However Letter seems to be always available. In addition you search for the paper defined as pref if available and use this which is correct. But if you don't have a paper defined already you just take the first paper you get from CUPS. But CUPS can define a default paper size which should be used in that case. (sorry, I never programmed with cups libs so I don't know how to get it) At least this is what is displayed as in this example: Hygiea:~ # lpoptions -p testy -l Duplex/Double-Sided Printing: *None DuplexNoTumble DuplexTumble PageSize/Media Size: Legal Executive A4 A5 B5 *EnvISOB5 Env10 EnvC5 EnvDL EnvMonarch InputSlot/Media Source: *Default Tray1 Tray2 Tray3 Manual PageRegion/PageRegion: Legal Executive A4 A5 B5 EnvISOB5 Env10 EnvC5 EnvDL EnvMonarch Resolution/Output Resolution: 150dpi *300dpi 600dpi 1200dpi 2400dpi I've tested your patch with this printer. The * marked size is the default set in the PPD. I've removed Letter from the available list but it's still displayed in the UI size-list.

Updated the patch: - file has been moved into widget/src/gtk2 - added minor feature, looking for CUPS system default paper size when none is present in the stored preferences Kenneth, could you please suggest what needs to be done for this patch to be usable (i.e., eventually committable)?

Arne, (In reply to comment #14) > Updated the patch: > - file has been moved into widget/src/gtk2 > - added minor feature, looking for CUPS system default paper > size when none is present in the stored preferences I've also prepared a modified patch to use the default paper given by the PPD configuration. I had a look at your patch but it seems that you never use your SytemDefault method? And as addition: After some more testing it seems that my first negative test-results were caused by old preferences. If I use a fresh profile I always got the correct results. mPPD should be initialized with nsnull.

> I've also prepared a modified patch to use the default paper given by the PPD > configuration. I had a look at your patch but it seems that you never use your > SytemDefault method? I forgot. This updated patch includes the call. > mPPD should be initialized with nsnull. What do you mean here? mPPD is not a static member, and it does get initialised in the constructor.

Kenneth: any objections to that patch? At least I would love to see it in Firefox2

Updated the patch for MOZILLA_1_8_BRANCH, and extended it to display human-readable paper sizes, if available. Kenneth: Where do you suggest we go from here with this bug?

FWIW, this is a slightly older patch applicable to the trunk. However, as printing directly to printers using the trunk version is not possible, cf. bug 335754, this patch is currently less interesting.

Now that bug 335754 has been fixed, here is a patch for the trunk.

Minor changes in response to comment #21.

In response to attachment #247841 [details] of comment #21: Thanks for your comments; I have followed your advice in many cases, and this attachment discusses the remaining ones.

Comment on attachment 247955 [details] Responses to comments in attachment #247841 [details] >>scary. by scary i was complaining about the lack of a temporary between an in and its replacement. it's no big deal. >>>Index: gfx/src/psshared/nsPSPrinters.h >>>+#include "nsCOMPtr.h" in general if the header itself isn't using something, you could probably stick the includes into the files that include the header. if it absolutely had to be in the header, that is really scary. it doesn't really matter at all. i just noted it because it was the *only* change in the file. it's very much stylistic (except for build time penalties involved in opening a file that isn't needed for any c++ files that include this chain and don't need this file, of which there are probably very few anyway). the other changes can certainly be handled in a later bug.

Comment on attachment 247950 [details] [diff] [review] Patch importing CUPS paper sizes for seamonkey trunk + nsCString paperName = nsnull; most of our magical classes have good defaults (ns*String, ns*Ptr<*>), don't assign nsnull here. i'd probably write PaperSizeManagerBlah instead of getting the Blah stuck in the middle.

Response to comment #24: nsCOMPtr.h is used by nsPSPrinters.h itself. Seems that nsPSPrinters.h previously has only been included at places where nsCOMPtr.h had already been included, thus causing no error messages. Response to comment #25: Fixed the two issues. Is this patch ready for superreview?

This problem still persists; it is impossible to print in any other format than one of seven standard formats, rendering the use of some printers (e.g., photo printers, label printers) impossible. I would very much like to see Mozilla improved to be able to use CUPS information on paper sizes. Further, I would be willing to make the present patch work with the latest Mozilla, if someone could tell me what steps would be necessary and sufficient to ensure it get included in future Mozilla releases?

Attachment #245644 [details] [diff]: >+nsPaperSizeCUPS::~nsPaperSizeCUPS() { >+ if (mPPD) mCups.mPpdClose (mPPD); >+ if (mPageSizeText) delete (mPageSizeText); >+} Without proper initializing these variables at nsPaperSizeCUPS::nsPaperSizeCUPS(), firefox will crash if you open the print dialog, select a printer with a valid PPD and then select a printer without a PPD, because those pointers will contain invalid values.

Comment on attachment 245644 [details] [diff] [review] Patch importing CUPS paper sizes for mozilla 1.8 branch Review of attachment 245644 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I haven't worked on Mozilla in years.

Comment on attachment 249890 [details] [diff] [review] Patch importing CUPS paper sizes for seamonkey trunk Review of attachment 249890 [details] [diff] [review]: ----------------------------------------------------------------- .