Replace Carbon printing dialog with Cocoa one

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: whimboo, Assigned: mstange)

Tracking

Trunk
All
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
blocking1.9.1 -
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Flags: blocking1.9.1?

Updated

10 years ago
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
(Reporter)

Updated

10 years ago
Blocks: 433778

Updated

10 years ago
No longer blocks: 425844

Updated

10 years ago
Flags: wanted1.9.2+

Updated

10 years ago
Flags: wanted1.9.1+ → wanted1.9.1-
Priority: -- → P1

Updated

9 years ago
Assignee: joshmoz → mstange

Updated

9 years ago
Blocks: 495567

Updated

9 years ago
Blocks: 468509
(Assignee)

Updated

9 years ago
Blocks: 268309
(Assignee)

Comment 1

9 years ago
Created attachment 398583 [details] [diff] [review]
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.
Attachment #398583 - Flags: review?(joshmoz)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED

Comment 2

9 years ago
(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?
(Assignee)

Comment 3

9 years ago
(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

9 years ago
(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

9 years ago
Probably would need to suppress the status UI with setShowsProgressPanel: as well.
(Assignee)

Comment 6

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

Comment 7

9 years ago
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

9 years ago
(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?
(Assignee)

Comment 9

9 years ago
(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

9 years ago
Created attachment 398933 [details]
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

9 years ago
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

9 years ago
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

9 years ago
(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.

Updated

9 years ago
Attachment #398583 - Flags: review?(joshmoz)
(Assignee)

Comment 14

9 years ago
Created attachment 403177 [details] [diff] [review]
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?
Attachment #398583 - Attachment is obsolete: true
Attachment #403177 - Flags: review?(joshmoz)
(Assignee)

Comment 15

9 years ago
Created attachment 403183 [details] [diff] [review]
v3, re-using the core text check
Attachment #403177 - Attachment is obsolete: true
Attachment #403183 - Flags: review?(joshmoz)
Attachment #403177 - Flags: review?(joshmoz)

Comment 16

9 years ago
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!
Attachment #403183 - Flags: review?(joshmoz) → review-

Comment 17

9 years ago
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
(Assignee)

Comment 18

9 years ago
(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.
(Assignee)

Comment 19

9 years ago
Created attachment 403344 [details] [diff] [review]
v4
Attachment #403183 - Attachment is obsolete: true
Attachment #403344 - Flags: review?(joshmoz)
(Assignee)

Comment 20

9 years ago
Created attachment 403345 [details] [diff] [review]
v4.1

This has the + in jar.mn.
Attachment #403344 - Attachment is obsolete: true
Attachment #403345 - Flags: review?(joshmoz)
Attachment #403344 - Flags: review?(joshmoz)
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.
(Assignee)

Comment 22

9 years ago
dom/locales! Thanks!
(Assignee)

Comment 23

9 years ago
Created attachment 403414 [details] [diff] [review]
v5

This combines the two printdialog.properties files.
Attachment #403345 - Attachment is obsolete: true
Attachment #403414 - Flags: review?(joshmoz)
Attachment #403345 - Flags: review?(joshmoz)

Updated

9 years ago
Attachment #403414 - Flags: review?(joshmoz) → review+

Comment 24

9 years ago
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

9 years ago
(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).
(Assignee)

Comment 26

9 years ago
Created attachment 403606 [details] [diff] [review]
v6

Changed the location of the autorelease calls. I also changed some sizes in order to allow for longer labels for verbose localizations.
Attachment #403414 - Attachment is obsolete: true
Comment on attachment 403606 [details] [diff] [review]
v6

sr=jst for the dom related changes.
Attachment #403606 - Flags: superreview+

Updated

9 years ago
Attachment #403606 - Flags: review?(benjamin)

Comment 28

9 years ago
Comment on attachment 403606 [details] [diff] [review]
v6

r=me on the build bits and such... I didn't do a code review
Attachment #403606 - Flags: review?(benjamin) → review+

Comment 29

9 years ago
I think that is all the necessary reviews, lets get this in. Thanks Markus!
(Assignee)

Comment 30

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8c4658f8f0dc
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Blocks: 520494
(Assignee)

Updated

9 years ago
No longer blocks: 433778
(Assignee)

Updated

9 years ago
No longer blocks: 495567
You need to log in before you can comment on or make changes to this bug.