194788 - Allow choice of Fw:Subject instead of [Fwd:Subject] for forwarded messages

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement, P2)

Description

[Jon Roland]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3b) Gecko/20030221
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3b) Gecko/20030221

Some of us would prefer to have the format of the subject line of a forwarded
message be changed to Fw:Subject instead of [Fwd:Subject]. I suggest the user be
given options for the format of changes to subject lines for forwarded messages,
or even enable him to design his own.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Jon Roland]

I should add that the user should be given the choice of only having one Fw: at
the beginning of the subject line, just as there ordinarily will be only one
Re:, so that subject lines don't become long strings of Fw: Fw: Fw: ... as a
result of successive forwards. That means the function should enable the user to
strip unnecessary Fw: (and perhaps Re: as well) from subject lines.

Comment 2

[Jon Roland]

Any further action on this? It is still a priority for me, as it is a constant
chore.

Comment 3

[Olivier Cahagne]

agree with this, [Fwd ... ] is ugly and adds no information. I much prefer Fw,
Fwd, etc.
No dupes found, marking NEW.

Comment 4

[Matthew Hodgson]

Attachment: Make Fwd format internationalisable

This patch makes the format of the Fwd: subject line customisable as the
mailnews.fwd_subject_format preference.  The preference is a string containing
a %s, which is substituted for the subject header of the original mail (default
is [Fwd %s]).  There is a potential unicode/charset corruption due to using
NS_LossyConvertUTF16toASCII() in the forward-inline part of the code, but I
can't immediately see a way around this without fixing up all of the i18n in
the old mimedrft.cpp code.

See also bug 168381. (Whilst I am deliberately not touching the Re: prefix for
replying, the default [Fwd: %s] prefix for forwarding is not standardised and
breaks threading on MUAs which understand the Fw: prefix.  My reason for adding
this config option was to allow me to use Fw: %s instead.)

Comment 5

[Florian Effenberger]

Any chance of getting this into an official TB release?

Comment 6

[Vidar Haarr (not reading bugmail)]

Matthew: Great work! I was just about to do a fix for bug 168381 but luckily it
has a reference to this bug :-)

One nit though; think you can fix up
<http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/toolbar-icon.xul#34>
while you're at it?

Comment 7

[Magnus Melin [:mkmelin]]

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

Comment 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[Jiri TRAVNICEK [:JITR]]

(In reply to comment #5)
> Any chance of getting this into an official TB release?

I, too, hope it will. It certainly *is* an useful feature as this thing badly needed to be customizable. The only other solutions I could find in MozillaZine forums involved patching the binary executable which is nasty. Thanx for the patch!

Comment 10

[Jiri TRAVNICEK [:JITR]]

Btw, is there any reationale on the current forward subject choice? I mean, there's probably some good reason for using the brackets I'm missing. Could someone knowledgeable enlighten this mystery? (Can't recall seeing this form of subject anywhere else...)

Comment 11

[A.P. Veening]

(In reply to comment #9)
> (In reply to comment #5)
> > Any chance of getting this into an official TB release?
> 
> I, too, hope it will. It certainly *is* an useful feature as this thing badly
> needed to be customizable. The only other solutions I could find in MozillaZine
> forums involved patching the binary executable which is nasty. Thanx for the
> patch!
> 
As the author of that small patching program, even I must admit it is a nasty solution, but I think it is still a lot better than patching by hand using a hex editor.

For those interested in the patch program, I put up a new version of [url=http://apveening.speedlinq.nl/software/edttbfwd.zip][b][u]Edit Thunderbird Forward[/u][/b][/url] today which pathes the executable for both in-line and as attachment forwarding. Unfortunately, at this moment it is still Windows only.

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to comment #10)
> Btw, is there any reationale on the current forward subject choice? 

I'd say let's skip the brackets... it's just ugly imo. 

Outlook: FW: subj
hotmail.com: FW: subj

Eudora: Fwd: subj
Opera: Fwd: subj
gmail.com: Fwd: subj

? Lotus notes: Fw: subj 
Evolution: [Fwd: subj]

Comment 15

[Jiri TRAVNICEK [:JITR]]

(In reply to comment #6)

> {...}
> One nit though; think you can fix up
> <http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/toolbar-icon.xul#34>
> while you're at it?

Well, aparently the code was not changed yet and the `[Fwd:' & `]' are still hard-coded there (line 56, within `openComposeWindowForRSSArticle()'). Has anyone happened to work on that? I might try to make a patch, but don't promise anything. I'd certainly spare myself reinventing the weel, though. ;-)

From my experience, this code has nothing to do with the e-mail composition. On the other hand, I don't have an idea where exactly it is being used and what functionality to get it executed. Suggestions anyone?


(In reply to comment #12)

Thanx for the interesting research. :-)

> {...}
> Evolution: [Fwd: subj]

So, perhaps this could be the reason behind that strange choice?

Comment 16

[A.P. Veening]

My patching program is still available at http://apveening.speedlinq.nl/software

Unfortunately, it is Windows only. However, the source is included so it can be addapted for other Operating Systems.

Besides that, I included a link to an extension that will do the same thing.

Comment 17

[Jiri TRAVNICEK [:JITR]]

Just read some rationalization in bug #168381, esp. those about localizability of the `Re:' string. From this point of view it seems the current patch might be too benevolent. To fullfill the purpose of this bug, a boolean preference which would toggle the addition of `[' and `]' should be just enough. (This way, we would get rid of the localization stuff. And, of course, functionality, too.)

On the other hand, the current implementation also allows to choose between `Fw:' and `Fwd:' which could be useful (see commet #4). Still, no other variants should probably be allowd.

We should probably apply similar approach as for the `Re:' prefix, but the bug #168381 has not reached any conslusion yet. It should be, however, taken into account the `Fw:' vs. `Fwd:' are less "standardized" than the `Re:' prefix.

Well, just my bit. What do the others think? Should the patch be kept in its current form or is simplification desired?


(In reply to comment #16)

> My patching program is still available at {...}
> 
> Unfortunately, it is Windows only. {...}

I guess Linux users will have less problems doing this by hand than Win ones.

> Besides that, I included a link to an extension that will do the same thing.

I have accidentaly found it yesterday (thought no such extension exists until then) and I'm seriusly considering this alternative.

Comment 18

[A.P. Veening]

(In reply to comment #17)
> Just read some rationalization in bug #168381, esp. those about localizability
> of the `Re:' string. From this point of view it seems the current patch might
> be too benevolent. To fullfill the purpose of this bug, a boolean preference
> which would toggle the addition of `[' and `]' should be just enough. (This
> way, we would get rid of the localization stuff. And, of course, functionality,
> too.)

That add-on works with a boolean (can be found in about:config when the add-on is installed).
 
> On the other hand, the current implementation also allows to choose between
> `Fw:' and `Fwd:' which could be useful (see commet #4). Still, no other
> variants should probably be allowd.
> 
> We should probably apply similar approach as for the `Re:' prefix, but the bug
> #168381 has not reached any conslusion yet. It should be, however, taken into
> account the `Fw:' vs. `Fwd:' are less "standardized" than the `Re:' prefix.
> 
> Well, just my bit. What do the others think? Should the patch be kept in its
> current form or is simplification desired?
> 
> 
> (In reply to comment #16)
> 
> > My patching program is still available at {...}
> > 
> > Unfortunately, it is Windows only. {...}
> 
> I guess Linux users will have less problems doing this by hand than Win ones.

I had no real problems doing it by hand either, but it became a bit repetitive and since I am a programmer ...
 
> > Besides that, I included a link to an extension that will do the same thing.
> 
> I have accidentaly found it yesterday (thought no such extension exists until
> then) and I'm seriusly considering this alternative.
> 
It has the double advantage of being platform independent and upgrade-proof.

Comment 19

[Jiri TRAVNICEK [:JITR]]

(In reply to comment #4)

> Created an attachment (id=192708) [details]
> Make Fwd format internationalisable
> 
> {...}

Matthew, just an innocent question: Have you tested the patch thoroughly? I'm having problems with it when forwarding a message as an *attachment* -- only the first character of the original subject gets substituted in place of the `%s'. I even tested with the trunk code and the result is the same. (I'm interating into the 2.0.0.9 version otherwise.)

The following works for me (the following is a patch to your patch):

--- fwd_subject_format.patch.orig	2008-01-31 02:04:12.131825800 +0100
+++ fwd_subject_format.patch	2008-01-31 02:04:49.765941000 +0100
@@ -65,7 +65,7 @@
 -              subject.Insert(NS_LITERAL_STRING("[Fwd: ").get(), 0);
 -              subject.Append(NS_LITERAL_STRING("]").get());
 +              PRUnichar *formattedString = nsnull;
-+              formattedString = nsTextFormatter::smprintf(fwdSubjectFormat.get(), subject.get());
++              formattedString = nsTextFormatter::smprintf(fwdSubjectFormat.get(), NS_ConvertUTF16toUTF8(subject).get());
 +              if (formattedString)
 +              {
 +                subject.Assign(formattedString);

Sorry if it comes through mangled, I didn't want to add it as an attachment.


(In reply to comment #6)

> {...}
> 
> One nit though; think you can fix up
> <http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/toolbar-icon.xul#34>
> while you're at it?

I've already created a new version of the patch, which fixes the above described problem as well as fullfilling this wish. I plan to release it in a few days.

Since there were almost no comments on the subject of the "desired extent of localizability", the patch will still support full string customization as Matthew's original patch does. Please, raise any possible objections and/or suggestions now. :-) If none appear, I'll post the version as just described.

Comment 20

[Onno Ekker [:nONoNonO UTC+1]]

I think localization of "Fwd:" is just as bad an idea as localization of "Re:", although "Fwd:" is less standard. You see all kind of variant, like Thunderbirds "[Fwd: ...]" and "(fwd) ...", etc.

If people want localized versions, it would be a better idea to make an extension for it that would parse the subject line and would present localized versions of "Re:" and "Fwd:", just as localized versions of header lines like "From:", "To:" and "Date:" are presented to the user, if you switch Thunderbird locale.

With dutch locale, I see "Van:", "Aan:" and "Datum:", although the message contains the original headers.

Comment 21

[Magnus Melin [:mkmelin]]

Well, I think we shouldn't localize fwd by default (in localized builds) but it should be an option. Re: is from latin I read somewhere, Fwd is pure english. 

I'd like to know if anyone really want to have the braces, which are my main gripe with the format. (Which would kind of be a pre req for bug 239257 also.)

Comment 22

[rsx11m]

(In reply to comment #20)
> With dutch locale, I see "Van:", "Aan:" and "Datum:", although the message
> contains the original headers.

Keep in mind that the "From:", "To:", and "Date:" are standardized in RFC822, thus the only place where those can be localized is the user interface. This is different for the forwarding and reply cases, even though "Re:" is commonly used and relied on by many clients.

(In reply to comment #21)
> I'd like to know if anyone really want to have the braces, which are my main
> gripe with the format. (Which would kind of be a pre req for bug 239257 also.)

The mailnews.localizedRe preference provides a similar solution for the threading and formatting of replies, considering local variants of the reply prefix. The braces always have been sort of a "trademark" for Mozilla, but the closing ']' may certainly interfere with correct threading. The current format can be retained for the default while making the string configurable for those who don't like them. The fix for bug 239257 could ignore extra punctuation for the purpose of threading, making it more complex then though.

Comment 23

[Jiri TRAVNICEK [:JITR]]

(In reply to comment #22)

> Keep in mind that the "From:", "To:", and "Date:" are standardized in RFC822,
> thus the only place where those can be localized is the user interface. {...}

And (I believe) also the textual representation of original message's headers (as when forwarding in-line).


As for the right extent of localizability: When it comes to subject prefixes, I believe it is the Bad Thing (tm) to localize them. Even Very Vad Thing For the `Re: '. Yet I'm not sure if we need to go to the extreme of preventing this. We can't really stop anyone.

But, one thing is localization, the other is to allow for other modifications. As already stated by the others, the forwarded message subject prefixes are much less standardized, thus the users may want and/or need to change them. Also the default brakckets are IMHO pretty strange.

Well, since there's already a patch with this extra functionality, I decided to keep it for now. It can be removed later and the patch simplified, if desired. It is possible to only remove the localization support and it is also possible to change the patch to only support certain variants (ie. with and without the brackets, `Fwd' vs. `Fw' etc.).

Therefore, I'll release the patch with all the support included, and we'll see how the review goes.

Thanx to the others for voicing their opinions.

Comment 24

[Jiri TRAVNICEK [:JITR]]

Attachment: new version of the patch (against the trunk)

Here goes the promised patch. Created and tested against current trunk.

A sort of changelog (relative to Matthew's patch):

* Added support of subject customization for the RSS feed messages (before, only e-mail messages were customizable).

* Fixed missing UTF-16 -> UTF-8 conversion in the code handilng forwarding as attachment. Without the fix, forwarded message's subject would include only the first character of original message's subject.

* Changed the location of added lines within the preferences and properties files to be more logical (before, they were "breaking" in a  sequence of related lines).


That is, except the last one (which is rather a cosmetical change), what was already promised -- comment #19 and #23.

Implementation note concerning the RSS code: As far as I know there's nothing like `printf()' available in the JavaScript. Thus, I created a little emulation instead, which can handle `%s' and `%%' sequences. These are the only ones which can be correctly accepted by the C++ code handling the subject for ordinary e-mails. (Theoretically, `printf()' should also correctly accept something like `%1$s' but I don't think anyone will use it and I didn't feel like implementing it, too.) So, for any reasonable format string the code should behave the same as Matthew's C++ code does. The only differences being it won't crash when too many `%s' are specified (it will substitute for each occurence, not the first one only). It will also ignore other `%<letter>' specifiers (they'll be kept intact).

Comment 25

[Jiri TRAVNICEK [:JITR]]

Attachment: new version of the patch (against Thunderbird ver. 2.0.0.9)

Should someone want this for the current stable version, I also provide the same patch created against Thunderbird ver. 2.0.0.9 which is what I'll be dogfooding. :-)

Comment 26

[James Carroll]

The following extension seems to solve this problem in a general way.  Please let me know if I am mistaken.

Clean Subject 0.3.1

https://addons.mozilla.org/en-US/thunderbird/addon/7844

Cheers,
James

Comment 27

[A.P. Veening]

(In reply to comment #26)
> The following extension seems to solve this problem in a general way.  Please
> let me know if I am mistaken.
> 
> Clean Subject 0.3.1
> 
> https://addons.mozilla.org/en-US/thunderbird/addon/7844
> 
> Cheers,
> James
> 
That extension does not mention removing the brackets. For the rest it looks like you are not mistaken. However, I haven't tried it myself. I suggest you do so and tell us the results.

Comment 28

[Jon Roland]

The download of Clean Subject would not install on my current install of TB.

Extensions are not a satisfactory solution as they don't always keep up with new versions of the base product.

Comment 29

[Wayne Mery (:wsmwk)]

cc: Bryan regarding UI

Jiri, if this gets UI approval you will want a patch for trunk.
Canceling the reviews - mscott is no actively reviewing.

Comment 30

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #29)
> cc: Bryan regarding UI
> 
> Jiri, if this gets UI approval you will want a patch for trunk.
> Canceling the reviews - mscott is no actively reviewing.

You forgot to cc Bryan, so I did for you. :) Nominating wanted-thunderbird3 since there's a sort-of patch available.

Comment 31

[Bryan Clark (DevTools PM) [:clarkbw]]

This seems really valuable, but I think it's going to be best suited as an extension.  

If we can get the necessary changes into the add-ons manager we can help people get this extension.  I'm not sure what the add-ons manager bug is right now, I'll try to look that up.

See Get Compose Add-ons Design
http://clarkbw.net/designs/inline-get-add-ons/account-settings-add-ons.png

Comment 32

[Magnus Melin [:mkmelin]]

Bryan: any thoughts on comment #12?

Comment 33

[Bryan Clark (DevTools PM) [:clarkbw]]

> Bryan: any thoughts on comment #12?

Oh sure, if this bug was just about removing the current brackets I'd approve that.  I'm totally with you on comment 12.  

Making this configurable is great, but as I mentioned I'd like to leave that to the add-on space to allow better customization than would be really possible in the default install.

Comment 34

[Magnus Melin [:mkmelin]]

(In reply to comment #31)
> This seems really valuable, but I think it's going to be best suited as an
> extension.  

Yeah I never meant it should have UI, so we should do something along what the bitrotted patch does, and leave possible UI to extensions.

Comment 35

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This patch loses the braces around the subject and adds a pref for the forwarding subject prefix. I didn't make it localizable - as I don't think localizers should change it normally.

The rest is just making the code less blocky. On compose we need the prefs a few times, if we can't get the pref service I assume we're better off just returning error anyway.

Will attach a diff -uw shortly.

Comment 36

[Magnus Melin [:mkmelin]]

Attachment: proposed fix (diff -uw)

Comment 37

[David :Bienvenu]

Comment on attachment 362745 [details] [diff] [review]
proposed fix

thx, Magnus.

Comment 39

[Ludovic Hirlimann [:Usul]]

As this been check-in ?

Comment 41

[Magnus Melin [:mkmelin]]

changeset:   2322:e825dcc496c8
http://hg.mozilla.org/comm-central/rev/e825dcc496c8

->FIXED

Comment 42

[[e-mail]]

Comment on attachment 362745 [details] [diff] [review]
proposed fix

>-              subject.Insert(NS_LITERAL_STRING("[Fwd: ").get(), 0);
>-              subject.Append(NS_LITERAL_STRING("]").get());
You copied some wacky string munging - the .get()s are unnecessary.

Comment 43

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, followup to comment 42

Remove the unnecessary get()s

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 370888 [details] [diff] [review]
proposed fix, followup to comment 42

changeset:   2339:4f871c9ec5f8
http://hg.mozilla.org/comm-central/rev/4f871c9ec5f8