66781 - [FIX]Mac UI needs to be hooked up for printing selection and page range [print] [mac]

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Comment 1

[dcone (gone)]

Attachment: nsDeviceContextSpec patch

Comment 2

[dcone (gone)]

Attachment: new file added to Mac GFX project.

Comment 3

[dcone (gone)]

Attachment: nsMacResources.h file added to GFX project

Comment 4

[dcone (gone)]

I changed one file:
nsDeviceContextSpecMac.cpp	   first attachment

I added 3 files to the GFX mac project to support resouces needed for printing

1.) nsMacResouces.cpp		   second attachment
2 [details] [diff] [review].) nsmacResources.h		   third attachment
3 [details] [diff] [review].) nsMacGFXl.rsrc		   not attached (it would not attach in bugzilla)

the nsMacGFX.rsrc has the DITL's needed for adding items to the print dialog.

Comment 5

[Simon Fraser [no longer active]]

dcone: please attach a screenshot of the print dialog stuff for Mac. You can 
attach the .rsrc file itself if you BinHex it.

Comment 6

[dcone (gone)]

Attachment: binhex resourced

Comment 7

[dcone (gone)]

Attachment: Screen shot of resources

Comment 8

[Simon Fraser [no longer active]]

(Please attach screenshots as PNG images; you can use the PictureViewer 
application to create these.)

Comments:

Resources and dialog UI:
Can we use Appearance-savvy controls here, and the Appearance Dialog Manager 
calls? This will make it easier to transition to Carbon, and look better (the
single pixel black outline for the Frames grouping looks ugly now, for example).

Also, you're using dialog_item_struct and append_item_list_struct to get at 
Dialog Manager internal data structures. This is a bad. It looks like you just 
need the number of items in the dialog, which you can get using CountDITL. I'd 
recommend that you look for a recent Apple technote or sample code on the 
recommended way to extend the print dialog.

+static pascal void MyBBoxDraw(WindowPtr theWindow, short aItemNo)

This routine never sets the foreground color, so you might draw the outline in 
random colors.

+    switch (myItem) {
+      case 1:
+        ::GetDialogItem((DialogPtr)aDialog,firstItem,&itemType,&itemH,&itemBox);
+        gPrintSelection = (gPrintSelection==PR_TRUE)?PR_FALSE:PR_TRUE;
+        ::SetControlValue((ControlHandle)itemH,gPrintSelection);
+        break;
+      case 2:
+      case 3:
+      case 4:

Please provide an enum for your dialog items. This makes the code more readable.


+        // print frames as is
+        ::GetDialogItem((DialogPtr)aDialog,firstItem+2-1,&itemType,&itemH,&
itemBox);
+        value = ::GetControlValue((ControlHandle)itemH);
+        if(1==value){
+          gCurrOptions->SetPrintFrameType(nsIPrintOptions::kFramesAsIs);
+        }
+        
+        // selected frame
+        ::GetDialogItem((DialogPtr)aDialog,firstItem+2-1,&itemType,&itemH,&
itemBox);
+        value = ::GetControlValue((ControlHandle)itemH);
+        if(1==value){
+          gCurrOptions->SetPrintFrameType(nsIPrintOptions::kSelectedFrame);
+        }
+        
+        // print all frames
+        ::GetDialogItem((DialogPtr)aDialog,firstItem+2-1,&itemType,&itemH,&
itemBox);
+        value = ::GetControlValue((ControlHandle)itemH);
+        if(1==value){
+          gCurrOptions->SetPrintFrameType(nsIPrintOptions::kEachFrameSep);
+        }        

You're using 'firstItem+2-1 for each of these. Is that wrong?

+#include <resources.h>
Please fix case (so it works on Mac OS X some day)

+  PrtJobDialog->pItemProc = NewPItemProc(MyJobItems);
This allocates a UPP, which you never free. This may leak a 4-byte pointer.

+  myDrawListUPP = NewUserItemProc(MyBBoxDraw);
Ditto.

General comments:
nsDeviceContextSpecMac.cpp has inconsistent tabbing/spacing.
It also uses some non-standard C++ formatting rules for {} indentation, spaces 
after , spaces around = and ==, around (), and the indentation of local 
variables. Also, I don't know if you like the if(1==value){ style, but please 
don't use it out of any sense of obligation.

Comment 9

[dcone (gone)]

Attachment: patch with fixes

Comment 10

[dcone (gone)]

I attached a patch which address Simons suggestions.  The only thing I did not 
address really was using the dialog_item_struct and append_item_List_struct which 
were taken straight from a technical note from apple on how to extend dialogs.

The issues on using the appearance manager I will address later, I wanted to get 
this in by .91, two days away and could not get that part addressed in time.

Comment 11

[Peter Lubczynski]

r=peterl

I assume in OS X there is completely different way of extending the print 
dialog, but Don for sure get this in for 0.9.1? Anyone know where those 
resources can be found?

Comment 12

[Simon Fraser [no longer active]]

More comments:
1. You still leak the UPPs for pItemProc, myDrawListUPP and theInitProcPtr if
   PrDlgMain() returns false (user cancelled?). pItemProc is never disposed.
2. Spacing is totally whacked in this file. You need to de-tab it (BBEdit
   can do this).
3. Would prefer enum over #defines for dialog items, like:
      enum {
        ePrintSelectionCheckboxID = 1,
        ePrintFrameStaticTextID
      ...etc.

The code would be much cleaner if you factored the GetDialogItem()/Get/
SetControlValue into helper methods.

Sorry, but I'm still not ready to sr this code.

Comment 13

[dcone (gone)]

Attachment: Fixed the problems.. with last patch.

Comment 14

[Simon Fraser [no longer active]]

Much more readable. sr=sfraser

Comment 15

[dcone (gone)]

Fixed

Comment 16

[sujay]

I see range UI. but no selection UI in 5/30 build on Mac.

Comment 17

[Kevin McCluskey (gone)]

Don says it works for him in a debug build but does not show up in a release build.

Comment 18

[Simon Fraser [no longer active]]

It works only in debug builds because the CFM init/terminate routines are only 
set correctly in the non-carbon debug target in the project (so the res fork is 
never opened when the lib is loaded). The Entry Points under the PPC linker panel 
in the project settings need to be fixed for all targets (including the carbon 
targets).

Comment 19

[dcone (gone)]

I put the CFM init/terminate routines into all the GFX builds.  I had Marc 
Attinasi and Peter L look at the changes.. and showed them the running 
application as a test.  Waiting for there r and sr.

Comment 20

[Peter Lubczynski]

visual review @ don's cube, r=peterl

Comment 21

[Marc Attinasi]

I looked at don's project on his machine, looks good. sr=attinasi

Comment 22

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 23

[dcone (gone)]

fixed

Comment 24

[sujay]

Guys, I see the page range feature in the print dialog, but I don't
see any printing selection UI or option...

reopening this bug.

Comment 25

[Chris Petersen]

I'm able to access and use the new print options in the latest mac build
(2001062108) Netscape 6 option appears in pulldown menu of print dialog. I was
able to print a selection on a page, a selection in frame, and a complete frameset. 

Comment 26

[sujay]

okay I see it...can we make this selection feature more accessible?
Currently its hidden in the "Netscape 6" menu item...nobody is
gonna know how to get to this....it fooled me...

Comment 27

[sujay]

verified.