550411 - "Type" of prefs entry of true/false at Config Editor which is defined in chrome://messenger/locale/messenger.properties is "string" instead of "boolean"

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[WADA:World Anti-bad-Duping Agency]

Build ID:
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8) Gecko/20100216 Thunderbird/3.0.2

"Type" of prefs entry of true/false at Config Editor which is defined via chrome://messenger/locale/messenger.properties is "string" instead of "boolean".
  mail.addr_book.displayName.lastnamefirst
  mail.addr_book.show_phonetic_fields
Will it produce some problems in future?

> http://mxr.mozilla.org/comm-central/source/mailnews/mailnews.js#217
> 217 // values for "mail.addr_book.lastnamefirst" are:
> 218 //0=displayname, 1=lastname first, 2=firstname first
> 219 pref("mail.addr_book.lastnamefirst", 0);
> 221 pref("mail.addr_book.displayName.lastnamefirst", "chrome://messenger/locale/messenger.properties");

> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#261
> 261 # generate display names in last first order
> 262 # valid mail.addr_book.displayName.lastnamefirst are: true or false
> 263 mail.addr_book.displayName.lastnamefirst=false

Comment 1

[Ludovic Hirlimann [:Usul]]

Wada how would you feel about providing a patch for this bug ?

Comment 2

[David von Oheimb]

Why doesn't anyone simply replace
pref("mail.addr_book.displayName.lastnamefirst", "chrome://messenger/locale/messenger.properties");
by the much more sensical
pref("mail.addr_book.displayName.lastnamefirst", false);

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> Why doesn't anyone simply replace
> pref("mail.addr_book.displayName.lastnamefirst",
> "chrome://messenger/locale/messenger.properties");
> by the much more sensical
> pref("mail.addr_book.displayName.lastnamefirst", false);

That would stop that preference being localised which would mean that the various locales couldn't set it to what is appropriate for their language.

Comment 4

[David von Oheimb]

Thanks Mark for your explanation. Interesting concept, but I'd consider this more a hack than a plain feature, since it is slightly confusing. Moreover it is not consistent with the definition that can be found in http://kb.mozillazine.org/Mail_and_news_settings where mail.addr_book.displayName.lastnamefirst clearly has the straightforward type "Boolean".
From the comments in bug #534487 I conclude that the value is considered false if it is any string different from "true". Hope this is better documented elsewhere.
BTW, mxr.mozilla.org, referred to above and from kb.mozillazine.org, is down.

Comment 5

[:aceman]

If anybody has privileges to fix th KB article, this needs to be done at http://kb.mozillazine.org/Mail_and_news_settings:
- mail.addr_book.displayName.lastnamefirst should be String and "Meaning" columns should contain that "true" and "false" (untranslated english words) are the only accepted values.
- mail.addr_book.lastnamefirst should be "Integer" and "Meaning" should be updated as
  "0=displayname, 1=lastname first, 2=firstname first", according to http://mxr.mozilla.org/comm-central/source/mailnews/mailnews.js#150 .

I'll see what can be done with the localization notes in the .properties files.
I'll fix the *.lastnamefirst prefs.
"mail.addr_book.show_phonetic_fields" is covered in bug 118624.

Comment 6

[:aceman]

While editing the KB article, please also change "mail.addr_book.show_phonetic_fields" in the same way as "mail.addr_book.displayName.lastnamefirst".

Comment 7

[Joe Sabash [:JoeS1]]

rsx11, could you help to correct this article please

Comment 8

[:aceman]

Attachment: patch

The pref mail.addr_book.lastnamefirst seems fine to me (as integer). It is not localizable but the default value of 0 seems to follow the display name which is localizable. It just needs to be fixed on the KB page.

The pref mail.addr_book.displayname.lastnamefirst needs to be fixed on KB too. This patch also improves the localization notes inside the .properties files so that translators better understand what to put there.

WADA, David, does this cover the reported problem?

Comment 9

[rsx11m]

(In reply to Joe Sabash [:JoeS1] from comment #7)
> rsx11, could you help to correct this article please

http://kb.mozillazine.org/index.php?title=Mail_and_news_settings&diff=46622&oldid=46213

aceman, can you make the same adjustments for mail.addr_book.show_phonetic_fields a few lines down?

Comment 10

[rsx11m]

Beware of copy-pasting... ;-)

Comment 11

[:aceman]

(In reply to rsx11m from comment #9)
> (In reply to Joe Sabash [:JoeS1] from comment #7)
> > rsx11, could you help to correct this article please
> 
> http://kb.mozillazine.org/index.
> php?title=Mail_and_news_settings&diff=46622&oldid=46213
>
Please also fix "mail.addr_book.lastnamefirst" as requested in comment 5.
I am not sure mentioning a "localized string" actually conveys the message that it should NOT be localized, but only strictly kept at the "true" and "false" English words. I wouldn't understand what the message says without knowing of this bug.

> aceman, can you make the same adjustments for
> mail.addr_book.show_phonetic_fields a few lines down?
Ok, I can pull those changes from bug 118624 here as this bug could be closed faster.

Comment 12

[:aceman]

Attachment: patch v2

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8574313 [details] [diff] [review]
patch v2

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

LGTM thx.

Looks like it's translated quite a bit... http://mxr.mozilla.org/l10n-central/search?string=mail.addr_book.displayName.lastnamefirst&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-central

Comment 14

[:aceman]

Comment on attachment 8574313 [details] [diff] [review]
patch v2

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

Yes it is. I plan to send a message to mozilla.dev.l10n .

Comment 15

[rsx11m]

Comment on attachment 8574313 [details] [diff] [review]
patch v2

Patch looks ok to me.

(In reply to :aceman from comment #11)
> Please also fix "mail.addr_book.lastnamefirst" as requested in comment 5.

Oops, missed that...

> I am not sure mentioning a "localized string" actually conveys the message
> that it should NOT be localized, but only strictly kept at the "true" and
> "false" English words. I wouldn't understand what the message says without
> knowing of this bug.

Yeah, "localized" isn't accurate. I've changed it into "locale-specific" which should convey what's meant. Since an enumeration is given {"true", "false"} and "representing a boolean value" is stated, it /should/ be unambiguous that these are the two values permitted. Since this doesn't appear to be the case in various locales, the KB entry now states "don't translate" explicitly.

http://kb.mozillazine.org/index.php?title=Mail_and_news_settings&diff=46626&oldid=46624

Comment 16

[:aceman]

(In reply to rsx11m from comment #15)
> Yeah, "localized" isn't accurate. I've changed it into "locale-specific"
> which should convey what's meant. Since an enumeration is given {"true",
> "false"} and "representing a boolean value" is stated, it /should/ be
> unambiguous that these are the two values permitted. Since this doesn't
> appear to be the case in various locales, the KB entry now states "don't
> translate" explicitly.
> 
> http://kb.mozillazine.org/index.
> php?title=Mail_and_news_settings&diff=46626&oldid=46624

OK, looks better to me now, thanks for fixing the page.

Comment 17

[Ian Neal]

Comment on attachment 8574313 [details] [diff] [review]
patch v2

Looks ok to me.

Comment 18

[:aceman]

Thanks to everybody involved.

Comment 19

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/dbb0d8ef7e8f

Comment 20

[:aceman]

Also thanks to Thomas, who investigated these kind of issues in separate newer bugs.

Comment 21

[:aceman]

We could get this into TB38 as it does not change the strings (IDs) themselves so localizers are not forced to retranslate them. I will just post to mozilla.dev.l10n in case they'd like to do it.

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 8574313 [details] [diff] [review]
patch v2

[Triage Comment]
Yes, lets land it for 38

Comment 24

[WADA:World Anti-bad-Duping Agency]

Sorry but I can say nothing on change, because I think string of true/false is pretty confusing. It true/false, should be Boolean, if not Boolean, is string, I prefer meaninghful text.

Comment 25

[Kent James (:rkent)]

https://hg.mozilla.org/releases/comm-aurora/rev/ab534427dce3