456646 - Replace Carbon printing dialog with Cocoa one

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080922020341 Minefield/3.1b1pre ID:20080922020341

Currently we fail while re-enabling the menu items inside some menus after the printing dialog is closed (see bug 425844). This is caused by using the old Carbon dialog. While we have already switched to mostly using Cocoa, the printing dialog has to be replaced by the Cocoa one - that is what Josh mentioned in bug 425844 comment 17.

I split this work into a new bug to get a better tracking.

Comment 1

[Markus Stange [:mstange]]

Attachment: v1

This will neither build nor work on 10.4. Josh, should I #ifdef all of the changes?

I'm using the following APIs that don't exist on 10.4:
 1. -[NSPrintPanel showModalWithPrintInfo:]
 2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]
 3. -[NSString stringByReplacingOccurrencesOfString:withString:]
 4. -[NSPrintPanel addAccessoryController:]
 5. PMPageFormatCreate(With)DataRepresentation
and probably some more.

The first two are essential to the implementation and I haven't found 10.4 replacements for them. 3 is just for convenience and 4 and 5 get rid of deprecated warnings.

Comment 2

[Tom Dyas]

(In reply to comment #1)
> Created an attachment (id=398583) [details]
> v1
> 
> This will neither build nor work on 10.4. Josh, should I #ifdef all of the
> changes?
> 
> I'm using the following APIs that don't exist on 10.4:
>  1. -[NSPrintPanel showModalWithPrintInfo:]

On 10.4, will it work to just create a dummy NSPrintOperation, load it with the NSPrintInfo via its setPrintInfo, and then call runModal?

>  2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]

On 10.4, will it work to use initWithDictionary NSPrintInfo initializer after filling in a custom dictionary from the Gecko print settings?

Comment 3

[Markus Stange [:mstange]]

(In reply to comment #2)
> On 10.4, will it work to just create a dummy NSPrintOperation, load it with the
> NSPrintInfo via its setPrintInfo, and then call runModal?

Could be! I haven't tested yet (and can't at the moment).
 
> >  2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]
> 
> On 10.4, will it work to use initWithDictionary NSPrintInfo initializer after
> filling in a custom dictionary from the Gecko print settings?

I don't think that's enough. We also need these Carbon objects in nsDeviceContextSpecX.mm for things like ::PMSessionBeginPageNoDialog, for which there is no Cocoa equivalent.

Comment 4

[Tom Dyas]

(In reply to comment #3)
> (In reply to comment #2)
> > On 10.4, will it work to just create a dummy NSPrintOperation, load it with the
> > NSPrintInfo via its setPrintInfo, and then call runModal?
> 
> Could be! I haven't tested yet (and can't at the moment).

The docs talk about the "current print operation" when using [NSPrintPanel runModal]. Probably referring to [NSPrintOperation setCurrentOperation].

> > >  2. -[NSPrintInfo PMPrintSettings/PMPrintSession/PMPageFormat]
> > 
> > On 10.4, will it work to use initWithDictionary NSPrintInfo initializer after
> > filling in a custom dictionary from the Gecko print settings?
> 
> I don't think that's enough. We also need these Carbon objects in
> nsDeviceContextSpecX.mm for things like ::PMSessionBeginPageNoDialog, for which
> there is no Cocoa equivalent.

Any benefit in rewriting that code in Cocoa?  Not sure if this would work:  Initiate the print operation using NSPrintOperation.  Suppress the print dialog using the setShowsPrintPanel: method.  And you'll need a Quartz surface which you can get by calling the context method to get a NSGraphicsContext (which I believe can be toll-free bridged to CGContextRef).  Not sure how extensive such changes would be though.

Comment 5

[Tom Dyas]

Probably would need to suppress the status UI with setShowsProgressPanel: as well.

Comment 6

[Markus Stange [:mstange]]

I don't think it's worth it if 10.4 compatibility is the only benefit of such a rewrite.

Comment 7

[Nomis101]

I've tried this patch and with this patch Thunderbird 3.1a1pre (haven't tested for Firefox) builds fine with the 10.6 SDK. But at the last step I get the error:

rm -f ../../mozilla/dist/Thunderbird.app/Contents/MacOS/thunderbird-bin
rsync -aL thunderbird-bin ../../mozilla/dist/Thunderbird.app/Contents/MacOS
mkdir -p ../../mozilla/dist/Thunderbird.app/Contents/Plug-Ins
rsync -a --copy-unsafe-links ../../mozilla/dist/package/PrintPDE.plugin ../../mozilla/dist/Thunderbird.app/Contents/Plug-Ins
rsync: link_stat "/temp/src/obj-i386-apple-darwin10.0.0/mail/app/../../mozilla/dist/package/PrintPDE.plugin" failed: No such file or directory (2)
rsync error: some files could not be transferred (code 23) at /SourceCache/rsync/rsync-37.3/rsync/main.c(992) [sender=2.6.9]
make[5]: *** [libs] Error 23
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_app] Error 2
make[2]: *** [tier_app] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

Comment 8

[Tom Dyas]

(In reply to comment #6)
> I don't think it's worth it if 10.4 compatibility is the only benefit of such a
> rewrite.

Probably true. Plus Cocoa inverts how the print operation works by driving the process itself and telling the NSView to draw each page, as opposed to Carbon, where the application tells Carbon about each page.

Would there be any benefit to having an all Cocoa solution going forward apart from 10.4 compatibility?

Comment 9

[Markus Stange [:mstange]]

(In reply to comment #7)
> But at the last step I get the error:

Try removing the printpde.plugin line from mail/app/Makefile.in. It looks like I need to remove that line from browser/app/Makefile.in, too.

(In reply to comment #8)
> Plus Cocoa inverts how the print operation works by driving the
> process itself and telling the NSView to draw each page, as opposed to Carbon,
> where the application tells Carbon about each page.

That's what scared me away from such a rewrite ;-)

> Would there be any benefit to having an all Cocoa solution going forward apart
> from 10.4 compatibility?

I don't know. It looks like the parts of (Carbon) Core Printing that we use with this patch are fully supported by Apple and 64 bit clean. Josh?

Comment 10

[Nomis101]

Attachment: Broken print dialog

After applying this patch, the print dialog looks like this. I've tried it no three times with fresh source code and checked all modified files.

Comment 11

[Josh Aas]

Markus - your patch needs to account for this line:

http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#348

That packages the PrintPDE plugin in the browser and shouldn't be executed when building with cocoa printing.

Comment 12

[Josh Aas]

Also, we unfortunately do need a patch that doesn't drop 10.4 support right now. Can you fix this to leave 10.4 support by making Cocoa printing a build-time option, much like the Core Text backend is a build-time option?

Comment 13

[Nomis101]

(In reply to comment #11)
> Markus - your patch needs to account for this line:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#348
> 
> That packages the PrintPDE plugin in the browser and shouldn't be executed when
> building with cocoa printing.

Same thing for Thunderbird 3.1a1pre (Thunderbird 3.next) in /mail/app/Makefile.in
Otherwise you will get a build error.

Comment 14

[Markus Stange [:mstange]]

Attachment: v2: add --disable-cocoa-printing configure option

I haven't been able to write a proper configure test. Is there a way to make AC_TRY_RUN run Objective C?

Comment 15

[Markus Stange [:mstange]]

Attachment: v3, re-using the core text check

Comment 16

[Josh Aas]

Comment on attachment 403183 [details] [diff] [review]
v3, re-using the core text check

+#include <Carbon/Carbon.h>

Use "ApplicationServices/ApplicationServices.h" - I doubt we need to use the actual Carbon headers anywhere.

+- (NSTextField*)label:(const char*)aLabel
+            withFrame:(NSRect)aRect
+            alignment:(NSTextAlignment)aAlignment

+- (NSPopUpButton*)headerFooterItemListWithFrame:(NSRect)aRect
+                                   selectedItem:(const PRUnichar*)aCurrentString

These method names suggest that the returned objects are autoreleased, per obj-c convention. In fact, as implemented, callers are required to release. Please return autoreleased objects and check all new obj-c methods for this problem.

+- (NSButton*)radioWithLabel:(const char*)aLabel andFrame:(NSRect)aRect;

This method doesn't appear to be used.

In general this looks great, thanks!

Comment 17

[Josh Aas]

Also, when I built with this patch and tried to pull up a print dialog I saw what nomis saw in comment #10, blank labels and options.

I see this console output when that happens:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1107
###!!! ASSERTION: nsPrintSettingsX::ReadPageFormatFromPrefs() failed: 'NS_SUCCEEDED(rv)', file /Users/josh/src/mozilla/ff_193_debug/widget/src/cocoa/nsPrintOptionsX.mm, line 92
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 1055
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040154: file /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp, line 910
getPrintSettings: [Exception... "Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIPrintSettingsService.defaultPrinterName]"  nsresult: "0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame :: chrome://global/content/printUtils.js :: anonymous :: line 125"  data: no]
2009-09-28 01:51:57.657 firefox-bin[65739:813] 	ERROR

Comment 18

[Markus Stange [:mstange]]

(In reply to comment #16)
Adressed.

(In reply to comment #17)
> Also, when I built with this patch and tried to pull up a print dialog I saw
> what nomis saw in comment #10, blank labels and options.

I can't reproduce this. I've added a plus to to the line in jar.mn, maybe that helps? What do you see when you put chrome://global/locale/printdialog.properties into your URL bar?

> WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000FFFF: file
> /Users/josh/src/mozilla/ff_193_debug/widget/src/xpwidgets/nsPrintOptionsImpl.cpp,
> line 1107

This seems to be unrelated. It's hidden deep in cross-platform code I didn't touch.

> ###!!! ASSERTION: nsPrintSettingsX::ReadPageFormatFromPrefs() failed:
> 'NS_SUCCEEDED(rv)', file
> /Users/josh/src/mozilla/ff_193_debug/widget/src/cocoa/nsPrintOptionsX.mm, line
> 92

The problem here is that I've changed the format the page format is stored in. This assertion will go away the next time you click OK in the Page Setup dialog.
I've changed the code in nsPrintSettingsX::ReadPageFormatFromPrefs to not return an error result code when the settings import fails.

The rests seems unrelated.

Comment 19

[Markus Stange [:mstange]]

Attachment: v4

Comment 20

[Markus Stange [:mstange]]

Attachment: v4.1

This has the + in jar.mn.

Comment 21

[Phil Ringnalda (:philor)]

Surely what you want is another name, rather than a plus? You use a plus when you want to overwrite an existing file that someone else put in the jar, even if their version is newer, because your file is a total replacement for theirs (same keys, different values, for a .properties).

In this case, you're either replacing dom/locales/en-US/chrome/printdialog.properties with your printdialog.properties on all platforms, which seems like it's going to come as a bit of a surprise to embedding/components/printingui/src/win/nsPrintDialogUtil.cpp since you don't have the same keys, or if dom/locales builds after toolkit/locales (which I don't think it does, but I'm often surprised), then you would have to touch yours every time that one changes, to keep it from overwriting you.

Comment 22

[Markus Stange [:mstange]]

dom/locales! Thanks!

Comment 23

[Markus Stange [:mstange]]

Attachment: v5

This combines the two printdialog.properties files.

Comment 24

[Josh Aas]

Comment on attachment 403414 [details] [diff] [review]
v5

+- (NSTextField*)label:(const char*)aLabel
+            withFrame:(NSRect)aRect
+            alignment:(NSTextAlignment)aAlignment
+{
+  NSTextField* label = [[NSTextField alloc] initWithFrame:aRect];
+  [label setStringValue:[self localizedString:aLabel]];
+  [label setEditable:NO];
+  [label setSelectable:NO];
+  [label setBezeled:NO];
+  [label setBordered:NO];
+  [label setDrawsBackground:NO];
+  [label setFont:[NSFont systemFontOfSize:[NSFont systemFontSize]]];
+  [label setAlignment:aAlignment];
+  return [label autorelease];
+}

Typically autorelease is done when the object is created. Doing so avoids problems if someone adds an early return, much like stack-based memory mgmt. Please fix that in the multiple places the problem occurs.

Other than that, this looks good. I don't see the blank labels problem any more.

Comment 25

[Nomis101]

(In reply to comment #24)
> I don't see the blank labels problem any
> more.
I can confirm this. v5 also works for me (no blank labels anymore).

Comment 26

[Markus Stange [:mstange]]

Attachment: v6

Changed the location of the autorelease calls. I also changed some sizes in order to allow for longer labels for verbose localizations.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 403606 [details] [diff] [review]
v6

sr=jst for the dom related changes.

Comment 28

[Benjamin Smedberg]

Comment on attachment 403606 [details] [diff] [review]
v6

r=me on the build bits and such... I didn't do a code review

Comment 29

[Josh Aas]

I think that is all the necessary reviews, lets get this in. Thanks Markus!

Comment 30

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/8c4658f8f0dc