273114 - When forwarding messages signature line is placed after forwarded message (should obey reply signature position pref)

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[rtoliver.80919]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041122
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041122

When you forward messages, the signature line is added at the bottom of all
messages forwarded. I would like to be able to configure the mail editor so the
signature line appears before the messages being forwarded and after the new
text entered. In other words, I would like the placement of the signature line
in forwarding messages to behave identically to the manner in which it behaves
when you reply to a message. 

Reproducible: Always
Steps to Reproduce:
1. Configure system to insert a signature line
2. Forward a message
3.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Not a preferences dialog issue..

Comment 2

[Mike Cowperthwaite]

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

Comment 3

[Matthew Hodgson]

Attachment: Honour the place-signature-above-reply setting when forwarding mails.

This is the same as #214514, which was perhaps incorrectly flagged as a dup of
the #62429 uberbug.  It's a quirk that i've constantly had complaints about,
and attached is a simple patch to compose/src/nsMsgCompose.cpp which fixes it.

Hopefully replacing the htmlEditor->RebuildDocumentFromSource(aBuf) with
htmlEditor->insertHTML(aBuf) doesn't clobber any edge cases I'm unaware of...

Comment 4

[Mike Cowperthwaite]

Removing "DUPEME" -- I believe Matthew has ID'd the potential dupe.

Comment 5

[Mike Cowperthwaite]

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

Comment 6

[Mike Cowperthwaite]

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

Comment 7

[Josh Skinner]

(In reply to comment #3)
> attached is a simple patch to compose/src/nsMsgCompose.cpp which fixes it.

Sorry, I am new at this. I see your patch from August, but that means I have to re-compile the source myself right or is there a binary available already somewhere else? Why was your patch not included in the more recent release of Thunderbird? Thanks Mike/Matthew.

Comment 8

[Josh Birnbaum]

The patch is flagged for review, but currently is still waiting in mscott's queue.  Depending on what areas of code a patch affects, and the status of the product, patches may need a review, super-review, and approval marking before being checked into the code.  Once checked in a fixed binary will be available as a trunk nightly; it may be a while before a officially released version includes the fix.  Of course, this process moves quicker for security and crash bugs versus enhancements.

Comment 9

[Josh Skinner]

Do anyone know of an existing Extension that would work in the interim?

Comment 10

[Matthew Hodgson]

nope, that's why I wrote the patch :(  Your only chance for now is to build your own custom build of Thunderbird (which takes a few hours of setup, and a few hours of compiling, assuming nothing goes awry).

Comment 11

[Magnus Melin [:mkmelin]]

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

Comment 12

[Dick]

This is to basic, please make it a part of Thunderbird 2.0 !

I like Thunderbird very much, but this is for me the most annoying "bug" of Thunderbird. 
Especially if the messages are longer I always have to go to the bottom and cut & past the signature to the place under my text and above the old messages as would happen with "reply".

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: unbitrotted patch

I was trying this out. The patch had bitrotted so here should be a version applying to trunk. 

I notice the "-- " delimiter is still there when putting the signature on top. (It shouldn't.)

But then again, I'm not sure this is functionality I would like. Maybe it should be a pref of it's own, not connected to replies. (Or no sig at all for fwd, bug 167319)

Comment 14

[Mike Cowperthwaite]

Does this patch get applied if body text is passed in from an external source?
Such as Send Link from Firefox, or a mailto: with a body argument?  I'm just not sure what situations lead to 'aBuf' containing content.

I like the idea of not adding sig at all for (inline) forward, but if that were made the only possibility, there's sure to be a load of complaints.

Comment 15

[Magnus Melin [:mkmelin]]

Yeah... seems like the htmlEditor->InsertHTML(aBuf); line should go. (Didn't test though.)

Comment 16

[Jeffrey J. Atto]

Hi, 
Is there any other way around this 'signature below rather than above' issue outside of recompiling?  (I have no idea how to recompile).

I'm an end user that cuts and pastes his signature from below to above during many forwarded e-mails to clients.  This would be an incredibly logical and efficient feature/bug fix.

Any information on the status of this would be greatly appreciated.  Thank you very much for your time and efforts.

Comment 17

[Josh Skinner]

I cannot believe that my first post about this issue (in the forums) was in 2005 and yet it still has not been resolved. I even posted to this bugzilla report 1.5 years ago (see above) and the patched .cpp fix still hasn't made it to the releases. Other than the endless/useless "top-posting vs bottom-posting" debate, what is the hold up for making this an OPTION or at least someone creating an extension for it??? 

On that note, knowing nothing about creating extensions (but having done some programming in university), if I were to try to make one myself is it possible, or would it be overly difficult to do so for this issue? Thanks.

Comment 18

[peter]

Has anyone compiled there own release of 2.0 with this fix. If you have can you email me?

Comment 19

[e.fabene]

Any news? I've hordes of users aiming at my throat for this. A few bosses already went back to Outlook :(

Comment 20

[peter]

check this out 

http://forums.mozillazine.org/viewtopic.php?t=512394&start=30

Comment 21

[Josh Skinner]

That doesn't address this problem. Signature still appears at the bottom and I don't have the Signature Switch addon. It's really just a matter of the Tbird developers not wanting to include something they feel is a microsoft violation of "traditional practices" (notice I didn't say standard - there's no actual RFC for this). Or, simply put, pride. Heck, I can't even get someone to help me with an extension for it. The lesson here is that Open-Source does not necessarily mean Open to Ideas. But that debate is for the forums - just wait it out, they'll come around.

Comment 22

[peter]

tested with 2.0.0.5 But the problem is still there

Comment 23

[Jeffrey J. Atto]

Hi,
I was wondering if there was any way that someone like myself (who is quite computer literate, but not a programmer) could fix this signature problem without spending hours compiling and recompiling?

Does anybody know if and or when this problem might be resolved?

Thank you for your time.

Comment 24

[rtoliver.80919]

I initially reported this problem in December of 2004.  I just installed Tbird version 2.0.0.6 and the problem is still there.  Is there any hope of getting this annoying problem fixed before we hit the third anniversary of my reporting the problem?

Comment 25

[Robinmozilla]

I have just upgraded to 2.0.0.6 (20070728)and hoped that the signature placement problem was fixed.  It takes up an enormous amount of my time cut and pasting my signature " below my reply (above the quote)".  And often it doesn't want to paste it.  This is my only complaint/irritation with Thunderbird.  Please will you fix it asap.  I have fixed signature placement on replies.  If anyone can help me with a fix for forwards, I would really appreciate it.  Thank you, Robin

Comment 26

[Jeffrey J. Atto]

I wholeheartedly agree.  I absolutely love Mozilla Thunderbird and have been using it for years.  This "cut-and-paste" of the e-mail signature when forwarding, which, as Robin mentioned, usually doesn't work, is a real annoyance.

I wish the problem could be resolved.  And I know that this flies in the face of open-source-ness, but I offer to donate to the person who gets this solved.  Just name your price.  I'll donate half to the Mozilla Project and the other half to the person or persons who gets this solved.

At times like these, I wish I were a programmer.

Comment 27

[Willie]

(In reply to comment #26)
> I wish the problem could be resolved.  And I know that this flies in the face
> of open-source-ness, but I offer to donate to the person who gets this solved. 
> Just name your price.  I'll donate half to the Mozilla Project and the other
> half to the person or persons who gets this solved.

Actually, it doesn't fly in the face of open source at all!  Programmers donate their time (time=money) to projects to create something they would like.  You (not being a programmer) donate your money to (in the end) create something that you would like.
In the end, everyone benefits.

I remember seeing some website where you could put what you wanted coded and how much you were willing to pledge (other people could jump on the bandwagon to add to the amount) and then a coder would take the project and get paid when it was complete.  I'm sorry I don't remember where it was exactly -- perhaps someone could help me out there?

Comment 28

[Benoît E. Donnette]

This feature is dearly wanted here too ! I am refreshing the patch for the current trunk (almost trivial in fact, I am spending most of the time compiling for both Linux and Windows), an I should be publishing the patch shortly to keep it fresh, but it seems a good thing to push this bug as it is a user-desired feature.

Comment 29

[Jeffrey J. Atto]

I would like to reiterate that I will pay for this signature problem to be resolved as it will save me much time and unnecessary annoyance.

What can we do to move this process along?

Comment 30

[Benoît E. Donnette]

I will refresh the patch (done), test it (underway) and then publish it (ASAP) and this should refresh the attention on this bug.

The French Ministry of Finance is expecting integration of this bug too. Hopefully this makes 2 big sets of users waiting for a good end on this bug, you may end up not needing to pay for it.

Comment 31

[Magnus Melin [:mkmelin]]

I think I'd really prefer it to be a pref of it's own, not connected to replies. Which should also handle the no at all for fwd, bug 167319.

Comment 33

[Benoît E. Donnette]

Attachment: Patch refreshed for release THUNDERBIRD_2_0_0_6_RELEASE

As stated, a freshly remade patch for release 2_0_0_6.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 284285 [details] [diff] [review]
Patch refreshed for release THUNDERBIRD_2_0_0_6_RELEASE

First, this is the same patch that is already attached to this bug. It also have at least two flaws (see comment 13 to 15). 

When you have a patch ready for review, set the r? flag of the attachment to the e-mail of a suitable reviewer to ask review. r+ says the patch was reviewed and ok. You also need superreview.

http://www.mozilla.org/hacking/life-cycle.html

As this isn't something that would go into the tb2 security releases, please make the patch against CVS trunk.

Comment 35

[Benoît E. Donnette]

Attachment: Addresses issur from comment 13, but does not permit separate setting yet.

Better patch, that addresses the delimiter issue, but it does not solve the separate setting wish (comment 15).

Comment 36

[Benoît E. Donnette]

Well, I had not quite understood that the "---" had to be removed (I should not read too quickly). Sorry.

Comment 38

[kamau mposi]

I am using 2.0.0.9 and the problem persists.

Comment 39

[peter]

how do i install the patch?

Comment 41

[rtoliver.80919]

I reported this problem in Dec, 2004.  Over three years ago.  

Is there any projection for a date when a normal user like me can install an update and have the problem resolved?  I am not ready to compile a a build using a patch.  

Thanks

Comment 43

[Benoît E. Donnette]

Well, I am checking a new version of the attached patch. It improves the original patch in the way it behaves well on the "-- " point that was announced as blocking. Still, no separate setting yet but I am ready to discuss this point. And to end the bug accordingly.

Comment 44

[Benoît E. Donnette]

Attachment: Based on 2.0.0.9, I am porting to trunc.

Comment 45

[Benoît E. Donnette]

Attachment: Same as 301844, just a side case (no sig) concreted.

Comment 46

[Benoît E. Donnette]

Attachment: Same as above, ported to trunc.

Comment 48

[Magnus Melin [:mkmelin]]

Comment on attachment 301851 [details] [diff] [review]
Same as above, ported to trunc.

This doesn't compile, and I don't see how it would work...

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 301849 [details] [diff] [review]
Same as 301844, just a side case (no sig) concreted.

This patch does not apply, i'm not sure to what version it was supposed to apply to either, but there is no need to post patches for anything but current cvs trunk before you have a patch that has been reviewed and checked in there.

Comment 50

[Magnus Melin [:mkmelin]]

I really think bug 167319 would be the way to go though, combined with the option to choose before or after the quoted message. The setting should be independent and not tied to replies. I know I never forward stuff to people who I don't know (and thus have no use of my sig).

Comment 51

[Benoît E. Donnette]

Well, as for the version aginst which it applies :
--- mailnews/compose/src/nsMsgCompose.cpp	28 Jan 2008 16:51:42 -0000	1.546
+++ mailnews/compose/src/nsMsgCompose.cpp	7 Feb 2008 09:10:00 -0000

...not too old, is it ?

Anyway, you're fundamentally right - forward is a pretty special case.
I suggest one thing : just tell me on which version you'd be reading and let me provide the patch for you. The behaviour is pretty simple, I just add the boolean addDashes according to what I want, plus the original ported patch.

Comment 52

[Magnus Melin [:mkmelin]]

Didn't check what the problem was, but at least one HUNK failed when trying to applying one of the patches, and the other applied but didn't compile due to the docshell stuff.

Latest cvs version please:)
I only officially review /mail code, try one of the Mail and News Backend peers, http://www.mozilla.org/owners.html for a real review.

Comment 53

[Benoît E. Donnette]

I am perfectly ready to do as much as possible, but you will realize that I am no more willing to port the patch on many versions, as well as you are not willing to patch it yourself. What we've learnt from this is that the target file, nsMsgCompose.cpp, seems to be under heavy patching. What I can do is to port the patch to today's trunc, then warn me as you read it so I make sure the patch is up to date and update one, obsolete the other at will. CVS provides some help with patching but it is sometimes just not enough, and we both don't want such an unpleasant surprise to happen again, so go for it ?
I make sure the patch gets up to date for today, 16.00 CET, and I ask you to warn me if you want to read it, so I check no update breaks the patched file, OK ?
Thanks in advance, and feel free to use my personal mail if appropriate.

Comment 54

[Benoît E. Donnette]

Well, the relevant file has been committed thrice since the patch, hence the problem... I make the patch up to date.

Comment 55

[Benoît E. Donnette]

Attachment: Same patch, ported to take last changes into account

ported to trunc this morning. One of the last 3 commits broke one of the chunk, the patch has been hand-remade.

Comment 56

[Benoît E. Donnette]

As of today, 16:33 CET, still no further change on nsMsgCompose.cpp. The patch has been build against 1.549, whoever will review may simply contact me if it needs an update.

Comment 57

[Magnus Melin [:mkmelin]]

Comment on attachment 304187 [details] [diff] [review]
Same patch, ported to take last changes into account

Yes, this seems to work, thx!
As I said, I only review /mail reviews, so try someone else on the list. bienvenu perhaps?

Comment 58

[Benoît E. Donnette]

Comment on attachment 304187 [details] [diff] [review]
Same patch, ported to take last changes into account

Would you mind reviewing this patch, or point me towards a reviewer ?

Comment 59

[peter]

I would test it but i have no idea how to install that nsMsgCompose.cpp

Comment 60

[peter]

i found the instructions. 
Im going to try and compile the newest 2.0b2 source for windows with this patch. I will host the file if i get it compiled.  If anyone compiles it before me, just email me the compiled files. 

Comment 62

[Worcester12345]

I'm not sure what this "patch" does, but I am hoping it adds another line to the configuration where instead of "Automatically quote the original message when replying", it says "Automatically quote the original message when forwarding" and has the same settings for this function.

Thanks.

Comment 63

[Worcester12345]

And shouldn't the "Version" here be "2.0" or "branch" instead of or in addition to "Trunk"? This is a low profile, high gain bug, and should have the following keywords: "mail6:Low risk, high gain Mail bugs. Mail Bugs which require little engineering effort but have high user benefit that might not otherwise be covered under the other keywords." "ue:For bugs with a specific detailed suggestion for improvement of the user experience. The problem describes a confusing or unhelpful aspect of the user interface."

Comment 64

[Magnus Melin [:mkmelin]]

(In reply to comment #62)
Doesn't change quoting, just puts the signature in the above the original message if reply-settings is "above".

There is no point playing with keywords, the mailX ones I understand were used - by developers - way back in the netscape era for prioritizing, especially as we already have a patch. Nowadays they really shouldn't be used imo, since it doesn't make sense that anyone can add them (at least to their own bugs).

Comment 65

[Benoît E. Donnette]

No, Worcester12345, I did add no configuration element, all I did was keeping in sync with the use : ie signature delimiter only when the signature is at  the end of the messsage.

I am ready to discuss this point, but I do beleive that adding yet another configuration element might be too much.

Comment 66

[Michael Borvang]

There is no need for a new configuration element, please fix this bug fast, it would be nice to have it in TB 2.0013 release, thanks !!!

I think it is more importent to make it work then long talks.

A new configuration element can wait for TB 3, this is a bug there gives a lot of problems so make is work with the setting for replaying messages and thats it.

Comment 67

[Benoît E. Donnette]

Michael, IMHO it is *actually fixed* by the patch enclosed first, which I just improved in the main sense that was asked for by the replies here (the remark #13 has been taken into account, Magnus could check, see #57).

What leavs to be done is the configuration element, but I do not yet know how to add one, this could take some time for me. Either include it temporarily with reserve or have it done if you want it for 2.0013, but I can't guarantee I'll meet the deadline !

Comment 68

[Michael Borvang]

Benoit, I understand your problem and I hope some one can help you with that, but this is a bug that never should have been. In all e-mail clients this is stand, so that way I think this need to be done fast.

Is there some of det other developers there can help to get this done ?

If this is not the case, can your fix use the settings form the configaration element for replaying messages ? If so I think that the way to go.

Comment 69

[Benoît E. Donnette]

It is what it already does, and what the initial patch has been doing for ages (almost 3 years...). What I did was simply the "-- " handling trick, ie no signature delimiter when the signature is placed before the reply.

As for a bug that should never have been, well, this behaviour goes simply against RFC1855... However I understand and find top-quoting quite convenient and productive myself. 

Comment 70

[Daniel Veditz [:dveditz]]

An enhancement request isn't going to "block" a release, but if the mail owners are happy with a small, safe, patch you can request approval on it. The fact that the mail owner hasn't reviewed the patch in over a month is not a great sign, however.

Comment 71

[Benoît E. Donnette]

Well, I haven't set the flag myself, but well, I wonder if I should flag it as FIXED so as to ring a bell to the supposed reviewer... Well, I begin with a proper assignment and see the rest.

Comment 72

[Benoît E. Donnette]

Well, who should set the bug "Fixed", the reviewer ? If yes, then please anyone to review ? I'd be ready to discuss the final "how" if there needs to be one, but some people are waiting this fix for so long...

Comment 73

[David :Bienvenu]

The patch looks basically reasonable.

+      if (sigOnTop && !aSignature.IsEmpty()) {
+        textEditor->InsertText(aSignature);
+        m_editor->EndOfDocument();
+      }

Could you fix the braces style to match the rest of the file, i.e.,

if (a)
{
}

Here,

   /* Some time we want to add a signature and sometime we wont. Let's figure that now...*/
   PRBool addSignature;
+  PRBool addDashes;

It seems like it would be cleaner to initialize addDashes to PR_FALSE so you don't need to set it to FALSE in three different places. Or,

PRBool addDashes = (mType == nsIMsgCompType::ForwardInline);

I haven't tested the patch; Does it apply, or do you need to update it to make it apply? In either case, if you could submit a new patch with my comments addressed, I can give it a try.

(You don't mark a bug fixed to bring attention to it; We mark it fixed when the patch is checked in)

Comment 74

[Benoît E. Donnette]

Attachment: Refreshed to take comments into account

bienvenu, this new patch takes both of your comments into account.

Comment 75

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

Moving from wanted‑thunderbird3.0a1? flag to wanted‑thunderbird3.0a2? flag since code for Thunderbird 3 Alpha 1 has been frozen.

Comment 76

[Magnus Melin [:mkmelin]]

Comment on attachment 317497 [details] [diff] [review]
Refreshed to take comments into account

I don't do mailnews backend reviews.

Comment 77

[Magnus Melin [:mkmelin]]

Plussing this for 3 since we have a patch, and it certainly is a common complaint.

Comment 78

[David :Bienvenu]

Comment on attachment 317497 [details] [diff] [review]
Refreshed to take comments into account

thx for the patch, Paul. Requesting review from Standard8. I've been running with this patch for a while and tested it somewhat, though I'm not a big sig or forward inline user.

Comment 79

[Mark Banner (:standard8)]

Comment on attachment 317497 [details] [diff] [review]
Refreshed to take comments into account

I'm happy with the code in this patch.

The only thing I'm not sure about is not having the dashes before the signature. I'm not saying I disagree, I'm just not sure what should happen, and I know Bryan has had discussions on this recently. Therefore requesting his ui-review to check this is what we want to do.

Comment 80

[Magnus Melin [:mkmelin]]

As with most things concerning signatures, people will likely get upset whatever you do... :) You can literally spend a day reading just a couple bugs where the signature options got implemented.

Having "-- " above would cause severe flaming, but i wonder if we couldn't instead use "--" for top posting and visual consistency. (Though I don't think this is the bug to do that.)

Comment 81

[Karsten Düsterloh]

> As with most things concerning signatures, people will likely get upset
> whatever you do... :)

Yeah. ;-)

> Having "-- " above would cause severe flaming, but i wonder if we couldn't
> instead use "--" for top posting and visual consistency.

But deliberately introducing a broken signature delimiter would be just bad.

Comment 82

[Magnus Melin [:mkmelin]]

> But deliberately introducing a broken signature delimiter would be just bad.

Heh, but it wouldn't be a signature delimiter then would it? ;) 

Comment 83

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

My understanding, after spending a couple days reading all those signature bugs, is that if we add the "-- " to a signature above an inline message forward thunderbird will render everything below that signature delimiter as part of the signature (bug 54570). 

Alternatively the current behavior always inserts this "-- " delimiter above all signatures so we could maintain consistency by continuing in that path.

My recommendation in bug 58406 was to use the delimiter by default, but not require it.  But that recommendation would likely still create the render problem. 

I would exclude the "-- " and not include anything else.  If bug 54570 is fixed before bug 58406 gets fixed there shouldn't be a problem.

Comment 84

[Magnus Melin [:mkmelin]]

Bryan: the patch does that - exclude the "-- " and not include anything else. (Just does the same thing as what we currently do for replies.)

Did you mean to mark it ui‑review+ 

Comment 85

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

Comment on attachment 317497 [details] [diff] [review]
Refreshed to take comments into account

right, sorry about that.  i'm still new at this. :)

Comment 86

[Magnus Melin [:mkmelin]]

np:)

Comment 87

[Mark Banner (:standard8)]

Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.569; previous revision: 1.568
done

I'm assuming this is fixed now, so marking as fixed.

Comment 89

[rtoliver.80919]

This was marked as FIXED on 6/11/08.  I just installed Tbird 2.0.0.16, but the problem does not appear to be fixed in this release.  Do I have to do anything to activate the fix or is it scheduled to be included in a later release?

Comment 90

[Magnus Melin [:mkmelin]]

It's just fixed for thunderbird3 (and nightly builds). In general, 2.0.0.x just get security and crash fixes.

Comment 91

[Worcester12345]

This is a big bug, and it would make a lot of sense to go above and beyond and include the code for the current shipping branch.

Comment 92

[Worcester12345]

(In reply to comment #91)
> This is a big bug, and it would make a lot of sense to go above and beyond and
> include the code for the current shipping branch.
> 

Still running into issues with this problem. Is there any way that this can now be landed on branch builds, since it has had a chance to "bake" on trunk builds? 

Comment 93

[Daniel Veditz [:dveditz]]

Not blocking a security branch release. If the mail module owners want they could back-port the patch and ask for branch approval for the next release (today is code freeze for 1.8.1.17 so it's not making that one).

Comment 95

[Benoît E. Donnette]

Daniel and al : if there is a chance that it makes its way to another branch, just tell me which branch, I'd be happy to backport. I'll test that one feature and just let you test an accept.

Comment 97

[Jeffrey J. Atto]

Hi, I apologize for my ignorance of Bugzilla, but I've been patiently waiting several years for this fix to be implemented.  The status says "Resolved Fixed", so does that mean it will be added to Thunderbird during the next release? 

I am using Thunderbird version 2.0.0.23 and the problem is still not resolved.

Thank you in advance for any estimate of when this might be implemented.

Comment 98

[Phil Ringnalda (:philor)]

See comment 90.

Comment 99

[raths]

I am using thunderbird beta 4 and it's resolved !