165283 - Make PrintDlgEx work without new SDK

Status: RESOLVED WONTFIX

(Core :: Printing: Output, defect)

Description

[rods (gone)]

Comment 1

[rods (gone)]

Attachment: patch v1

The new approach (thanks to roc) is to create a copy of the necessary strcut
definitions for compiling and then at runtime check to see if the Procs exist
for calling the new PrintDlgEx and CreatePropertyPage. If not, use the old
standard call.

Comment 2

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

Comment on attachment 97066 [details] [diff] [review]
patch v1

The code looks good. I'm a bit worried about the copyright implications of
duplicating the MS header files. You should definitely rip out all the
comments. I think we should also get rid of any field names we don't need
(replace them with dummy names).

Comment 3

[rods (gone)]

I am having a lot of problems with this, something is horrible wrong...

Comment 4

[rods (gone)]

Attachment: patch v2

The problem with the previous patch was definition of the PrintDlgEx function,
it should now be defined as this:
typedef HRESULT APIENTRY PrintDlgExProc(PrintDlgExType* lppd);

Before it was missing the "APIENTRY" it was causing major problems.

So this patch work as does the other one, checks the DLL t see if it can
dynamically call the PrintDlgEx function and we now have out own version of the
header file.

Comment 5

[:deanis74]

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

Comment 6

[timeless]

Comment on attachment 104769 [details] [diff] [review]
patch v2

change the license for the new file to MPL/LGPL/GPL (c) 2002, and then get some
people to sign off on this :)

Comment 7

[:deanis74]

Comment on attachment 104769 [details] [diff] [review]
patch v2

>+  if ( comdlg32Lib ) {

Remove the extra spaces, it's predominantly |if (condition)| in the file.

>+  return gPrintDlgProc != NULL && gCreatePropertySheetPageProc != NULL?PR_TRUE:PR_FALSE;

Why do you need != NULL?  Can't you just |return gPrintDlgProc &&
gCreatePropertySheetPatgeProc ? PR_TRUE : PR_FALSE;| ?

//-----------------------------------------------------------------------------
-----
>@@ -323,6 +334,12 @@
>   if (aDevMode->dmFields & DM_PAPERSIZE) {
>     aPrintSettings->SetPaperSizeType(nsIPrintSettings::kPaperSizeNativeData);
>     aPrintSettings->SetPaperData(aDevMode->dmPaperSize);
>+    for (PRInt32 i=0;i<kNumPaperSizes;i++) {
>+      if (kPaperSizes[i].mPaperSize == aDevMode->dmPaperSize) {
>+        aPrintSettings->SetPaperSizeUnit(kPaperSizes[i].mIsInches?nsIPrintSettings::kPaperSizeInches:nsIPrintSettings::kPaperSizeMillimeters);
>+        break;
>+      }
>+    }

>-        aPrintSettings->SetPaperSizeUnit(kPaperSizes[i].mIsInches?nsIPrintSettings::kPaperSizeInches:nsIPrintSettings::kPaperSizeInches);
>+        aPrintSettings->SetPaperSizeUnit(kPaperSizes[i].mIsInches?nsIPrintSettings::kPaperSizeInches:nsIPrintSettings::kPaperSizeMillimeters);

Are these two part of another patch?  It's already in the tree.


>+void SetDevModeTmp(DEVMODE * aDevMode)
>+{
>+  LPDEVMODE dv = aDevMode;
>+
>+}

Where is this used?

>+  // Get the Print Name to be used

s/Print/Printer.  Kind of an excessive comment, though.  The two lines of code
seem pretty self-explanatory.

>+  if (!printerName) return NS_ERROR_FAILURE;

Linebreak before |return|, please.

>+      if (pPageRanges) ::GlobalFree(pPageRanges);
>+      if (hGlobalDevMode) ::GlobalFree(hGlobalDevMode);

Linebreaks, please.

>+      if (pPageRanges) ::GlobalFree(pPageRanges);
>+      if (hGlobalDevMode) ::GlobalFree(hGlobalDevMode);

Again.

>+    // Since we just applied the values into the print settings
>+    // it really is the same as an abort for us
>+    rv = prntdlg.m_dwResultAction == NS_PRTDLG_APPLY ? NS_ERROR_ABORT : NS_OK;
>   } else {
>-    if (hGlobalDevMode) ::GlobalFree(hGlobalDevMode);
>-    return NS_ERROR_ABORT;
>+    rv = NS_ERROR_ABORT;
>   }

So if rv is NS_ERROR_ABORT we handle it, and if it isn't then we make it
NS_ERROR_ABORT?  I don't quite understand what's going on here, perhaps the
comment needs to be clearer?

>+  if (pPageRanges) ::GlobalFree(pPageRanges);
>+  if (hGlobalDevMode) ::GlobalFree(hGlobalDevMode);

Linebreaks.


So mostly just nits, but if you can clear up the few questions I have then
everything else looks great.

Comment 8

[Ere Maijala (slow)]

Comment on attachment 104769 [details] [diff] [review]
patch v2

The patch doesn't apply cleanly to the current tree anymore.

Comment 9

[Wayne Mery (:wsmwk)]

Roc, After bug 122530, are this bug and Bug 122798 still needed?  

Comment 10

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

Probably not, but I'm not sure. Ere will know.

Comment 11

[Ere Maijala (slow)]

I think this would be useless. However print dialog stuff could use some cleanup in trunk now that old Windows versions are not supported.