Closed
Bug 319037
Opened 19 years ago
Closed 15 years ago
Provide default value for mailnews.localizedRe Re:
Categories
(Thunderbird :: Preferences, defect)
Thunderbird
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b3
People
(Reporter: bugzilla.mozilla.org-3, Assigned: dmosedale)
References
Details
(Keywords: intl)
Attachments
(3 files, 5 obsolete files)
3.78 KB,
patch
|
dmosedale
:
superreview+
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
10.32 KB,
patch
|
Details | Diff | Splinter Review |
The pref mailnews.localizedRe allows Thunderbird to recognise non-standard versions of "Re:" used by some broken mail clients, e.g. non-English versions of MS Outlook. This was implemented in bug 213004. Optimally Thunderbird should recognize all such non-standard versions of Re (I imagine it's not that many strings afterall). Adding a long list to the default value of mailnews.localizedRe may be controversial, though. A less pervasive solution is to specify a default value in localized builds of Thunderbird that corresponds to the versions in use in the specific language area. This will not help non-English users using the English version of Thunderbird, though.
Reporter | ||
Comment 1•19 years ago
|
||
Other possible improvements include specifying the default value for the default (US English) builds, and adding UI for this option.
Attachment #205555 -
Flags: review?(bienvenu)
Updated•19 years ago
|
Attachment #205555 -
Flags: review?(bienvenu) → review+
Reporter | ||
Comment 2•19 years ago
|
||
Thanks for the review. Do Thunderbird patches need super-review? (is there a place I can find out without asking?)
Comment 3•19 years ago
|
||
Comment on attachment 205555 [details] [diff] [review] Allow localizers to specify default value it depends on the patch :-) In this case, I'm going to check with mscott - I don't think he'll have a problem with it, but I just want to run it past him, since this patch makes it so localized builds work a little differently from each other. He'll be back from vacation in a few days.
Attachment #205555 -
Flags: superreview?(mscott)
Reporter | ||
Comment 4•18 years ago
|
||
mscott: Any chance you could have a look at this patch anytime soon? Thanks :-)
Reporter | ||
Comment 5•17 years ago
|
||
Attachment #205555 -
Attachment is obsolete: true
Attachment #259453 -
Flags: superreview?(mscott)
Attachment #205555 -
Flags: superreview?(mscott)
Reporter | ||
Comment 6•17 years ago
|
||
Is there anybody else who can do a superreview? This very simple patch has been waiting for superreview for 15 months now.
Reporter | ||
Updated•17 years ago
|
Attachment #259453 -
Flags: superreview?(mscott) → superreview?(sspitzer)
Comment 7•17 years ago
|
||
Comment on attachment 259453 [details] [diff] [review] Updated patch forwarding review to mscott
Attachment #259453 -
Flags: superreview?(sspitzer) → superreview?(mscott)
Comment 8•17 years ago
|
||
"mailnews.localizedRe=" should go into http://lxr.mozilla.org/seamonkey/source/suite/locales/en-US/chrome/mailnews/region.properties also, no? This sounds like it could improve the threading experience.
Comment 9•16 years ago
|
||
Cristian: can you update the patch to include the value property for the seamonkey too? Comment #8.
Assignee: mscott → bugzilla.mozilla.org-1
Comment 10•16 years ago
|
||
Comment on attachment 259453 [details] [diff] [review] Updated patch Suite properties file needs a value too.
Attachment #259453 -
Attachment is obsolete: true
Attachment #259453 -
Flags: superreview?(mscott)
Comment 11•16 years ago
|
||
And maybe composeMsgs.properties is a better place...?
Reporter | ||
Comment 12•16 years ago
|
||
Sorry for the delay. I somehow missed the activity in this bug in January.
>And maybe composeMsgs.properties is a better place...?
The pref isn't just used when replying but also for display in the thread pane, so I don't think that is composeMsgs.properties is better a place. But I don't know.
Reporter | ||
Updated•16 years ago
|
Attachment #316458 -
Flags: superreview?(mscott)
Updated•16 years ago
|
Attachment #316458 -
Flags: superreview?(mscott) → superreview?(dmose)
Comment 13•16 years ago
|
||
Comment on attachment 316458 [details] [diff] [review] Patch updated to also cover Seamonkey [Checkin: Comment 17] dmose? if not, punt :)
Assignee | ||
Comment 15•16 years ago
|
||
Urg, sorry for falling down on this. My review queue is overloaded at the moment; I hope to start catching up soon.
Assignee | ||
Comment 16•15 years ago
|
||
Comment on attachment 316458 [details] [diff] [review] Patch updated to also cover Seamonkey [Checkin: Comment 17] Looks good; thanks for the patch, and sorry for the delay on our end. sr=dmose. I do agree with comment 1 that we can probably do even better in the future. I'm also curious as to what the likely controversy around just having a really long list would be -- are you concerned about false positives, or something else?
Attachment #316458 -
Flags: superreview?(dmose) → superreview+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 17•15 years ago
|
||
Comment on attachment 316458 [details] [diff] [review] Patch updated to also cover Seamonkey [Checkin: Comment 17] http://hg.mozilla.org/comm-central/rev/bb54cade7709
Attachment #316458 -
Attachment description: Patch updated to also cover Seamonkey → Patch updated to also cover Seamonkey
[Checkin: Comment 17]
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Depends on: 213004
Keywords: checkin-needed
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
Comment 18•15 years ago
|
||
Comment on attachment 316458 [details] [diff] [review] Patch updated to also cover Seamonkey [Checkin: Comment 17] >+pref("mailnews.localizedRe", "chrome://messenger-region/locale/region.properties"); Nice try but unfortunately the back end code uses GetCharPref instead of GetComplexValue(nsIPrefLocalizedString)...
Comment 19•15 years ago
|
||
Nobody actually tested the patch???
Assignee | ||
Comment 20•15 years ago
|
||
Doh! Indeed. Reopening, and marking as blocking, since we now need to either back this out or fix it.
Status: RESOLVED → REOPENED
Flags: wanted-thunderbird3+ → blocking-thunderbird3+
Resolution: FIXED → ---
Assignee | ||
Comment 21•15 years ago
|
||
As penance for not catching this; here's a not-yet-tested patch that adjusts the backend. However, several things remain to be done: * it's not clear to me whether the subject passed in to NS_MsgStripRE is guaranteed to have been RFC 2047 decoded already. Is it? If not, can that cause us problems? * what happens if somebody already has this pref set to a non-default value in prefs.js? Will this code fall over because it's expecting a localized pref now? If so, I suspect the right course is to switch to a different pref name. * I think we need a C++ unit test here. Christian, would you be willing to take these items and run with them?
Comment 22•15 years ago
|
||
Question: should I unclude colons when localizing the value, or not?
Comment 23•15 years ago
|
||
Rimas: no colons
Comment 24•15 years ago
|
||
Re comment 21: * No, the subject is mime encoded - so the pref won't work if the localized Re: prefix is non-ascii (and encoded). I don't know if there are such prefixes in use, all the ones I've seen are ascii. Anyway, the only "problem" it will cause is to do nothing at all. * Having the pref set already doesn't cause problems.
Reporter | ||
Comment 25•15 years ago
|
||
> Nobody actually tested the patch??? My bad :-/ I did test that the value showed up in about:config, though, but apparently that wasn't enough. I can confirm that attachment 355001 [details] [diff] [review] fixes the problem. I have tested with the following subjects with localizedRe set to "SV,ÆØÅ", and the prefix was detected properly in all cases: Subject: SV: =?ISO-8859-1?Q?=C6blegr=F8d?= (SV: Æblegrød) Subject: =?ISO-8859-1?Q?SV=3A=C6blegr=F8d?= (SV:Æblegrød) Subject: =?ISO-8859-1?Q?=C6=D8=C5=3A_Foo_bar?= (ÆØÅ: Foo bar) Subject: =?ISO-8859-1?Q?=C6=D8=C5=3AFoo_bar?= (ÆØÅ:Foo bar) (In reply to comment #16) > I'm also curious as to what the likely controversy around just having > a really long list would be -- are you concerned about false positives, or > something else? The list may get very long if it contains all more or less widespread "Re" variants for every language on planet. Some of these may not look like a regular prefix (e.g. the Danish language version of Lotus Notes uses "Vedr.:"), so in case of a false positive the result is particularly confusing. I imagine that adding a list of hundreds of variants would be controversial - no? The list doesn't have to be complete to be useful, though, so shipping a fairly short list by default may solve the majority of mails with non-standard Re equivalents. I'll look into writing a unit test. I haven't written tests for Thunderbird/Firefox before.
Assignee | ||
Comment 26•15 years ago
|
||
My hope is that it won't be too hard to figure out how to test this using an xpcshell unit test that drives the code in question from JS and is run with "make check". <https://wiki.mozilla.org/MailNews:Automated_Testing> has pointers.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs input dmose]
Updated•15 years ago
|
Whiteboard: [needs input dmose] → [needs input dmose][no l10n impact]
Assignee | ||
Comment 27•15 years ago
|
||
Christian, any chance you would be willing to try and write a unit test for this within the next week? W.r.t. the long list, I think it might be a bit complicated (mostly relating to false positives, as you say), but I'm not convinced that makes it not worth doing.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs input dmose][no l10n impact] → [no l10n impact]
Reporter | ||
Comment 28•15 years ago
|
||
> Christian, any chance you would be willing to try and write a unit test for
> this within the next week?
Not within the next week, I'm afraid. But possibly the weekend after that.
Comment 29•15 years ago
|
||
Since what exists now is broken and as stated, this either fixes it or does nothing we should probably land this. We should aim to commit this patch by the freeze date and continuing to let this bug block tb3 waiting on the unit test post freeze / release.
Whiteboard: [no l10n impact] → [no l10n impact][waiting on unit test]
Updated•15 years ago
|
Whiteboard: [no l10n impact][waiting on unit test] → [no l10n impact][waiting on unit test][target slushy]
Assignee | ||
Comment 30•15 years ago
|
||
Since we'd like to see this land by Thursday, I'm going to grab this and run with it under the assumption that Christian won't mind. Christian, please ping me either here or in email if that's not the case.
Assignee: bugzilla.mozilla.org-1 → dmose
Reporter | ||
Comment 31•15 years ago
|
||
I don't mind :-)
Assignee | ||
Updated•15 years ago
|
Attachment #355001 -
Flags: superreview?(bienvenu)
Attachment #355001 -
Flags: review?(bienvenu)
Updated•15 years ago
|
Attachment #355001 -
Flags: superreview?(bienvenu)
Attachment #355001 -
Flags: superreview+
Attachment #355001 -
Flags: review?(bienvenu)
Attachment #355001 -
Flags: review+
Comment 32•15 years ago
|
||
Comment on attachment 355001 [details] [diff] [review] patch to fix backend, v1 [checked in] http://hg.mozilla.org/comm-central/rev/d05b4e440745
Attachment #355001 -
Attachment description: patch to fix backend, v1 → patch to fix backend, v1 [checked in]
Comment 33•15 years ago
|
||
Thanks to Phil for checking this in. This is just waiting on the unit test now and we can target that piece for the b3 release.
Severity: enhancement → normal
Whiteboard: [no l10n impact][waiting on unit test][target slushy] → [no l10n impact][needs unit test]
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Comment 34•15 years ago
|
||
(In reply to comment #21) > * what happens if somebody already has this pref set to a non-default value in prefs.js? Localisation only affects default or locked values, not user set values.
Assignee | ||
Comment 35•15 years ago
|
||
Attachment #363205 -
Flags: superreview?(bienvenu)
Attachment #363205 -
Flags: review?(bienvenu)
Comment 36•15 years ago
|
||
Comment on attachment 363205 [details] [diff] [review] unit tests and documentation fixup for localizedRe code NS_ARRAY_LENGTH from nsMemory.h is nicer than +# define NUM_TESTS 4 + } else { + if (strcmp(expectedOutput, encodedInout)) { + return 4; + } + } you can use else if (strcmp...) here and get rid of some braces. you can also lose the braces here: + // make sure we got the right results + if (didModify != expectedDidModify) { + return 2; + } I'll give this patch a try in a few minutes.
Comment 37•15 years ago
|
||
when I run make check, I see: /Users/davidbienvenu/tbirdcheck/config/rules.mk:1116: target `TestMsgStripRE' given more than once in the same rule. /Users/davidbienvenu/tbirdcheck/config/rules.mk:1116: target `TestMsgStripRE' given more than once in the same rule. and I don't see the test run, and make check never completes. This is on a mac.
Updated•15 years ago
|
Attachment #363205 -
Flags: superreview?(bienvenu)
Attachment #363205 -
Flags: superreview+
Attachment #363205 -
Flags: review?(bienvenu)
Attachment #363205 -
Flags: review+
Comment 38•15 years ago
|
||
Comment on attachment 363205 [details] [diff] [review] unit tests and documentation fixup for localizedRe code if I remove all the cruft about CPP_UNIT_TESTS being broken, i.e., +# XXXdmose Note that the comm-central build system's CPP_UNIT_TESTS +# infrastructure is currently busted in some way that has turned out to be a +# bunch of work to track down. Rather than blocking TestMsgStripRE on +# that being un-horked, I've cargo-culted over some fragile code from +# one of the IMAP tests that's "good enough". Fixing the underlying bug should +# allow us to get rid of the hackery here. +CPPSRCS = $(foreach src,$(CPP_UNIT_TESTS),$(src).cpp) + +SIMPLE_PROGRAMS = $(CPPSRCS:.cpp=$(BIN_SUFFIX)) and +check:: + @echo "Running MailNews base compiled CPP tests" + @$(EXIT_ON_ERROR) \ + for f in $(CPP_UNIT_TESTS); do \ + XPCOM_DEBUG_BREAK=stack-and-abort $(RUN_TEST_PROGRAM) $(DIST)/bin/$$f; \ + done then make check works fine. So r/sr=me, with those changes, and the previous ones I mentioned.
Assignee | ||
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Whiteboard: [no l10n impact][needs unit test] → [no l10n impact][needs updated patch; landing]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [no l10n impact][needs updated patch; landing] → [m3][no l10n impact][needs updated patch; landing]
Assignee | ||
Comment 39•15 years ago
|
||
Changeset 2ccafa3b5bf7 pushed with bienvenu's comments addressed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Whiteboard: [m3][no l10n impact][needs updated patch; landing]
Comment 40•15 years ago
|
||
(In reply to comment #39) > Changeset 2ccafa3b5bf7 pushed with bienvenu's comments addressed. Note this got backed out due to build failures. Dan, I think you need to add a section at the start of the file: ifndef MOZ_INCOMPLETE_EXTERNAL_LINKAGE MOZILLA_INTERNAL_API = 1 endif in a similar way to http://mxr.mozilla.org/comm-central/source/mailnews/imap/test/Makefile.in Why it worked without that on Linux & Windows, I'm not quite sure.
Assignee | ||
Comment 41•15 years ago
|
||
That was one of the things I tried before backing out; while it may be necessary for the fix, it's unfortunately not sufficient. I'll add updated patches and more info here soon, as I suspect your insights are likely to be very helpful in figuring out the underlying issues here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Comment 42•15 years ago
|
||
I had a short time where I was waiting for my main dev builds to finish, so I took a quick shot at fixing the test. The main changes were the switch to include the internal api, nsStringAPI.h -> nsStringGlue.h in the test code, and also added some more REQUIRES options.
Attachment #363205 -
Attachment is obsolete: true
Assignee | ||
Comment 43•15 years ago
|
||
Thanks, Mark! My mistake was letting the perfect (don't pull in the internal API) get in the way of the good (hey, let's get this thing tested!). I'll update with the other changes and requested by David and the compilers and land soon.
Assignee | ||
Comment 44•15 years ago
|
||
Updated with bienvenu's comments; sending to the try server now.
Attachment #371864 -
Attachment is obsolete: true
Attachment #371961 -
Flags: superreview+
Attachment #371961 -
Flags: review+
Assignee | ||
Comment 45•15 years ago
|
||
Uploaded the wrong version of the patch last time around; retrying.
Attachment #371961 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Updated•15 years ago
|
No longer depends on: 488229
Whiteboard: [submitted to tryserver, which appears to be having issues] → [submitted to tryserver; awaiting results]
Assignee | ||
Comment 46•15 years ago
|
||
Re-landed. Since the tryserver seemed happy, this should stick.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: in-testsuite+
Updated•15 years ago
|
Whiteboard: [submitted to tryserver; awaiting results]
Comment 47•15 years ago
|
||
add Re: to summary for searchability
Summary: Provide default value for mailnews.localizedRe → Provide default value for mailnews.localizedRe Re:
You need to log in
before you can comment on or make changes to this bug.
Description
•