213004 - Localized "Re:" strings are not removed when replying (implements `mailnews.localizedRe`)

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Matthias Bockelkamp]

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.

Comment 1

[Matthias Bockelkamp]

Attachment: patch

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.

Comment 2

[R.K.Aa.]

Related/dup of bug 29179 / bug 108163

Comment 3

[Jo Hermans]

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 4

[[e-mail]]

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.

Comment 5

[Matthias Bockelkamp]

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$[e-mail]> and netscape.public.mozilla.prefs
<bfdqcn$dg9tt$[e-mail]>, 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.

Comment 6

[Matthias Bockelkamp]

Attachment: patch v2

Please see Comment #5, first paragraph.

Comment 7

[Matthias Bockelkamp]

Comment on attachment 130192 [details] [diff] [review]
patch v2

Btw. does anyone know a better name for the pref?

Comment 8

[[e-mail]]

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.

Comment 9

[Matthias Bockelkamp]

Attachment: patch v3

The strdup() is necessary because strtok() replaces the delimiters with 0s.

Comment 10

[Scott MacGregor]

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. 

Comment 11

[Matthias Bockelkamp]

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 12

[[e-mail]]

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.

Comment 13

[Matthias Bockelkamp]

Attachment: patch v4

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.

Comment 14

[[e-mail]]

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 15

[[e-mail]]

Comment on attachment 131228 [details] [diff] [review]
patch v4

>+  PRInt32 tokenLength = 0;
I think the declaration belongs here:
>+    tokenLength = strlen(lrToken);

Comment 16

[Matthias Bockelkamp]

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.

Comment 17

[Matthias Bockelkamp]

Attachment: patch v4.1

Comment 18

[[e-mail]]

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

Comment 19

[Matthias Bockelkamp]

Attachment: 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.

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.

Comment 20

[[e-mail]]

(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 21

[David :Bienvenu]

Comment on attachment 131248 [details] [diff] [review]
patch v4.2

I'll sr this...

Comment 22

[David :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...

Comment 23

[David :Bienvenu]

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.

Comment 24

[Matthias Bockelkamp]

Attachment: Patch v5

Comment 25

[David :Bienvenu]

  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...

Comment 26

[Brodie]

+  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? 

Comment 27

[Matthias Bockelkamp]

Attachment: Patch v5a

Comment 28

[Matthias Bockelkamp]

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.

Comment 29

[Brodie]

+  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).

Comment 30

[Matthias Bockelkamp]

Attachment: Patch v5b

Comment 31

[Matthias Bockelkamp]

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.

Comment 32

[[e-mail]]

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.

Comment 33

[Matthias Bockelkamp]

Attachment: Patch v5c

>>+#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.

Comment 34

[David :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...

Comment 35

[Matthias Bockelkamp]

Attachment: Patch v5d

>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...

Comment 36

[[e-mail]]

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 ;-)

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 152550 [details] [diff] [review]
Patch v5d

a=asa (on behalf of drivers) for checkin to 1.8a2

Comment 38

[[e-mail]]

Fix checked in.

Comment 39

[David :Bienvenu]

we want this for thunderbird 1.0...I'll make sure it gets in.

Comment 40

[David :Bienvenu]

when I check this into the trunk, I made sure I fixed bug 252712 before checking
in...

Comment 41

[Karsten Düsterloh]

*** Bug 324628 has been marked as a duplicate of this bug. ***

Comment 42

[David :Bienvenu]

*** Bug 324628 has been marked as a duplicate of this bug. ***

Comment 43

[u49640]

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?

Comment 44

[David :Bienvenu]

using the config editor, you need to set "mailnews.localizedRe" to include "Antwort:"

Comment 45

[u49640]

(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)

Comment 46

[David :Bienvenu]

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...

Comment 47

[Angelo Dipierro]

See http://kb.mozillazine.org/Reply_indicators 

Comment 48

[David :Bienvenu]

*** Bug 29179 has been marked as a duplicate of this bug. ***