1635458 - Add print context menu items and print button to address book toolbar

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, enhancement)

Description

[Ian Neal]

Add print context menu by porting the relevant parts of the following bug to SeaMonkey:

• Bug 364133 - Print command added to address book's context menu

Reorganise the File menu by porting the relevant parts of the following bug to SeaMonkey:

• Bug 485371 - New gnomestripe icons for address book

Also add print button to address book toolbar

Comment 1

[Ian Neal]

Attachment: Addressbook print patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: no print in context menu, no option to add print button to toolbar
Testing completed (on m-c, etc.): 2.53.3
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: 3 new strings

Comment 2

[Ian Neal]

The one thing I've not done is add the print button as a default to the toolbar, fairly simple to tweak the patch to do that though.

Comment 3

[Frank-Rainer Grahl (:frg)]

The mailnews part would TB. Could you take a look for comm-central.

Comment 4

[Ian Neal]

Attachment: Addressbook print patch v1.1

Disables print button when card pane has focus but nothing selected.

Comment 5

[Ian Neal]

For comm-central, TB doesn't use cmd_print or cmd_printpreview in their addressbook so adding these should make no difference to them. Once other ab patches have landed on c-c, I'll create a c-c version of this patch.

Comment 6

[Ian Neal]

Attachment: Addressbook print patch v1.1 for cc

Patch for comm-central. Potentially could use if (AppConstants.MOZ_APP_NAME == "seamonkey") around the code but I think that would add to the complexity and seeing only SeaMonkey would be calling with cmd_print or cmd_printpreview is unnecessary.

Comment 7

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9151343 [details] [diff] [review]
Addressbook print patch v1.1 for cc

LGTM

Comment 8

[Frank-Rainer Grahl (:frg)]

Comment on attachment 9151330 [details] [diff] [review]
Addressbook print patch v1.1

LGTM r/a+ for our release branches.

Comment 9

[Ian Neal]

Comment on attachment 9151343 [details] [diff] [review]
Addressbook print patch v1.1 for cc

For mailnews js changes - as TB doesn't use cmd_print or cmd_printpreview in it's addressbook, it should never call this code, but thought it was best to check no issues with putting it in.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9151343 [details] [diff] [review]
Addressbook print patch v1.1 for cc

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

::: mailnews/addrbook/content/abResultsPane.js
@@ +442,5 @@
>          return enabled;
>        }
> +      case "cmd_printpreview":
> +      case "cmd_print":
> +        var enabled = (GetNumSelectedCards() > 0);

I'm not sure why you're not using cmd_printcard?
Anyway, why not querySelectorAll like above to make it clear what's happening and also protecting against the case where the button is not there. Not sure that would happen in practice

::: suite/locales/en-US/chrome/mailnews/addressbook/abMainWindow.dtd
@@ +80,5 @@
>  <!ENTITY deleteItemButton.accesskey                     "D">
>  <!ENTITY newimButton.label                              "Instant Message">
>  <!ENTITY newimButton.accesskey                          "I">
> +<!ENTITY printButton.label                              "Print">
> +<!ENTITY printButton.accesskey                          "r">

at least in tb, r is taken for Redo

Comment 11

[Ian Neal]

Attachment: Addressbook print patch v1.2 for cc

(In reply to Magnus Melin [:mkmelin] from comment #10)

Comment on attachment 9151343 [details] [diff] [review]
Addressbook print patch v1.1 for cc

Review of attachment 9151343 [details] [diff] [review]:

::: mailnews/addrbook/content/abResultsPane.js
@@ +442,5 @@

     return enabled;
   }

•  case "cmd_printpreview":
  

•  case "cmd_print":
  

•    var enabled = (GetNumSelectedCards() > 0);
  

I'm not sure why you're not using cmd_printcard?
Anyway, why not querySelectorAll like above to make it clear what's
happening and also protecting against the case where the button is not
there. Not sure that would happen in practice
I am using cmd_print as that means that it works for both cards and addressbooks depending on which pane has focus. Happy to use querySelectorAll to keep it consistent with other parts of the code.

::: suite/locales/en-US/chrome/mailnews/addressbook/abMainWindow.dtd
@@ +80,5 @@

<!ENTITY deleteItemButton.accesskey "D">
<!ENTITY newimButton.label "Instant Message">
<!ENTITY newimButton.accesskey "I">
+<!ENTITY printButton.label "Print">
+<!ENTITY printButton.accesskey "r">

at least in tb, r is taken for Redo
As far as I am aware printButton.label is only used in the results pane tree context menu whereas redo is in the edit menu, so no clash currently.

Comment 12

[Ian Neal]

Attachment: Addressbook print patch v1.2

[Triage Comment]
Carrying forward r/a for SM repos

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9151451 [details] [diff] [review]
Addressbook print patch v1.2 for cc

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

::: mailnews/addrbook/content/abResultsPane.js
@@ +439,5 @@
>              e.setAttribute("label", e.getAttribute(labelAttr));
>            }
>          });
>          return enabled;
>        }

Maybe add a comment
// "cmd_printpreview" and "cmd_print" are only used in SeaMonkey.

Comment 14

[Ian Neal]

Attachment: Addressbook print patch v1.3 for cc

Added comments.
Carrying forward r+ from frg and mkmelin

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a172c7754d7d
Add print context menu items and print button to address book toolbar. r=frg r=mkmelin

Comment 16

[Frank-Rainer Grahl (:frg)]

taskcluster seems to dislike the suite/mailnews patch and didn't even start a TB job:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a172c7754d7d9721ab93c80fd4d0def59f395563

Comment 17

[Magnus Melin [:mkmelin]]

Bug 1640382 - not specific to that.

Comment 18

[Frank-Rainer Grahl (:frg)]

Target 2.53.3
https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/3aeadca0bc1101fdd10ba56b4f01e803c24689ff
Add print context menu items and print button to address book toolbar. r=frg a=frg