Closed Bug 213004 Opened 17 years ago Closed 16 years ago

Localized "Re:" strings are not removed when Replying

Categories

(MailNews Core :: Composition, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbockelkamp, Assigned: mbockelkamp)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 10 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030715
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030715

German Outlook doesn't use "Re:" by default when replying. It appends "AW:" to
the existing header. When using Mozilla to reply to such a messege, the "AW:"
isn't removed, so the subject then is "Re: AW: ...".

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Attached patch patch (obsolete) — Splinter Review
The attached patch additionally checks for a user set String (pref:
mailnews.localizedRe) and removes it. The "Re:" check is still static so that
noone can prevent correct handling of the standard string.
Attachment #127967 - Flags: review?(neil.parkwaycc.co.uk)
Related/dup of bug 29179 / bug 108163
Status: UNCONFIRMED → NEW
Ever confirmed: true
This looks like a patch for bug 29179, but it will only work with a single
'localised Re'. But there could be many, like 'Sv:', 'Odp:', 'Re(2):', ...
Wouldn't it be better to have a full list, like suggested by bug 29179 comment
12 ? After all, the subject-header is determined by the sender of the message,
and he/she might not be using the same software, or he/she might be using a
different language.

Note that a mailer should normally only use 'Re' to indicate a reply, all these
localised prefixes aren't allowed. But a mailer is allowed to remove them (see
bug 29179 comment 7), and that's what this patch is trying to do.
Comment on attachment 127967 [details] [diff] [review]
patch

Your pref foo looks wrong (but find someone else to help you out), in fact I
think that you should use a pref localized string so that translators can
automagically provide a useful default. You also don't free lr when you have
finished with it.

Never compare to PR_TRUE (or PR_FALSE).

And you've got toupper(lr[0]) in a loop which is wrong. I'm not sure that
toupper is locale-safe anyway. You'll need a locale expert to figure out the
best way to do this.
Attachment #127967 - Flags: review?(neil.parkwaycc.co.uk) → review-
Problems...

I had a look at local prefs, but I thinks that is the wrong way. I'm living in
Germany, but I use english Mozilla builds. So I would have the same problem as
before. The thing is: This pref must not be dependent on the language of Mozilla
or the language of the user's OS or the language the user is speaking. It
depends on the persons who are sending emails to the user. E.g. if I get an
email from german Outlook, it will have "AW:" instead of "Re:", if I get one
from france Outlook, it will have "Ré:" etc. So this must either include _all_
possible localizations (which I would not recommend), or it must be a normal
user-editable pref, which should be empty by default and then be filled by the user.

Further, I sent some mails to netscape.public.mozilla.i18n
<bfdr6r$dq1lg$1@ID-19347.news.uni-berlin.de> and netscape.public.mozilla.prefs
<bfdqcn$dg9tt$1@ID-19347.news.uni-berlin.de>, but didn't get an answer yet. So I
thougt about writing a new function, but I saw that subjects with special chars
are MIME encoded and therefore converted to ASCII before. But those special
chars are not part of ASCII. This should perhaps be commented better in source
(whether ANSI oder extended DOS-ASCII or whatever is used or if the whole thing
is "illegal"). Perhaps the solutions is to make the pref case-dependent.
Depends on: 216588
Attached patch patch v2 (obsolete) — Splinter Review
Please see Comment #5, first paragraph.
Attachment #127967 - Attachment is obsolete: true
Comment on attachment 130192 [details] [diff] [review]
patch v2

Btw. does anyone know a better name for the pref?
Attachment #130192 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #130192 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #130192 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview?(scott)
Comment on attachment 130192 [details] [diff] [review]
patch v2

I don't think this will work for localizedRe strings that start with the
letters Re, you need to check s[reLength] == ':' etc. immediately the strncmp
succeeds.
Also PL_strfree(lr) is wrong when you have lr = "Re,RE,re,rE" or lr = tmp = new
char[].
Consider using PR_smprintf to join the strings (this allocates, use
PR_smprintf_free), also use it in the else case, thus avoiding the strdup.
Attachment #130192 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch patch v3 (obsolete) — Splinter Review
The strdup() is necessary because strtok() replaces the delimiters with 0s.
Attachment #130192 - Attachment is obsolete: true
Attachment #130956 - Flags: superreview?(scott)
Attachment #130956 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #130192 - Flags: superreview?(scott)
question: shouldn't the pref for the Re text really be a localized pref string?
That is, the value in mailnews.js should point to a chrome url such as
messenger's region.properties.

Localizers can then change Re: to wahtever they want (such as AW:) when
localizing the build by changing this string in region.properties. 
Please see Comment #5, first paragraph. I think that this pref should really be
empty by default to make some pressure on the authors of other Mail programs. If
e.g. for german builds "AW" were set by default, no german Outlook user would
himself switch to "Nachrichtenkennzeichnungen in Englisch" which I'm now able to
persuade about the half to.
Comment on attachment 130956 [details] [diff] [review]
patch v3

No, you don't need the strdup any more, because the PR_smprintf allocates
memory so it's safe for strtok to overwrite it. Also the free of lrToken is
wrong. And I still don't understand why you don't do the rest of the
(?:[[(]\d+[])])?: (optional bracketed number before colon) test inside the
token loop.
Attachment #130956 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch patch v4 (obsolete) — Splinter Review
Let me try to explain why the strdup() _is_ necessary. As you wrote, the
PR_smprintf() allocates memory so it is safe for strtok() to overwrite it. But
there is the label AGAIN: and the GOTOs. Every time this "loop" begins, the
lrWithRe must be tokenized again, so the changes made to it by strtok() are not
acceptable. This is the cause why I make the temporary copy lrWithReTmp.
Attachment #130956 - Attachment is obsolete: true
Attachment #131228 - Flags: superreview?(scott)
Attachment #131228 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #130956 - Flags: superreview?(scott)
Comment on attachment 131228 [details] [diff] [review]
patch v4

Ah yes, but I couldn't understand the previous patches well enough to figure
out what you were doing.
Comment on attachment 131228 [details] [diff] [review]
patch v4

>+  PRInt32 tokenLength = 0;
I think the declaration belongs here:
>+    tokenLength = strlen(lrToken);
I thought it would be better to not put it in the loop (I'm not using C++ for
long).  I'll attach a changed version.
Attached patch patch v4.1 (obsolete) — Splinter Review
Attachment #131228 - Attachment is obsolete: true
Attachment #131235 - Flags: superreview?(scott)
Attachment #131235 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #131228 - Flags: superreview?(scott)
Attachment #131228 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 131235 [details] [diff] [review]
patch v4.1

Almost there!

>+  // get localizedRe pref
>+  nsresult rv;
>+  nsCOMPtr<nsIPrefBranch> prefBranch =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
Actually you copied the wrong way to get a pref branch. Use
nsCOMPtr<nsIPrefService> prefService = // do_GetService stuff;
nsCOMPtr<nsIPrefBranch> prefBranch;
rv = prefService->GetBranch(nsnull, getter_AddRefs(prefBranch));
See e.g. extensions/pref/autoconfig/src/nsReadConfig.cpp, line 160

>+  if (NS_SUCCEEDED(rv) && lr) 
>+  {
>+    lrWithRe = PR_smprintf("Re,RE,re,rE,%s",lr);	
>+  } else {
>+    lrWithRe = PR_smprintf("Re,RE,re,rE");	
>+  }
>+  PL_strfree(lr);
That belongs in the success case, PL_strfree is not null-safe.

>+  while (lrToken!=NULL)
while (lrToken) will do.

Did you realize that this code affects the thread pane as well?
i.e. it changes AW: your message to Re: your message
Attachment #131235 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch patch v4.2 (obsolete) — Splinter Review
> Did you realize that this code affects the thread pane as well?
> i.e. it changes AW: your message to Re: your message

Yes, but I think this is another bug because we are already displaying "Re:"
instead of "re:", "RE:", "Re[2]:", "Re(2):" etc.

A quick look at lxr makes me believe the "Re:" part isn't stored in the MSFs.
Changing that could perhaps be done together with Bug #195737.
Attachment #131235 - Attachment is obsolete: true
Attachment #131248 - Flags: superreview?(scott)
Attachment #131248 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #131235 - Flags: superreview?(scott)
Attachment #131248 - Flags: review?(neil.parkwaycc.co.uk) → review+
(In reply to comment #19)
>Created an attachment (id=131248)
>patch v4.2
>>Did you realize that this code affects the thread pane as well?
>>i.e. it changes AW: your message to Re: your message
>Yes, but I think this is another bug because we are already displaying "Re:"
>instead of "re:", "RE:", "Re[2]:", "Re(2):" etc.
Ah in fact this is a feature :-)
Comment on attachment 131248 [details] [diff] [review]
patch v4.2

I'll sr this...
Attachment #131248 - Flags: superreview?(mscott) → superreview?(bienvenu)
Comment on attachment 131248 [details] [diff] [review]
patch v4.2

I think this would be cleaner w/o strtok:

const char *tokPtr = lrWithReTmp;
for (tokenLength = 0; *tokPtr != ',' && *tokPtr; tokenLength++, tokPtr++)
then you have tokenLength, and you can just keep advancing tokPtr, and reset it
when you've stripped something...
Neil points out that you'd need to be careful about badly formed pref strings,
e..g, leading ',', or ",," but I think just continuing if the token length is
zero would handle that.
Attachment #131248 - Attachment is obsolete: true
Attachment #131248 - Flags: superreview?(bienvenu)
Attachment #131248 - Flags: review+
Attached patch Patch v5 (obsolete) — Splinter Review
Assignee: sspitzer → mbockelkamp
Status: NEW → ASSIGNED
Attachment #151571 - Flags: superreview?(bienvenu)
Attachment #151571 - Flags: review?(neil.parkwaycc.co.uk)
  nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));

you can get the pref branch all at once...

other than that, looks OK...thx for doing this. I'll wait for Neil to do his
review first...
+  char *lr = 0;
...
+      rv = prefBranch->GetCharPref("mailnews.localizedRe", &lr);

Why don't you use an nsXPIDLCString instead and then you don't need to worry
about when to free it....

nsXPIDLCString localizedRe;
...
rv = prefBranch->GetCharPref("mailnews.localizedRe", getter_Copies(localizedRe));


+  if (NS_SUCCEEDED(rv) && lr) 
+  {
+    lrWithRe = PR_smprintf("Re,RE,re,rE,%s",lr);	
+    PL_strfree(lr);
+  } else {
+    lrWithRe = PR_smprintf("Re,RE,re,rE");	
+  }

You can get rid of this malloc altogether (too much memory allocation already in
mozilla). It also relieves you have needing to know when to delete the string.

nsCAutoString checkString("Re,RE,re,rE");
if (!localizedRe.IsEmpty()) 
  checkString.Append(NS_LITERAL_CSTRING(",") + localizedRe);


What is the reason that we don't check using case insensitive comparisons
instead of 4 different checks against "re" "RE" "Re" and "rE". The last one
especially is surely invalid and could be ignored. Would case insensitive
comparison be a problem for the localized Re? 
Attachment #151571 - Attachment is obsolete: true
Attachment #151571 - Flags: superreview?(bienvenu)
Attachment #151571 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v5a (obsolete) — Splinter Review
Comment on attachment 151594 [details] [diff] [review]
Patch v5a

> you can get the pref branch all at once...

done.

> Why don't you use an nsXPIDLCString instead and then you don't need to worry
about when to free it....

done.

> You can get rid of this malloc altogether (too much memory allocation already in
mozilla). It also relieves you have needing to know when to delete the string.

done.

> What is the reason that we don't check using case insensitive comparisons
instead of 4 different checks against "re" "RE" "Re" and "rE". The last one
especially is surely invalid and could be ignored. Would case insensitive
comparison be a problem for the localized Re? 

There seems to be no function to covert the case of special chars used in some
Languages. Also see comment #5. 

About the "rE": That is just what the old code was doing: Compare the first
character to 'r' and 'R' and then compare the second character to 'e' and 'E'.
As far as I've read the RFC822 it doesn't say anything about the case, "Re:" is
used everywhere.
Attachment #151594 - Flags: superreview?(bienvenu)
Attachment #151594 - Flags: review?(neil.parkwaycc.co.uk)
+  const char *tokPtr = ToNewCString(checkString);

You are currently leaking one copy of checkString every time you go through the 
loop. Because you aren't modifying the string you only need to use get() now.

const char *tokPtr = checkString.get();

+        /* Skip forward over digits after the "[". */
+        while (s2 < (s_end - tokenLength) && IS_DIGIT(*s2))
+        s2++;

The (s_end - tokenLength) here is wrong. The length of the token doesn't have 
any relevance here. You are looking for only 3 non-null bytes e.g. "1]:" So you 
should do the same as the old code and compare to s2 < (s_end - 2). (2 because 
s_end points at the null byte).
Attachment #151594 - Attachment is obsolete: true
Attachment #151594 - Flags: superreview?(bienvenu)
Attachment #151594 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v5b (obsolete) — Splinter Review
Comment on attachment 151606 [details] [diff] [review]
Patch v5b

> You are currently leaking one copy of checkString every time you go through the 
loop. Because you aren't modifying the string you only need to use get() now.

done.

> The (s_end - tokenLength) here is wrong. The length of the token doesn't have 
any relevance here. You are looking for only 3 non-null bytes e.g. "1]:" So you 
should do the same as the old code and compare to s2 < (s_end - 2). (2 because 
s_end points at the null byte).

done.
Attachment #151606 - Flags: superreview?(bienvenu)
Attachment #151606 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 151606 [details] [diff] [review]
Patch v5b

>+#include "nsIPrefBranch.h"
>+#include "nsIPrefService.h"
>+#include "nsIServiceManager.h"
This file has changed since you started work on this, so we already include
these files these days :-)

>+  nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
Strangely enough revision 1.88 checked in by bienvenu has the more correct
longhand way of getting a branch :-P

>+  if (NS_SUCCEEDED(rv))
I'm not sure which style is best here - r1.88 uses if (pref); while your old
patch had the belt-and-braces approach :-)

>+    rv = prefBranch->GetCharPref("mailnews.localizedRe", getter_Copies(localizedRe));
It occurs to me that you're not using this rv.

>+  PRInt32 tokenLength;
>+  const char *tokPtr = checkString.get();
>+  while (*tokPtr)
>+  {
>+    //tokenize the comma separated list
>+    for (tokenLength = 0; *tokPtr && *tokPtr != ','; tokenLength++, tokPtr++);
I don't like bienvenu's collapsed loop - I'd prefer
PRSize tokenLength = 0;
while (*tokPtr && *tokPtr != ',')
{
  tokenLength++;
  tokPtr++;
}
[Note: PRSize picked at random; PRUintn and PRUptrdiff are both alternate
candidates for your length, as I'm expecting it to be unsigned...]

>+    tokenLength = 0;
You don't need this.
Attachment #151606 - Attachment is obsolete: true
Attachment #151606 - Flags: superreview?(bienvenu)
Attachment #151606 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v5c (obsolete) — Splinter Review
>>+#include "nsIPrefBranch.h"
>>+#include "nsIPrefService.h"
>>+#include "nsIServiceManager.h"
>This file has changed since you started work on this, so we already include
these files these days :-)

Removed.

>>+  nsCOMPtr<nsIPrefBranch>
prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>Strangely enough revision 1.88 checked in by bienvenu has the more correct
longhand way of getting a branch :-P

>>+  if (NS_SUCCEEDED(rv))
>I'm not sure which style is best here - r1.88 uses if (pref); while your old
patch had the belt-and-braces approach :-)

I assume you don't want anything changed here.

>>+    rv = prefBranch->GetCharPref("mailnews.localizedRe",
getter_Copies(localizedRe));
>It occurs to me that you're not using this rv.

Removed.

>I don't like bienvenu's collapsed loop - I'd prefer

Changed.

>>+    tokenLength = 0;
>You don't need this.

Removed.
Attachment #152548 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #152548 - Flags: review?(bienvenu)
Comment on attachment 152548 [details] [diff] [review]
Patch v5c

+    while (*tokPtr && *tokPtr != ',') {
+      tokenLength++; 
+      tokPtr++;

isn't there supposed to be a } after this? The idea was to compute the whole
token length and then see if "s" started with it...
Attachment #152548 - Attachment is obsolete: true
Attachment #152548 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #152548 - Flags: review?(bienvenu)
Attached patch Patch v5dSplinter Review
>isn't there supposed to be a } after this? The idea was to compute the whole
token length and then see if "s" started with it...

Hmm, yes. It worked and I didn't think about it...
Attachment #152550 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #152550 - Flags: review?(bienvenu)
Attachment #152550 - Flags: review?(bienvenu) → review+
Comment on attachment 152550 [details] [diff] [review]
Patch v5d

There are ways and means of getting prefs, it might yet get standardized at
some point ;-)
Attachment #152550 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #152550 - Flags: approval1.8a2?
Comment on attachment 152550 [details] [diff] [review]
Patch v5d

a=asa (on behalf of drivers) for checkin to 1.8a2
Attachment #152550 - Flags: approval1.8a2? → approval1.8a2+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
we want this for thunderbird 1.0...I'll make sure it gets in.
when I check this into the trunk, I made sure I fixed bug 252712 before checking
in...
Keywords: fixed-aviary1.0
Product: MailNews → Core
Blocks: 271733
*** Bug 324628 has been marked as a duplicate of this bug. ***
*** Bug 324628 has been marked as a duplicate of this bug. ***
since the bug i replied to was marked a dupe of this:

some mails i receive have "Antwort:" instead of "Re:" could this be added to the patch?
using the config editor, you need to set "mailnews.localizedRe" to include "Antwort:"
(In reply to comment #44)
> using the config editor, you need to set "mailnews.localizedRe" to include
> "Antwort:"
> 

that doesnt work.

ive set mailnews.localizedRe (it wasnt in the config editor, so i created it) to "Antwort:" and "Antwort", but all Replies i send to those mails have "Re: Antwort:"

(using Thunderbird 1.5)
you need to delete the .msf file for the folder containing the messages you're replying to, unless the messages arrived after you set that pref...
*** Bug 29179 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 415655
Product: Core → MailNews Core
Duplicate of this bug: 108163
No longer blocks: 319037
Duplicate of this bug: 810778
You need to log in before you can comment on or make changes to this bug.