90751 - Print Options for Carbon

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Mike Pinkerton (not reading bugmail)]

The print options done for classic macos9 need to be implemented for carbon. 
documentation for the carbon print manager is here:

http://developer.apple.com/techpubs/macosx/Carbon/graphics/CarbonPrintingManager/
carbonprintingmgr.html

Comment 1

[Mike Pinkerton (not reading bugmail)]

this is platform parity, and needed for osX rtm.

Comment 2

[Steve Dagley]

looks like it's not going to make the 094+ branch - removing OSX+ (sigh)

Comment 3

[Simon Fraser [no longer active]]

ping

Comment 4

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 5

[Kevin McCluskey (gone)]

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

Comment 6

[Conrad Carlen (not reading bugmail)]

I've looked into this. There's example code for building a PDE (Printer Dialog
Extension) on /Developer/Examples/Printing/App. The biggest challenge in this is
making an equivalent CW Mach-0 bundle project. Once that's done, the sample code
could easily be adapted. There would also have to be some build packaging work
to add this bundle to the app bundle.

Comment 7

[dcone (gone)]

Is that on a web site.. or the Developers CD.. I just got that CD and have not 
installed it yet.

Comment 8

[Conrad Carlen (not reading bugmail)]

That's where it'll be on your HD after instaliing the Developer's CD.

Comment 9

[Syd Logan]

adt2 per adt triage

Comment 10

[dcone (gone)]

Attachment: Plugin main file

Comment 11

[dcone (gone)]

Attachment: Plugin include file

Comment 12

[dcone (gone)]

Attachment: plugin common include

Comment 13

[dcone (gone)]

Attachment: resources for plugin

Comment 14

[dcone (gone)]

Attachment: localize string file

Comment 15

[dcone (gone)]

Attachment: ndDeviceContextSpecX changes.

Comment 16

[Marc Attinasi]

Comment on attachment 77307 [details] [diff] [review]
ndDeviceContextSpecX changes.

fprintf / fflush need to #ifdef DEBUG_dcone, otherwise sr=attinasi for the
changes in this patch.

Aren't there some new files too?

Comment 17

[Marc Attinasi]

Doh - I see the new files now - sorry - trying to review.

Comment 18

[Marc Attinasi]

Comment on attachment 77095 [details]
Plugin main file

tabs are bad - other than that I have no idea if this is correct or not, so
rubber-stamping: rs=attinasi

You really need to have somebody who is familiar with this stuff review this
patch.

Comment 19

[Marc Attinasi]

Comment on attachment 77097 [details]
Plugin include file

Pleas align the values. sr=attinasi

Comment 20

[Marc Attinasi]

Comment on attachment 77098 [details]
plugin common include

sr=attinasi

Comment 21

[Marc Attinasi]

Comment on attachment 77100 [details]
resources for plugin

sr=attinasi (like I even understand this)

Comment 22

[Marc Attinasi]

Comment on attachment 77101 [details]
localize string file

What the heck is this? Must be UNICODE or something, sorry I cannot tell if it
is good or not.

Comment 23

[Peter Lubczynski]

Comment on attachment 77095 [details]
Plugin main file

r=peterl

Comment 24

[Peter Lubczynski]

Comment on attachment 77097 [details]
Plugin include file

r=peterl

Comment 25

[Peter Lubczynski]

Comment on attachment 77098 [details]
plugin common include

r=peterl

Comment 26

[Peter Lubczynski]

Comment on attachment 77100 [details]
resources for plugin

r=peterl

Comment 27

[Peter Lubczynski]

Comment on attachment 77101 [details]
localize string file

r=peterl

Comment 28

[Peter Lubczynski]

Comment on attachment 77307 [details] [diff] [review]
ndDeviceContextSpecX changes.

r=peterl but why mix/match "Boolean" vs. PRBool

Comment 29

[Conrad Carlen (not reading bugmail)]

Comment on attachment 77307 [details] [diff] [review]
ndDeviceContextSpecX changes.

>   status = ::PMDefaultPrintSettings(mPrintSettings);
>   if (status != noErr) return NS_ERROR_FAILURE;
> 
>-  if (! aQuiet)
>-  {
>+  if (! aQuiet) {
>+  Boolean plugInExtended,accepted=false;
>+  PRBool  isOn;
>+  PRInt16 howToEnableFrameUI = nsIPrintSettings::kFrameEnableNone;
>+  nsPrintExtensions       printData = {false,false,false,false,false,false,false,false};
>+  #define kPDE_Creator    'mozi'		
>+  
> 		::InitCursor();
> 
>-    Boolean accepted = false;
>+    // set the values for the plugin here
>+    aPS->GetPrintOptions(nsIPrintSettings::kEnableSelectionRB, &isOn);
>+    printData.mHaveASelection = isOn;
>+
>+    aPS->GetHowToEnableFrameUI(&howToEnableFrameUI);
>+    if (howToEnableFrameUI == nsIPrintSettings::kFrameEnableAll) {
>+      printData.mHaveFrames = true;
>+      printData.mHaveFrameSelected = true;
>+    }
>+    
>+    if (howToEnableFrameUI == nsIPrintSettings::kFrameEnableAsIsAndEach) {
>+      printData.mHaveFrames = true;
>+      printData.mHaveFrameSelected = false;
>+    }
>+        
>+    aPS->GetShrinkToFit(&isOn);
>+    printData.mShrinkToFit = isOn;    
>+        
>+    plugInExtended = LoadPrinterPlugin();

Does this need to be done before every call to PMPrintDialog?
What is the lifetime of this component once registered?
You said before loading the plugin was expensive. Is this still the case now
that it's within our bundle?

>+
>+    status = PMSetPrintSettingsExtendedData(mPrintSettings,kPDE_Creator,sizeof(printData),&printData);
>+
>     status = ::PMPrintDialog(mPrintSettings, mPageFormat, &accepted);
>+
>+

Comment 30

[Conrad Carlen (not reading bugmail)]

Comment on attachment 77101 [details]
localize string file

For this, do we need placeholder files in various locales that the
internationalization people can go fill in later?

Comment 31

[Simon Fraser [no longer active]]

I'd prefer to look over these changes before they get checked in.

Comment 32

[dcone (gone)]

The plugin.. once loaded.. loads fast in our bundle.  After the first time it 
seems to load very fast.  Also.. seems when I run under the debug.. things are 
very slow.. but if I just run the dialog comes up fast.

Simon.. all the code for the plug-in are attachments in this bug..so you can see 
what I did.  There are some tab issues I fixed and I took out the one fprintf, 
but besides that.. the code attached to this bug is what I have.

Comment 33

[Simon Fraser [no longer active]]

Comment on attachment 77095 [details]
Plugin main file

Needs work, comments coming.

Comment 34

[Simon Fraser [no longer active]]

Comment on attachment 77095 [details]
Plugin main file

 #define kPMPrintingManager		       
CFSTR("com.appvendor.printingmanager")
 #define kSampleAppUserOptionKindID	       
CFSTR("com.appvendor.print.pde.PrintDialogPDEOnly")
 #define kPrintDialogPDEBundleID	       
CFSTR("com.appvendor.print.pde.PrintDialogPDE")

You need to fix these to use "org.mozilla.print...". Ideally, we'd
need different ones for Mozilla and Netscape builds, but I think using
"org.mozilla" for both is OK for now.

 #define kPrintDialogPDEIntfFactoryIDStr
CFSTR("F48965F3-45C2-11D6-A87C-00105A183419")

Did you make a unique UUID here?

 #define kMAXH			150			// max size of our
drawing area
 #define kMAXV			130			// should be calculated
based on our needs

Did you adjust these numbers? Do we need code that
adjusts them dynamically?

 const ResType kPlugInCreator = kPDE_Creator;		// should be set to an
appropriate creator

Is this set correctly? This is another thing that should ideally
differ between Mozilla ('MOZZ') and commercial ('MOSS') builds.

 static 
 OSStatus MyPDEPrologue()

Please rename all "MyFoo" functions to something more Mozilla-specific, like
MozPDEPrologue.

 if(!err){

Space after the 'if' and before the '{' please (here and lots of other places).

 if(false == printSettings.mHaveSelection) {

I think the utility of 'constant == value' tests are outweighed by the loss of
readability. In addition, boolean tests are always more readable written
as 'if (foo)' and 'if (!foo)'. Please fix.

 HiliteControl((ControlHandle)myContext->thePrintSelectionOnly, 255);

Please use EnableControl/DisableControl, rather than HiliteControl().

 theControlValue = (printSettings.mPrintSelection) ? 1 : 0;
 SetControlValue(myContext->thePrintSelectionOnly, theControlValue); 

You can safely cast a boolean to a value:

 SetControlValue(myContext->thePrintSelectionOnly,
(SInt32)printSettings.mPrintSelection);

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 77097 [details]
Plugin include file

 #define kPrintSelectionControlID 4000
 #define kPrintFrameAsIsControlID 4001
 #define kPrintSelectedFrameControlID 4002
 #define kPrintFramesSeperatlyControlID 4003
 #define kShrinkToFiControlID 4004
 #define kRadioGroupControlID 4005

Prefer enums. Also, does any code outside of the .c file need to know
these IDs?

Comment 36

[Simon Fraser [no longer active]]

Comment on attachment 77098 [details]
plugin common include

  #define kPDE_Creator		'mozi'		

Should probably use 'MOZZ' or 'MOSS' here. Also need to fix:
#define kAppPrintDialogPDEOnlyKey	       
CFSTR("com.apple.print.PrintSettingsTicket.mozi")


      Here we use 'paul' as the 4 byte identifier.

Fix this part of the comment.

Comment 37

[Simon Fraser [no longer active]]

Comment on attachment 77307 [details] [diff] [review]
ndDeviceContextSpecX changes.

+typedef struct {
+    Boolean mHaveASelection;
+    Boolean mHaveFrames;
+    Boolean mHaveFrameSelected;
+    Boolean mPrintSelection;
+    Boolean mPrintFrameAsIs;
+    Boolean mPrintSelectedFrame;
+    Boolean mPrintFramesSeperatly;
+    Boolean mShrinkToFit;
+} nsPrintExtensions;
+

This struct is in two places; here and in the plugin. You should move it
to a common .h file so that it stays in sync.

+  nsPrintExtensions	   printData =
{false,false,false,false,false,false,false,false};

Why not give nsPrintExtensions a constructor that does this? You
can #ifdef __cplusplus it.

+    status =
PMSetPrintSettingsExtendedData(mPrintSettings,kPDE_Creator,sizeof(printData),&p
rintData);

Why not ::PMSetPrintSettingsExtendedData()? Also, spaces after the commas
please.

+    if(status == noErr){

Space after 'if' please (other places too).

+      status = PMGetPrintSettingsExtendedData(mPrintSettings, kPDE_Creator,
&bytesNeeded, NULL);

::PMGetPrintSettingsExtendedData() ?\

Comment 38

[dcone (gone)]

Attachment: New plugin main file cleaned up.

Comment 39

[dcone (gone)]

Attachment: New plugin include file

Comment 40

[dcone (gone)]

Attachment: New resource file.

Comment 41

[dcone (gone)]

Attachment: New plugin common file. This is used by the plugin and nsDeviceContextSpecX to share common data.

Comment 42

[dcone (gone)]

Attachment: patch of mozilla files.

Cleanup up nsDeviceContextSpecX, the perl script to move the plugin and a small
fix in nsIPrintSettings to return the correct type.

Comment 43

[Peter Lubczynski]

Comment on attachment 77726 [details]
New plugin main file cleaned up.  

r=peterl

Comment 44

[Peter Lubczynski]

Comment on attachment 77727 [details]
New plugin include file

r=peterl

Comment 45

[Peter Lubczynski]

Comment on attachment 77731 [details]
New plugin common file.  This is used by the plugin and nsDeviceContextSpecX to share common data.

r=peterl

Comment 46

[Peter Lubczynski]

Comment on attachment 77729 [details]
New resource file.

r=peterl

Comment 47

[Peter Lubczynski]

Comment on attachment 77736 [details] [diff] [review]
patch of mozilla files.

r=peterl

Comment 48

[Marc Attinasi]

Comment on attachment 77726 [details]
New plugin main file cleaned up.  

sr=attinasi

Comment 49

[Marc Attinasi]

Comment on attachment 77727 [details]
New plugin include file

sr=attinasi

Comment 50

[Marc Attinasi]

Comment on attachment 77729 [details]
New resource file.

sr=attinasi

Comment 51

[Marc Attinasi]

Comment on attachment 77731 [details]
New plugin common file.  This is used by the plugin and nsDeviceContextSpecX to share common data.

sr=attinasi

Comment 52

[Marc Attinasi]

Comment on attachment 77736 [details] [diff] [review]
patch of mozilla files.

sr=attinasi

Comment 53

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 54

[Kevin McCluskey (gone)]

adt1.0.0

Comment 55

[Paul Wyskoczka]

Has this been tested by QA? If not, could you get a test build to Sujay. 

Comment 56

[Jaime Rodriguez, Jr.]

Removing adt1.0.0. Pls land this on the trunk, and let it bake for a couple of
days. If there are no regressions, and QA approves, pls renominate for ADT approval.

Comment 57

[Kevin McCluskey (gone)]

sujay: Could you verify the fix on the trunk? Thanks!

Comment 58

[dcone (gone)]

Can't verify this yet.. because it has to be put into the build.  I am working
with JJ Enser to get this in.  He thinks it will be monday or tuesday if the
tree does not open soon.

Comment 59

[dcone (gone)]

Attachment: patch for packages-mac (obsolete)

Comment 60

[J.J. Enser]

Comment on attachment 79525 [details] [diff] [review]
patch for packages-mac (obsolete)

This patch copies and exposes unwanted CVS directories under .plugin structure.
 Will submit another one that takes care  of this issue.

Comment 61

[J.J. Enser]

Attachment: Patch#2 to packages-mac

This patch copies the .plugin "directory" and removes all underlying CVS
directories.
Note: this should be cleaned up after we build this plugin ourselves and create
the plugin bundle rather than checking out the whole structure.

Comment 62

[rods (gone)]

Comment on attachment 79545 [details] [diff] [review]
Patch#2 to packages-mac

r=rods

Comment 63

[dcone (gone)]

I checked this patch into the trunk .. based on jj's code..my review of the 
code.. and I had Rods review that code for the packager. This should get the 
dialog extended for the build on the trunk.

Comment 64

[dcone (gone)]

Attachment: Patch to use relative path

Comment 65

[Peter Lubczynski]

Comment on attachment 79703 [details] [diff] [review]
Patch to use relative path

r=peterl

Comment 66

[Marc Attinasi]

Comment on attachment 79703 [details] [diff] [review]
Patch to use relative path

sr=attinasi - Assuming of course that this is all tested and working (I know
nothing about this code...)

Comment 67

[Marc Attinasi]

Comment on attachment 79545 [details] [diff] [review]
Patch#2 to packages-mac

sr=attinasi (like I have a clue)

Comment 68

[Conrad Carlen (not reading bugmail)]

Comment on attachment 79703 [details] [diff] [review]
Patch to use relative path

Very bad - don't do this. Flattening to a path when the path contains non-ASCII
chars on OS X is problematic.
>+    mozFile->GetPath(&fullModuleName);
>+    nsFileSpec  file(fullModuleName);
>+    
>+    const FSSpec  spec = file;
>+

Instead, do this:
nsCOMPtr<nsILocalFileMac> mozMacFile(do_QueryInterface(mozFile));
mozMacFile->GetFSSpec(&spec);

Or, to save the QI:

+  nsCOMPtr<nsILocalFileMac> mozFile;
+  nsCOMPtr<nsIProperties>
directoryService(do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID));
+  if (!directoryService) {
+    return false;
+  } else {
+    directoryService->Get(
NS_XPCOM_CURRENT_PROCESS_DIR,NS_GET_IID(nsILocalFileMac),
getter_AddRefs(mozFile));

nsIProperties can do it that way.

Comment 69

[Steve Dagley]

to amplify ccarlen's needs-work comemnt on this patch it breaks the mach-o build
due to the lack of nsFileSpec in that build.  Not that that isn't a problem with
the mach-o build but nsFileSpec is supposed to be deprecated right?

Comment 70

[Simon Fraser [no longer active]]

No new code should use nsFileSpec. This code should use nsILocalFileMac, QIable
to nsIFile/nsILocalFile.

Comment 71

[Chris Petersen]

Using the OS X April 19th build (2002-04-19-03) , Browser Printing Extensions
appears as a menu item in Print dialog drop-down menu. Shrink to Fit is enabled
by default.

Comment 72

[dcone (gone)]

I checked in a version that had the changes from comment #68, and ifdef'd out 
the code for the Mach o build.  Condrad is checking some ideas we had to fix for 
that.. I will open a new bug to fix the Mach O build.  This is fixed.. 

Comment 73

[Chris Petersen]

Per Sujay's request, marking verified on the April 22 OS X trunk build
(2002-04-22-03). Browser Printing Extensions is available and it's options are
functional (whether the site is loaded in a frameset or not).

Comment 74

[scottputterman]

adding adt1.0.0+, please check into the branch as soon as possible and add the
fixed1.0.0 keyword.

Comment 75

[dcone (gone)]

Attachment: Patch that has all the changes for the branch in one file, besides the new files

This patch has all the changes in it.. for the branch including the ideas from
comment 68 and 69.  These changes have been on the trunk for a few weeks.

Comment 76

[scottputterman]

changing to adt1.0.0-.  Let's get this in for rtm.

Comment 77

[scottputterman]

From comment #72, does this mean the most recent patch no longer needs work? 
Does it have an r= and sr=?

Comment 78

[dcone (gone)]

This has all the proper reviews and super reviews.  All the comments mentioned 
were fixed.. there are just so many things that happend.  The last patch is what 
is currently on the trunk.. with all the metioned fixes.. I will have that 
reviewed and super reviewed..

Comment 79

[Marc Attinasi]

Comment on attachment 81570 [details] [diff] [review]
Patch that has all the changes for the branch in one file, besides the new files

rs=attinasi

Comment 80

[Conrad Carlen (not reading bugmail)]

Comment on attachment 81570 [details] [diff] [review]
Patch that has all the changes for the branch in one file, besides the new files

r=ccarlen

Comment 81

[scottputterman]

adding adt1.0.0+.  Please get drivers approval and then check into the 1.0 branch.

Comment 82

[scottputterman]

Adding adt1.0.1+ for adt approval for branch checkin for the 1.0.1 milestone. 
Please get drivers approval before checking in.

Comment 83

[scottputterman]

Are there any changes in the patches that require localization?  If so, you will
need to get mcarlson's approval asap prior to checking this in.  If not, please
ask for driver's approval

Comment 84

[dcone (gone)]

The localization strings are  in the plugin and can be localized with the mac 
tools as outlined by how to localize plugins.  

Comment 85

[Conrad Carlen (not reading bugmail)]

Don, can you hold off on landing this? I was on vacation and thought that this
would be in and I'd have to deal with the conflicts. Bug 135441, which Rod is
checking in tomorrow, contains essentially this code. With that, all printing UI
is now in embedding and gets removed from gfx. That new code in embedding
already has this code. If this is checked in before Rod checks in his (I'm
dealing with the Mac portion of that) I'll have to resolve the conflicts that
arise between that and this patch.

Comment 86

[Paul Wyskoczka]

Adding mozilla1.0.1 keyword.

Comment 87

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is there still a desire for this patch to be checked in to the branch?  If not,
should the adt1.0.1+ be removed?  (It's showing up on queries of adt1.0.1+ bugs
that drivers haven't approved yet.)

Comment 88

[Ying-Lin Xia]

> The localization strings are  in the plugin and can be localized with the 
> mac tools as outlined by how to localize plugins.  

Please do not put the UI strings into the plugin binary, it makes l10n more 
difficult. If you do so, we will file a new bug for that ;-) We already had some 
l10n bugs for the UI strings in other plugin binaries.

Comment 89

[scottputterman]

We did want this at one point which is why it's an adt1.0.1+, but it looks like
there may be some issues with the string and it's past the l10n freeze. 
Removing the + for reevaluation.

Comment 90

[dcone (gone)]

I dont know of anyone who has written this type of plugin since this is a 
special MacOSX package.  This is a project builder (MAC OSX type) that extends 
there dialog.  This means we do not get any interaction with the event loop, 
initialization, etc.  This is not a mac resource change for these strings.. this 
is more like strings placed into the projects, given attributes, etc with Mac 
tools.  Maybee you know of someone who has done this type of things.. but I 
really doubt this type of plugin is currently used in Mozilla since we are still 
having problems getting this packaged up properly for Mozilla (I have been 
trying to help the build engineer get this plugin packaged into the build)

Comment 91

[scottputterman]

In patch http://bugzilla.mozilla.org/attachment.cgi?id=77729&action=view there
are a number of controls with what look like labels.  I don't know anything
about Mac resource files, so I just want to verify that those strings won't show
up in the UI. If they won't then it looks like we're fine.

Comment 92

[scottputterman]

adding adt1.0.1-.  Let's get this in the next release.