Fix broken controllers for cmd_printcard and cmd_printcardpreview

RESOLVED FIXED in Thunderbird 57.0

Status

defect
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: bugzilla2007, Assigned: bugzilla2007)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 57.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ 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.
Assignee: nobody → bugzilla2007
Status: NEW → ASSIGNED
Those menus would also really need dynamic labels, but I'm not sure if I'm in the mood of doing that too.
No longer depends on: 196135
7 lines of code, should be straightforward.
Attachment #8896021 - Flags: review?(jorgk)
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.
Attachment #8896021 - Flags: review?(jorgk) → review?(acelists)
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?
Attachment #8896021 - Flags: feedback+
Seamonkey may need the command update calls too.
Flags: needinfo?(iann_bugzilla)
(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.
Attachment #8896021 - Attachment is obsolete: true
Attachment #8896021 - Flags: review?(acelists)
Attachment #8896789 - Flags: review?(acelists)
Comment on attachment 8896789 [details] [diff] [review]
1320475_ab_print_controller.patch

Review of attachment 8896789 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #8896789 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/81a123c7b693
Fix broken controllers for cmd_printcard and cmd_printcardpreview. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
Thanke aceman. I put the info in our 2.57 master bug 1433370 and removed the NI for IanN here.
Flags: needinfo?(iann_bugzilla)
Blocks: 1504543
You need to log in before you can comment on or make changes to this bug.