Closed Bug 550411 Opened 15 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird38+ fixed, seamonkey2.36 verified)

RESOLVED FIXED
Thunderbird 39.0
Tracking Status
thunderbird38 + fixed
seamonkey2.36 --- verified

People

(Reporter: World, Assigned: aceman)

References

()

Details

Attachments

(1 file, 1 obsolete file)

(deleted), patch
mkmelin
: review+
iannbugzilla
: 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 ?
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.
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.
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
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)
Attached patch patch (obsolete) (deleted) — 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)
(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)
Beware of copy-pasting... ;-)
(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)
Attached patch patch v2 (deleted) — Splinter 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+
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 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+
(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 on attachment 8574313 [details] [diff] [review] patch v2 Looks ok to me.
Attachment #8574313 - Flags: review?(iann_bugzilla) → review+
Component: Backend → Address Book
OS: Windows XP → All
Hardware: x86 → All
Version: 1.9.1 Branch → Trunk
Thanks to everybody involved.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
Also thanks to Thomas, who investigated these kind of issues in separate newer bugs.
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 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.

Attachment

General

Creator:
Created:
Updated:
Size: