Closed Bug 319037 Opened 14 years ago Closed 11 years ago

Provide default value for mailnews.localizedRe Re:

Categories

(Thunderbird :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: bugzilla.mozilla.org-3, Assigned: dmose)

References

Details

(Keywords: intl)

Attachments

(3 files, 5 obsolete files)

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.
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)
Attachment #205555 - Flags: review?(bienvenu) → review+
Thanks for the review. Do Thunderbird patches need super-review? (is there a place I can find out without asking?)
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)
mscott: Any chance you could have a look at this patch anytime soon? Thanks :-)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #205555 - Attachment is obsolete: true
Attachment #259453 - Flags: superreview?(mscott)
Attachment #205555 - Flags: superreview?(mscott)
Is there anybody else who can do a superreview? This very simple patch has been waiting for superreview for 15 months now.
Attachment #259453 - Flags: superreview?(mscott) → superreview?(sspitzer)
Comment on attachment 259453 [details] [diff] [review]
Updated patch

forwarding review to mscott
Attachment #259453 - Flags: superreview?(sspitzer) → superreview?(mscott)
"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.
Cristian: can you update the patch to include the value property for the seamonkey too? Comment #8. 
Assignee: mscott → bugzilla.mozilla.org-1
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)
And maybe composeMsgs.properties is a better place...?
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.
Attachment #316458 - Flags: superreview?(mscott)
Keywords: intl
Attachment #316458 - Flags: superreview?(mscott) → superreview?(dmose)
Comment on attachment 316458 [details] [diff] [review]
Patch updated to also cover Seamonkey
[Checkin: Comment 17]

dmose?  if not, punt :)
dmose: r ping for the patch here
Flags: wanted-thunderbird3+
Urg, sorry for falling down on this.  My review queue is overloaded at the moment; I hope to start catching up soon.
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+
Keywords: checkin-needed
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]
Status: NEW → RESOLVED
Closed: 11 years ago
Depends on: 213004
Keywords: checkin-needed
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
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)...
Nobody actually tested the patch???
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 → ---
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?
Question: should I unclude colons when localizing the value, or not?
Rimas: no colons
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.
> 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.
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.
Whiteboard: [needs input dmose]
Whiteboard: [needs input dmose] → [needs input dmose][no l10n impact]
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.
Whiteboard: [needs input dmose][no l10n impact] → [no l10n impact]
> 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.
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]
Whiteboard: [no l10n impact][waiting on unit test] → [no l10n impact][waiting on unit test][target slushy]
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
I don't mind :-)
Attachment #355001 - Flags: superreview?(bienvenu)
Attachment #355001 - Flags: review?(bienvenu)
Attachment #355001 - Flags: superreview?(bienvenu)
Attachment #355001 - Flags: superreview+
Attachment #355001 - Flags: review?(bienvenu)
Attachment #355001 - Flags: review+
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]
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
(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.
Attachment #363205 - Flags: superreview?(bienvenu)
Attachment #363205 - Flags: review?(bienvenu)
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.
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.
Attachment #363205 - Flags: superreview?(bienvenu)
Attachment #363205 - Flags: superreview+
Attachment #363205 - Flags: review?(bienvenu)
Attachment #363205 - Flags: review+
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.
Status: REOPENED → ASSIGNED
Whiteboard: [no l10n impact][needs unit test] → [no l10n impact][needs updated patch; landing]
Whiteboard: [no l10n impact][needs updated patch; landing] → [m3][no l10n impact][needs updated patch; landing]
Changeset 2ccafa3b5bf7 pushed with bienvenu's comments addressed.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [m3][no l10n impact][needs updated patch; landing]
(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.
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 → ---
Status: REOPENED → ASSIGNED
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
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.
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+
Uploaded the wrong version of the patch last time around; retrying.
Attachment #371961 - Attachment is obsolete: true
Depends on: 488229
No longer depends on: 213004
Whiteboard: [submitted to tryserver, which appears to be having issues]
No longer depends on: 488229
Whiteboard: [submitted to tryserver, which appears to be having issues] → [submitted to tryserver; awaiting results]
Re-landed.  Since the tryserver seemed happy, this should stick.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Depends on: 488971
Whiteboard: [submitted to tryserver; awaiting results]
add Re: to summary for searchability
Summary: Provide default value for mailnews.localizedRe → Provide default value for mailnews.localizedRe Re:
Duplicate of this bug: 634896
You need to log in before you can comment on or make changes to this bug.