455246 - Change address book UI to refer to Contacts rather than Cards

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

This is something Bryan and I have discussed in the past, and it seems like a good idea. We should be referring to the 'cards' in the address book as 'contacts' as it is more user friendly.

We probably can't do a straight search and replace for example on the card view pane it won't look right, but we should start transitioning where possible.

Comment 1

[Mark Banner (:standard8)]

Attachment: [checked in] Part 1 - Most of TB address book

This does a large proportion of the renaming within Thunderbird. There's still some areas that haven't been renamed, in code that is shared etc. I'll be sorting out those later.

Going for ui-review first to check that Bryan is happy with these changes.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 351988 [details] [diff] [review]
[checked in] Part 1 - Most of TB address book

The strings look good so far.  Though I haven't tested any of it, from looking at the string changes I can see where they fit into the AB.

Comment 3

[Phil Ringnalda (:philor)]

Comment on attachment 351988 [details] [diff] [review]
[checked in] Part 1 - Most of TB address book

>--- a/mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
>+++ b/mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
>-## LOCALIZATION NOTE (totalCardStatus): 
>-## %1$S is address book name, %2$S is card count
>-totalCardStatus=Total Cards in %1$S: %2$S
>+## LOCALIZATION NOTE (totalContactStatus): 
>+## %1$S is address book name, %2$S is contact count
>+totalContactStatus=Total contacts in %1$S: %2$S

If you're going to own blame on that l10n note, you probably don't want to own the trailing whitespace.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 351988 [details] [diff] [review]
[checked in] Part 1 - Most of TB address book

Checked in: http://hg.mozilla.org/comm-central/rev/96e4acad5ce7

Comment 5

[Alex Ihrig [:Thunderbird_Mail_DE]]

(In reply to comment #1)
> This does a large proportion of the renaming within Thunderbird. There's still
> some areas that haven't been renamed, in code that is shared etc. I'll be
> sorting out those later.

What about the following lines in addressBook.properties?

cardsCopied=...
cardCopied=...
cardsMoved=...
cardMoved=...

Comment 6

[Phil Ringnalda (:philor)]

http://mxr.mozilla.org/comm-central/search?string=cardsCopied - two .properties, one use in code, makes that exactly what he meant by "in code that is shared".

Comment 7

[Mark Banner (:standard8)]

Attachment: [checked in] Part 2 - Most of SM address book

This does the renaming in most of the SeaMonkey address book. I think there's still a few instances of SM specific strings, but I'll catch them later. Like with the TB patch, I'll do a third part for strings that are shared between the two apps.

Comment 8

[Karsten Düsterloh]

Comment on attachment 352910 [details] [diff] [review]
[checked in] Part 2 - Most of SM address book

Changing all property and entity names seems a bit overkill, given there's no fundamental change...

>+++ b/mailnews/addrbook/resources/content/addressbook.xul
>-  <key id="key_printCard"  key="&printCardViewCmd.key;" command="cmd_printCard"  modifiers="accel"/>
>+  <key id="key_printCard"  key="&printContactViewCmd.key;"
>+       command="cmd_printCard"  modifiers="accel"/>

What about ids with "card" in it, like "key_printCard" or "button-editcard"?

I suppose commands names like "cmd_printCard" will be changed with the patch for the common parts?

>+                                oncommand="AbNewCard();"/>

Will functions like this change their name as well?
If not, the other (unnecessary) changes in this patch seem a bit pointless...

>+++ b/suite/locales/en-US/chrome/browser/mailNavigatorOverlay.dtd
>+<!ENTITY newContactCmd.label             "Address Book Contact…">

"Address Book" seems a bit superfluous here - which other "Contacts" do we have? It made sense to differ between ab cards and vcards, but "Address Book Contact" looks just odd...
(Same for all other "address book contacts".)

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> (From update of attachment 352910 [details] [diff] [review])
> Changing all property and entity names seems a bit overkill, given there's no
> fundamental change...

As I understand the general rule is that if you change the context of a string you should also change the entity name. This then gives localisers the choice of keeping the same string or changing the context of their own string - but they will know about the change rather than find out about it later.

> >+++ b/mailnews/addrbook/resources/content/addressbook.xul
> >-  <key id="key_printCard"  key="&printCardViewCmd.key;" command="cmd_printCard"  modifiers="accel"/>
> >+  <key id="key_printCard"  key="&printContactViewCmd.key;"
> >+       command="cmd_printCard"  modifiers="accel"/>
> 
> What about ids with "card" in it, like "key_printCard" or "button-editcard"?
> 
> I suppose commands names like "cmd_printCard" will be changed with the patch
> for the common parts?

I'm not proposing on renaming the backend. That would effectively require us renaming the c++ as well as all the js. That is unnecessary at the moment.

> >+                                oncommand="AbNewCard();"/>
> 
> Will functions like this change their name as well?
> If not, the other (unnecessary) changes in this patch seem a bit pointless...

As I said earlier, the other changes aren't unnecessary.

> >+++ b/suite/locales/en-US/chrome/browser/mailNavigatorOverlay.dtd
> >+<!ENTITY newContactCmd.label             "Address Book Contact…">
> 
> "Address Book" seems a bit superfluous here - which other "Contacts" do we
> have? It made sense to differ between ab cards and vcards, but "Address Book
> Contact" looks just odd...
> (Same for all other "address book contacts".)

Now I look at it again, I agree we can drop the "Address Book".

Comment 10

[Karsten Düsterloh]

(In reply to comment #9)
> > Changing all property and entity names seems a bit overkill,
> > given there's no fundamental change...
> 
> As I understand the general rule is that if you change the context of a string
> you should also change the entity name.

First, I'm well aware of the bad habit of letting broken L10N tools render XUL concepts useless - if entitiy/property names need to change even for minor changes to their content, there's almost no point in using them in the first place...

Second, I don't see a real context change here...

> This then gives localisers the choice of keeping the same string or 
> changing the context of their own string - but they will know about
> the change rather than find out about it later.

Localizers need to know about changes, no doubt about that.
But changing the names, because L10N tools don't properly recognize changes to the base language, is just bad...
 
KaiRo, is there any progress on fixing the tools?

> I'm not proposing on renaming the backend. That would effectively require us
> renaming the c++ as well as all the js. That is unnecessary at the moment.

"Unnecessary" is an odd term here with all these name changes around. :-D

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> Second, I don't see a real context change here...

So from what you say, there's no fundamental difference between "I'll just call my card" and "I'll just call my contact".

Comment 12

[Karsten Düsterloh]

We don't have that string. ;-)
What I'm saying is that IMO(!) most of the wording changes you made don't require a renaming of the property/entity.
But if KaiRo says that we *need* to do this, I can accept it. 
Grudgingly. 
Because I very much think it's the wrong thing to do...

Comment 13

[Robert Kaiser]

You really need to ask Axel on where we need to change IDs. The thing I can tell you is that the L10n dashboard won't show the need for a localizer to do anything if you don't change string IDs.

Comment 14

[Axel Hecht [:Pike]]

key name changes it is, a Card is not a Contact.

Karsten, there's no way to "fix tools", there needs to be some kind of per-entry versioning, and the way we do that is that the entry just appears once in history. Doing anything else would be a drastic change in our l10n infrastructure.

Comment 15

[Karsten Düsterloh]

Comment on attachment 352910 [details] [diff] [review]
[checked in] Part 2 - Most of SM address book

(In reply to comment #14)
> key name changes it is, a Card is not a Contact.

In this special case, in fact, it is/was.

> Karsten, there's no way to "fix tools", there needs to be some kind of
> per-entry versioning, and the way we do that is that the entry just appears
> once in history. Doing anything else would be a drastic change in our l10n
> infrastructure.

I know, we discussed this a while ago - and nothing happened, only more users/tools started to rely upon this really whacky workaround, harming code instead of fixing the real problem.

But since noone else cares, I shouldn't probably either.

r=me with the "Address Book Contact" issue mentioned in comment #8 addressed.

Comment 16

[[e-mail]]

Comment on attachment 352910 [details] [diff] [review]
[checked in] Part 2 - Most of SM address book

>+  <key id="key_printCard"  key="&printContactViewCmd.key;"
>+       command="cmd_printCard"  modifiers="accel"/>
Nit: might as well change to " key=" and " modifiers=" (one space)

>+        <toolbarbutton class="toolbarbutton-1" id="button-delete"
>+                       label="&deleteButton2.label;"  command="button_delete"
>+                       tooltiptext="&deleteButton2.tooltip;"/>
And " command=". I didn't spot any others though.

>-<!ENTITY editButton.label                               "Properties">  
>-<!ENTITY editButton.accesskey                           "P">  
>+<!ENTITY editButton2.label                              "Properties">  
>+<!ENTITY editButton2.accesskey                          "P">  
Nit: trailing whitespace (git-apply claims 6 whitespace errors)

>-<!ENTITY editButton.tooltip                             "Edit the selected card"> 
>+<!ENTITY editButton2.tooltip                            "Edit the selected item">
Eww, so we've got to rename three entities just to change one word? I'm wondering whether we should change to delete/edititemButton instead.

>-<!ENTITY  newCardCmd.accesskey            "c">
>+<!ENTITY  newContactCmd.label             "Address Book Contact…">
>+<!ENTITY  newContactCmd.accesskey         "c">
Nit: might as well change this to uppercase "C".

Comment 17

[[e-mail]]

(In reply to comment #8)
>(From update of attachment 352910 [details] [diff] [review])
>>+++ b/suite/locales/en-US/chrome/browser/mailNavigatorOverlay.dtd
>>+<!ENTITY newContactCmd.label             "Address Book Contact…">
>"Address Book" seems a bit superfluous here - which other "Contacts" do we
>have? It made sense to differ between ab cards and vcards, but "Address Book
>Contact" looks just odd...
>(Same for all other "address book contacts".)
I agree, just use "Contact" everywhere.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 352910 [details] [diff] [review]
[checked in] Part 2 - Most of SM address book

Checked in with comments addressed: http://hg.mozilla.org/comm-central/rev/a21dd6e6a6b2

Comment 19

[Vlado Valastiak [:wladow]]

Comment on attachment 352910 [details] [diff] [review]
[checked in] Part 2 - Most of SM address book

>+<!ENTITY printPreviewContactViewCmd.label               "Print Preview Contact">
>+<!ENTITY printPreviewContactViewCmd.accesskey           "v">
> <!-- LOCALIZATION NOTE (printCardViewCmd.key) : DONT_TRANSLATE -->  
>-<!ENTITY printCardViewCmd.key                           "P">
>+<!ENTITY printContactViewCmd.key                        "P">

Either fix the localization note or remove it - it's a bit needless because we shouldn't localize .keys in general.

> # status bar stuff
> ## LOCALIZATION NOTE (totalCardStatus): 
> ## %1$S is address book name, %2$S is card count
>-totalCardStatus=Total Cards in %1$S: %2$S
>+totalContactStatus=Total contacts in %1$S: %2$S

Fix the localization note

Comment 20

[Mark Banner (:standard8)]

Attachment: Final patch

This is the final part of the bug - the only remaining references to "Card" are in reference to vCard and hence I think its right that we keep card in those contexts.

This patch does the remaining instances and switches the drag/drop copy and move strings to use plural form.

Comment 21

[[e-mail]]

Comment on attachment 369562 [details] [diff] [review]
Final patch

>+## LOCALIZATION NOTE (contactsCopied): Semi-colon list of plural forms
>+## #1 is the number of contacts that were copied.
>+contactsCopied=#1 contact copied;#1 contacts copied
>+
>+## LOCALIZATION NOTE (contactsMoved): Semi-colon list of plural forms
>+## #1 is the number of contacts that were moved.
>+contactsMoved=#1 contact moved;#1 contacts moved
See attachment 359916 [details] [diff] [review] for an alternative way of using plural forms.

Comment 22

[Robert Kaiser]

(In reply to comment #21)
> See attachment 359916 [details] [diff] [review] for an alternative way of using plural forms.

Note that there's an open question and possible stringbundle issue noted in the m.d.a.thunderbird thread about plural forms, and I haven't tested the case Axel mentioned in his latest comment there. That case might possibly be more of an issue here than in the other bug.

Robert Kaiser

Comment 23

[Mark Banner (:standard8)]

(In reply to comment #22)
> (In reply to comment #21)
> > See attachment 359916 [details] [diff] [review] [details] for an alternative way of using plural forms.
> 
> Note that there's an open question and possible stringbundle issue noted in the
> m.d.a.thunderbird thread about plural forms, and I haven't tested the case Axel
> mentioned in his latest comment there. That case might possibly be more of an
> issue here than in the other bug.

I've just tested that case (as you'll see from the newsgroup), I couldn't see any failures with no replacement symbol in the string.

Comment 24

[Mark Banner (:standard8)]

Attachment: Final patch v2

Revised patch to use getFormattedString.

Comment 25

[Phil Ringnalda (:philor)]

Comment on attachment 369639 [details] [diff] [review]
Final patch v2

Sorry to be so slow - some nits, and one question I can't seem to answer.

>diff --git a/mail/locales/en-US/chrome/messenger/addressbook
>+## LOCALIZATION NOTE (contactsCopied): Semi-colon list of plural forms
>+## #1 is the number of contacts that were copied.
>+contactsCopied=1 contact copied;%1$S contacts copied
>+
>+## LOCALIZATION NOTE (contactsMoved): Semi-colon list of plural forms
>+## %1$S is the number of contacts that were moved.
>+contactsMoved=1 contact moved;%1$S contacts moved

The NOTE for the first one doesn't match the substitution marker; I guess now that it's been copied so much we're stuck with the nonsense "Semi-colon list of"? (which at least does keep people from trying to spell "separated" and failing); I'm pretty sure from trying to answer my question that you don't want to use %1$S unless you really have more than one, because it's much more expensive; I'm pretty sure you do want to use substitution for the "1".

What I don't know about is whether you want to use "#1" like you did for the SM strings. As Neil pointed out, it is being used fairly often for PluralForm strings, though I suspect that stems from copying an original use which for no apparent reason doesn't actually use getFormattedString to replace it (there are a surprising number of places which roll their own with .replace() instead). So I went looking for where getFormattedString replaces #1, to see if we were getting unexpected (or expected, I guess) semantics from it, and completely failed to see why it gets replaced at all. Can you make me happy about it?

>+PrintingContact=Printing contact

Not actually a nit, it's the right thing to say to match our use of contact, but that phrase is *so* confusing for anyone who's done photography.

Comment 26

[[e-mail]]

(In reply to comment #25)
> I'm pretty sure ... that you don't want to use %1$S unless
> you really have more than one, because it's much more expensive
So, more expensive then the JS caller doing a .replace("#1", count) ?

Comment 27

[Phil Ringnalda (:philor)]

Oh, the answer to my "where does getFormattedString replace '#1' since it just calls through to printf code that only knows about %" question is that it doesn't, and "#1 contacts copied" comes out the other end as "#1 contacts copied" so apparently _all_ of the many uses of #1 with PluralForm are hand-rolling getFormattedString, not just some.

Comment 28

[Phil Ringnalda (:philor)]

But, more expensive than .replace()? I'd be astounded. With "%1$S" versus "%S" you're just talking about an extra scan through the string (one to see if there are any numbered args, a second to capture them), plus some (rather inefficiently ordered for what really gets used) char by char scanning of the character or two after a %. For .replace(), no matter how optimized it might be for simple static patterns it's still dragging a whole regex engine behind it

Comment 29

[[e-mail]]

Note that the whole point of plural forms means that the coder doesn't know how many times the localiser is going to need to use %1$S.

Comment 30

[Robert Kaiser]

If you're using <stringbundle>s (and I think that's the case here), then using the stringbundle's formatted string functionality is safe and also a good idea, IMHO (why duplicate an existing mechanism with a workaround?). If you're using the XPCOM string bundle service, it's not safe yet, but will be if/when bug 288400 lands a JS-driven service to replace the C++ one.
I think using #1 and .replace() always was a bad idea here, it's a hack that duplicates what stringbundles can do internally already (IMHO, it would have made sense to extend stringbundles to deal with plural forms themselves). The really clean solution of doing all this via the L20n system will still come around some time in the future, I hope.

Comment 31

[Phil Ringnalda (:philor)]

Hrm? <stringbundle>.getFormattedString() just returns this.stringBundle.formatStringFromName(), where this.stringBundle is what you would expect it to be, Ci.nsIStringBundleService. If something works for one but not the other, then that's pretty magical, and where is the bug on it not working directly, and does it have an actual example of code+data that fails?

Comment 32

[Phil Ringnalda (:philor)]

(In reply to comment #29)
> Note that the whole point of plural forms means that the coder doesn't know how
> many times the localiser is going to need to use %1$S.

Well, two separate things.

One is that in the PluralForm.get(1, bundle.getFormattedString("foo", ["1"])) style, you do indeed not know how many instances of your substitution marker will be (pointlessly) replaced: getFormattedString might return the silly string "1 foo;1 foos" or the silly string "1 foo;1 foos;1 fooi;1 fooes", and you don't much care because you're going to throw the nonsense out with the PluralForm.get and you figure it's faster to have dosprintf do one to four substitutions than it is to have PluralForm pick out the right section, and then do just one substitution with .replace(). So long as dosprintf, which has no idea about the meaning of your semicolon separated string, does its job and replaces *every* instance of whatever marker you use with your single param, then nobody cares whether there's one, two, or four markers.

The other is that within each semicolon separated part of the string, %1$S only adds confusion if there is no %2$S: "%S foo;%S foos" will always be substituted the same way as "%1$S foo;%1$S foos" but because localizers know that they need to be careful with "%1$S of %2$S" in case their language would say that as "%2$S done %1$S", using "%1$S foo;%1$S foos" just makes every localizer have to study it to realize that there is really only one thing for each form. In "%2$S%% of %1$S file - Download Manager;%2$S%% of %1$S files - Download Manager" the numbered arg makes perfect sense because you are substituting two different params in each chunk of the string, but in "%1$S contacts copied;%1$S contacts copied" it only confuses (and makes dosprintf work harder).

Comment 33

[Phil Ringnalda (:philor)]

Comment on attachment 369639 [details] [diff] [review]
Final patch v2

Per comment 32, please make all of the strings "%S contact foo;%S contacts foo" and all of the l10n notes agree that it's a %S rather than a #1 or a %1$S.

Comment 34

[Mark Banner (:standard8)]

(In reply to comment #33)
> (From update of attachment 369639 [details] [diff] [review])
> Per comment 32, please make all of the strings "%S contact foo;%S contacts foo"
> and all of the l10n notes agree that it's a %S rather than a #1 or a %1$S.

Which won't work, because as https://developer.mozilla.org/En/XUL/Method/GetFormattedString says:

"...where each occurrence of %S (uppercase) is replaced by each successive element in the supplied array."

So if we carry on using this function, I have to use %1$S:

"Alternatively, numbered wildcards of the format %n$S (e.g. %1$S, %2$S, etc.) can be used to specify the position of corresponding parameter explicitly."

Comment 35

[Mark Banner (:standard8)]

Attachment: Final patch v3

Updated patch to use %1$S consistently everywhere (p.s. sorry for not remembering what I said in comment 34 earlier).

Comment 36

[Axel Hecht [:Pike]]

The localization notes should be more explicit that you're actually using %1$S multiple times on purpose and that just using %S is not going to work. This is a somewhat awkward twist of using the printf stuff and without explanation here, I expect it to go wrong.

I'm not sure if I like the slight twist in use here. #1 is out of the standard as well, but it's consistent with the use in other places.

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #36)
> The localization notes should be more explicit that you're actually using %1$S
> multiple times on purpose and that just using %S is not going to work. This is
> a somewhat awkward twist of using the printf stuff and without explanation
> here, I expect it to go wrong.

I can add that.

> I'm not sure if I like the slight twist in use here. #1 is out of the standard
> as well, but it's consistent with the use in other places.

<rant>
Whilst I really like to try and help l10n wherever possible, I'm starting to get depressed by it. The plural implementation doesn't seem to have been well thought out - anything that is proposed based on what is already there just gets rejected/complained about because there isn't a good implementation which is also performance optimised, as a result there are now various inconsistencies across the implementations we do have; and there is no c++ interface so we can't do it consistently across the whole of our applications.
</rant>

Comment 38

[Mark Banner (:standard8)]

Attachment: [checked in] Final patch v4

Now with slightly more explicit notes.

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 371431 [details] [diff] [review]
[checked in] Final patch v4

Checked in: http://hg.mozilla.org/comm-central/rev/510a939476cc

Comment 40

[Mark Banner (:standard8)]

All patches in -> fixed.