+++ This bug was initially created as a clone of Bug #1319409 +++ (In reply to Thomas D. (needinfo?me) from Bug 1319409 comment #15) > ::: mail/components/addrbook/content/addressbook.js > @@ +336,1 @@ > > return; > > Right, but I wonder why we need the check for selected directory at all. > There's no way you can select a contact without prior selection of a > directory, so this can getSelectedDirectory can never fail. So I think we > can just drop the check for selected directory, uri isn't otherwise used in > this function. > Even the check for selected cards does not belong here; ironically, they > never bothered to disable the menuitems/command so you can still click print > contact or print contact preview even without a contact selected, which will > silently fail on our condition here. STR 1) In main AB, deselect all contacts by Ctrl+clicking 2) Press Ctrl+P or File > Print Contact | Print Preview Contact Actual result - Keyboard shortcut / Menus are enabled, but do nothing. Expected result - Where we do nothing, menus and keyboard shortcut must be disabled. Implementation: - Technically, we want to disable the respective commands using their controllers' isCommandEnabled function. - Turns out that these twins commands are only half implemented; for starters, there's nothing on the resultspane controller. - Then we have to look into the logics and see which scenarios we want to enable or not. I've got this working on my local installation by adding the missing cases on the controller switch cases, and massaging conditions for isCommandEnabled a bit, plus actually adding the respective goUpdateCommand(...) to the CommandUpdate_AddressBook() function. Btw that function seems to run at least twice for every selection change, not sure if that's really needed.

Those menus would also really need dynamic labels, but I'm not sure if I'm in the mood of doing that too.

7 lines of code, should be straightforward.

Comment on attachment 8896021 [details] [diff] [review] 1320475_ab_print_controller.patch Looks good to me, but I'm really not such a controller guy. I'd be more comfortable for Aceman to approve this.

Comment on attachment 8896021 [details] [diff] [review] 1320475_ab_print_controller.patch Review of attachment 8896021 [details] [diff] [review]: ----------------------------------------------------------------- It seems I have exactly this in my WIP patch in bug 1193160 (seems I never uploaded it) :) Will be good if we can land at least this part. ::: mailnews/addrbook/content/abResultsPane.js @@ +417,5 @@ > } > return (enabled && (numSelected > 0)); > + case "cmd_printcardpreview": > + case "cmd_printcard": > + return (gAbView.selection.count > 0); Do we really support printing multiple cards at once? In my patch I have 'return (GetSelectedCardIndex() != -1)' but that only enables the command if a single card is selected. But atleast it does not call gAbView directly as that may not always exist (see the function). Maybe you want GetNumSelectedCards() here?

Seamonkey may need the command update calls too.

(In reply to :aceman from comment #4) > Comment on attachment 8896021 [details] [diff] [review] > 1320475_ab_print_controller.patch > > Review of attachment 8896021 [details] [diff] [review]: > ----------------------------------------------------------------- > > It seems I have exactly this in my WIP patch in bug 1193160 (seems I never > uploaded it) :) > Will be good if we can land at least this part. > > ::: mailnews/addrbook/content/abResultsPane.js > @@ +417,5 @@ > > } > > return (enabled && (numSelected > 0)); > > + case "cmd_printcardpreview": > > + case "cmd_printcard": > > + return (gAbView.selection.count > 0); > > Do we really support printing multiple cards at once? Well, for Print of multiple selected contacts, we print one card/list per page, in separate sequential print jobs. Bugs on record to just print multiple cards on one page, as we do for entire AB's. Print Preview of multiple selected contacts/lists shows only first one, and printing from preview then also only prints first card. (bug on record). But we're not here to fix that. So I think we should enable printing for multiple cards, with a view on fixing those other bugs later. It's obviously useful to print multiple cards. One per page is odd, but better than not being able to print them at all. > In my patch I have 'return (GetSelectedCardIndex() != -1)' but that only > enables the command if a single card is selected. > But atleast it does not call gAbView directly as that may not always exist > (see the function). > Maybe you want GetNumSelectedCards() here? OK.

Comment on attachment 8896789 [details] [diff] [review] 1320475_ab_print_controller.patch Review of attachment 8896789 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/81a123c7b693 Fix broken controllers for cmd_printcard and cmd_printcardpreview. r=aceman

Thanke aceman. I put the info in our 2.57 master bug 1433370 and removed the NI for IanN here.