364133 - Add Print option on context menu for individual address book contacts

Status: RESOLVED FIXED

(Thunderbird :: Address Book, enhancement)

Description

[adam]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0

The Properties panel for individual address book entries is frankly bizarre. You should be able to simply Open a card, and the usual File, Edit, etc menus should be available. At the very least, as in the subject, you should be able to Print from the context menu for a card, rather than the current convoluted method of selecting an item and going to the File menu (which I wasn't aware of before coming to Bugtraq).

Reproducible: Always

Comment 1

[Giulio Portioli [:gportioli]]

Attachment: Mod v1 - Print command added to context menu

This mod addresses the request in this bug. I have essentially copied one of the existing entries in the Address Book context menu and changed its attributes to the followings:

- id="abResultsTreeContext-print": something unique in DXR and consistent with local naming convention.

- label="&printButton2.label;": again, something unique similar to existing local labels. Corresponding entity definition added to the DTD file.

- accesskey="&printButton2.accesskey;": ditto

- command="cmd_printcard": copied from the existing 'Print...' item of the 'File' menu in the Address Book; such <menuitem> has also a key="key_printContact" attribute defined, which I'm not sure what it does and does not seem required by the context menu, so I left it out.

The chosen access key is 'R', because 'P' is already taken by the 'Properties' menu item.

I've updated the associated addressbook.css file with the id of the new context menu entry. The DTD file contained also a few spurious trailing white spaces, which I've cleaned up.

It looks as though the same menu is used also for SeaMonkey; perhaps the new context menu entry should be added there as well?

Comment 2

[Ian Neal]

Comment on attachment 8663062 [details] [diff] [review]
Mod v1 - Print command added to context menu

>+<!ENTITY printButton2.label                             "Print…">
>+<!ENTITY printButton2.accesskey                         "R">
Nit: it is better for the accesskey to match the case of the letter, so in this case it should be "r"

Comment 3

[Giulio Portioli [:gportioli]]

Attachment: Mod v2 - Print command added and DTD file cleaned up

Thanks, I hadn't spot the convention. In fact, there were about twenty more accesskey letters whose case that did not match the corresponding target letter, so I've fixed them too - See Mod v2.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8663291 [details] [diff] [review]
Mod v2 - Print command added and DTD file cleaned up

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

Overall this looks fine, but I think there are some unintended changes in here.

::: mail/locales/en-US/chrome/messenger/addressbook/abMainWindow.dtd
@@ +6,5 @@
>  <!ENTITY blankResultsPaneMessage.label                  "This address book shows contacts only after a search">
>  <!ENTITY localResultsOnlyMessage.label                  "Contacts from remote address books are not shown until you search">
>  <!-- File Menu -->
>  <!ENTITY fileMenu.label                                 "File">
> +<!ENTITY fileMenu.accesskey                             "F">

How come all of these accesskeys had their cases changed?

Comment 5

[Giulio Portioli [:gportioli]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> How come all of these accesskeys had their cases changed?

As pointed out in comment 2, best practice seems to be for the access key letter to have the same case as the target letter in the label, so, after fixing the access key in my mod, I couldn't help fixing all other access keys that had non-matching case, in the DTD file. I've tested the corresponding menu entries and they all work as expected.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Giulio Portioli [:gportioli] from comment #5)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> > How come all of these accesskeys had their cases changed?
> 
> As pointed out in comment 2, best practice seems to be for the access key
> letter to have the same case as the target letter in the label, so, after
> fixing the access key in my mod, I couldn't help fixing all other access
> keys that had non-matching case, in the DTD file. I've tested the
> corresponding menu entries and they all work as expected.

Ah, I see.

I'd really rather not mix fixes to unrelated issues in the same patch. Would it be possible for you to file and attach the accesskey changes to a separate bug?

Comment 7

[Giulio Portioli [:gportioli]]

OK, will do, but in a couple of weeks.

Comment 8

[Giulio Portioli [:gportioli]]

Attachment: Mod v3 - Unrelated changes to DTD file reverted

Please see attached Mod v3, which is the same as v2, but with all the additional tidying up of the DTD file reverted, as discussed.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669238 [details] [diff] [review]
Mod v3 - Unrelated changes to DTD file reverted

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

r=me with the string keys renamed from printButton2 to printButton! Thanks!

::: mail/locales/en-US/chrome/messenger/addressbook/abMainWindow.dtd
@@ +138,5 @@
>  <!ENTITY newmsgButton.label                             "Write">
>  <!ENTITY newmsgButton.accesskey                         "W">
>  <!ENTITY newIM.label                                    "Instant Message">
>  <!ENTITY newIM.accesskey                                "I">
> +<!ENTITY printButton2.label                             "Print…">

I just realized that this can be printButton, and doesn't need to be printButton2 - the only time we'd bump the number is if we were replacing the string.

Comment 10

[Giulio Portioli [:gportioli]]

> this can be printButton, and doesn't need to be printButton2

I had thought about printButton, but then noticed that DXR already returns several entries for that identifier, although not in the same DTD nor XUL files.
If reusing the same identifier - in different places - does not cause any problem, I'm happy to drop the '2' at the end and change it to printButton.

Comment 11

[Magnus Melin [:mkmelin]]

It would only be a problem if the addressbook.xul xul file loads a dtd that also includes the same entity, but that isn't the case.

Comment 12

[Giulio Portioli [:gportioli]]

Attachment: Mod v4 - printButton2 changed to printButton

Please see attached mod v4 with entity renamed as printButton, instead of printButton2.
I tested it and could print out contacts as expected, using either mouse, access keys or CTRL+P. No regressions observed on printing functions in the main message window.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8673663 [details] [diff] [review]
Mod v4 - printButton2 changed to printButton

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

Looks good to me - thanks!

Comment 14

[Giulio Portioli [:gportioli]]

Check-in request cancelled pending successful push-to-try.

Comment 15

[Giulio Portioli [:gportioli]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cd0ccb2a782

Comment 16

[Kent James (:rkent)]

Correct try link is https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7943b4ff0988  I don't think this patch really needs a try run before landing, but looking at those results I don't seen any new issues from that (though there are new build issues there that are probably unrelated, or due to errors in the try push).

Comment 17

[Kent James (:rkent)]

Tracking because there is no good reason we don't get this landed in TB 45 before the string freeze.

Comment 18

[Giulio Portioli [:gportioli]]

OK, marked for checkin as suggested. I'll push it again to try later just to leave a clean build on the try server.

Comment 19

[:aceman]

https://hg.mozilla.org/comm-central/rev/c46ebcce67bb