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

RESOLVED FIXED in Thunderbird 39.0

Status

defect
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: World, Assigned: aceman)

Tracking

Trunk
Thunderbird 39.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird38+ fixed, seamonkey2.36 verified)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

7.00 KB, patch
mkmelin
: review+
iann_bugzilla
: review+
rsx11m.pub
: feedback+
Details | Diff | Splinter Review
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
Wada how would you feel about providing a patch for this bug ?

Comment 2

8 years ago
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);
(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

8 years ago
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.
Assignee

Comment 5

4 years ago
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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Assignee

Comment 6

4 years ago
While editing the KB article, please also change "mail.addr_book.show_phonetic_fields" in the same way as "mail.addr_book.displayName.lastnamefirst".
Flags: needinfo?(jsabash)
rsx11, could you help to correct this article please
Flags: needinfo?(jsabash) → needinfo?(rsx11m.pub)
Assignee

Comment 8

4 years ago
Posted patch patch (obsolete) — Splinter Review
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?
Attachment #8574288 - Flags: feedback?(mueller8)
Attachment #8574288 - Flags: feedback?(m-wada)

Comment 9

4 years ago
(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?
Flags: needinfo?(rsx11m.pub) → needinfo?(acelists)

Comment 10

4 years ago
Beware of copy-pasting... ;-)
Assignee

Comment 11

4 years ago
(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.
Flags: needinfo?(acelists) → needinfo?(rsx11m.pub)
Assignee

Comment 12

4 years ago
Posted patch patch v2Splinter Review
Attachment #8574288 - Attachment is obsolete: true
Attachment #8574288 - Flags: feedback?(mueller8)
Attachment #8574288 - Flags: feedback?(m-wada)
Attachment #8574313 - Flags: review?(mkmelin+mozilla)
Attachment #8574313 - Flags: feedback?(rsx11m.pub)
Attachment #8574313 - Flags: feedback?(m-wada)
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
Attachment #8574313 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 14

4 years ago
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 .
Attachment #8574313 - Flags: review?(iann_bugzilla)

Comment 15

4 years ago
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
Flags: needinfo?(rsx11m.pub)
Attachment #8574313 - Flags: feedback?(rsx11m.pub) → feedback+
Assignee

Comment 16

4 years ago
(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

4 years ago
Comment on attachment 8574313 [details] [diff] [review]
patch v2

Looks ok to me.
Attachment #8574313 - Flags: review?(iann_bugzilla) → review+

Updated

4 years ago
Component: Backend → Address Book
OS: Windows XP → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Assignee

Comment 18

4 years ago
Thanks to everybody involved.
Keywords: checkin-needed

Comment 19

4 years ago
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/dbb0d8ef7e8f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
Assignee

Comment 20

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

Comment 21

4 years ago
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.
Assignee

Updated

4 years ago
Duplicate of this bug: 534487
Comment on attachment 8574313 [details] [diff] [review]
patch v2

[Triage Comment]
Yes, lets land it for 38
Attachment #8574313 - Flags: approval-comm-aurora+
Attachment #8574313 - Flags: feedback?(m-wada)
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.
You need to log in before you can comment on or make changes to this bug.