30208 - Don't add "-- " divider if signature already starts with one

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[Roland Mainz]

RFE: Prefs should verify that the signature beginns with a "-- " (<-- note the
tailing blank !!) line and should reject the signature if the blank is missing,
forcing the user to use a correct signature seperator ("-- ") or using no
seperator at all.

Comment 1

[Richard Zach]

The "-- " separator is inserted automatically.  Right now, it's "--", but that
is bug 29119.

Comment 2

[Roland Mainz]

[Automatic insertion of signature seperator] But some people don't want to get
the "-- " set automatically (for example, because their numer of lines in the
sig is to large (>5 lines).

Example signature:
-- snip --
bla blah blah
xxx xxx xxx
-- 
xx xx xx xx
main signature
-- snip --

The "trick" here is to split the signature up in a signature and a non-signature
part to work around the <= 5 line limit.

Comment 3

[Jean-Francois Ducarroz]

I don't know yet what we should do about this. Rich, Phil, any comment?

Comment 4

[rhp (gone)]

We now support HTML signatures so I don't think this is a practical thing.

- rhp

Comment 5

[Phil Peterson]

The de factor standard is "-- " and we should insert that automatically, as we 
always have. I can't think of a valid reason to let users change the sig 
separator, which would have significant negative consequences in terms of 
interop with other mail/news clients. I don't understand why having a >5 line 
sig means users need a different separator.

Comment 6

[rhp (gone)]

Phil, 
What about HTML sigs?

- rhp

Comment 7

[Phil Peterson]

As I see it, the app should hard-code "-- " and insert it before every sig, no 
matter whether the sig is plain text or HTML.

Comment 8

[Roland Mainz]

Insert it ("-- ") before every sig if the signature itself doesn't contain a "--
" seperator.

Comment 9

[rhp (gone)]

I'll keep harping on this issue, but this is fine as long as the signature is 
plain text, but we don't want to try to parse any HTML to find "-- " since this 
is a slippery slope we shouldn't try to climb.

- rhp

Comment 10

[Roland Mainz]

Stepping thougth HTML in Mozilla is't very hard (in this case).
AFAIK Mozilla has a HTML2PlainText converter. Use it and feed the search-for-"--
"-function with the output. 
Thats all :-)

Comment 11

[Phil Peterson]

We really have bigger fish to fry than this. Matching the 4.x behavior is fine.
Moving the rest of this signature-parsing stuff out to the helpwanted list.

Comment 12

[Daniel Bratell]

I think this should be closed as WONTFIX. There's really no harm with inserting 
a "-- " even when the signature already contains it apart from a really minor 
esthetical glitch. 

To scan the signature for an already existing "-- " is a much more dubious way 
to walk and even if we find a sig divider somewhere in the sig, it might be 
correct to insert our own sig divider. It's only in the case where the sig 
divider is the very first thing in the sig file that would be unnecessary and 
that might not be very easy to detect after converting the file from HTML to 
text since that conversion might have dropped something.

Comment 13

[Ben Bucksch (:BenB)]

Daniel, tnx for ccing me, but I disagree with you :). IIRC, 4.x does detect a
leading "-- " in the sig file and doesn't insert one more itself then. In all
other cases, it inserts a "-- ". I think, this is reasonable. Adding 4xp.

I do see a valid reason for an option to omit the sig: To automatically include
some "polite blurb" like "Sincerely, My Name". But this might be abused, so I'm
not too keen to get that part implemented.

Re HTML sigs: IMO, there is no reason to include "-- " in an HTML msg. This
convention is for plaintext msgs, there is none (to my knowledge) for HTML msgs.
Howver, HTML has its own markup standard. IMO, we should just make one up
(possibly submitting it as draft standard or so, references welcome), e.g. "<div
class="signature">" or so. We could then recognize this tag in our HTML->TXT
converter and generate "-- " for plaintext from that. I expect this proposal to
be controversial and so propose to discuss it on .mail-news and then file a new
bug on that.

Comment 14

[Matthew T (active 1999-2002)]

Practical reason: The user might use other mailers besides Mozilla, with the
same signature file, and one or more of these mailers may neglect to add `-- '
at the start of the signature (but otherwise be good-quality software). So the
user may insert `-- ' herself at the start of the signature file, and Mozilla
should respect that.

I would guess fixing this would involve adding two lines of code.

Comment 15

[Gervase Markham [:gerv]]

So the fix for this is: if the first four characters of signature.txt are "--
\n" then those characters are removed from the signature before it is appended.

So where do we make this check? Can anyone on the mailnews team tell me?

Gerv

Comment 16

[Daniel Bratell]

Don't know if it's enough to check the first four chars. There could be 
whitespace there too.

Anyway, a guess is /mailnews/compose/src/nsMsgCompose.cpp or something similar. 
A search for "signature" or "sig" surely brings something up.

Comment 17

[Gervase Markham [:gerv]]

The check needs to be made here:
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompose.cpp#2633

But, in absence of any documentation whatsoever on our String classes, I have no
idea what to replace an nsAutoString with so that I can chop the first four
bytes off it, or how to accomplish this task. <sigh>

Why, oh why don't we have any docs?

Gerv

Comment 18

[Daniel Bratell]

But there are docs. Look at http://lxr.mozilla.org/seamonkey/source/string/doc/ 
. Also, you can always ask scc to hear about the latest in strings. :-)

There are also a lot of text in the header files in 
http://lxr.mozilla.org/seamonkey/source/string/public/

Anyway, there is nothing wrong with nsAutoStrings. If you would like everything 
but the first four chars you could make a substring of it with something like.

nsAutoString theSig;
...
nsReadingIterator<PRUnichar> start, end;
theSig.BeginReading(start);
theSig.EndReading(end);
start.advance(4); // It better be four chars long or more

WriteToOutput(Substring(start, end));
  - or -
// The string below might die with theSig
nsAString& shorterSig = Substring(start, end); 
 - or -
// The string below will probably be copied to a new buffer
nsAutoString shorterSig2 = Substring(start, end); 


The old nsString API also contains a Cut method that I believe is obsolete, but 
it's still possible to use.

Comment 19

[Gervase Markham [:gerv]]

> But there are docs. Look at 
> http://lxr.mozilla.org/seamonkey/source/string/doc/  

I was searching www.mozilla.org for "String Guide" and getting the following:
http://www.mozilla.org/projects/xpcom/string_guide.html
which contains no useful info at all.

That really needs sorting out.

I have a patch but I've put printfs in the code at the point I mentioned and it
does seem that it's not getting called when you Compose a new message. Anyone
know why that is? It seems very odd to me.

Gerv

Comment 20

[Ben Bucksch (:BenB)]

gerv, you found the right code. It should be called.

If you found a legal sig delimiter in the user-sig already, why don't you just
skip the output of *our* sig delimiter, instead of removing the one from the user?

Comment 21

[Gervase Markham [:gerv]]

Because that's far too sensible :-)

Yes, I'll do that. But I still can't work out why my changes aren't showing up.
It does install the library when I make...

Gerv 

Comment 22

[Ben Bucksch (:BenB)]

Do a make at mailnews/composer (maybe even mailnews/?), for your changes to show.

Comment 23

[Gervase Markham [:gerv]]

Thanks, BenB. OK, here's the patch. Looking for r=. 

It takes Substring(0, 4) of a string which is not guaranteed to be four
characters long, but nothing evil seems to happen when it isn't, so I assume
that Substring Does The Right Thing in this case. The reviewer might like to
comment on this.

Gerv

Comment 24

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Comment 25

[Gervase Markham [:gerv]]

Daniel, Roland: are either of you able to review this three-line fix?

Gerv

Comment 26

[Daniel Bratell]

How about "-- \r\n", is that not a legal divider? If not, and you've tested that
the patch works you can have my r=.

Comment 27

[Ben Bucksch (:BenB)]

You'll want to include CRLF in the if block.

I don't now the Mozilla string API version 4.51 ;-P. If anybody could just
review the line
!(Substring(sigData, 0, 4)).Equals(NS_LITERAL_STRING("-- \n"))
?

Comment 28

[Ben Bucksch (:BenB)]

Sorry, I missed Daniel's comment.
> You'll want to include CRLF in the if block.

s/CRLF/one of the new CRLFs, that we output,/

Comment 29

[Daniel Bratell]

As far as I can tell, the string code is fine. It's of the kind that scc
recommended just a week or two ago. 

As for the line ending, it will be in a file from the user, possibly shared
between several systems/OS:es so we should be tolerant unless there is a
standard specifying how it must look like.

Comment 30

[Gervase Markham [:gerv]]

Having checked the RFC, either line delimiter seems to be acceptable. What do
you mean, "new CRLFs"? I planned to add a second check for "-- \r\n" - won't
that do the trick? Do we need to check for "--  \r"? Could we just avoid this by
checking for "-- " on the basis that it's simpler and the chances of it Doing
The Wrong Thing are extremely small?

Gerv

Comment 31

[Ben Bucksch (:BenB)]

gerv,

if you test for |"-- \n" or "-- \r"|, you will catch all cases. (Unix uses LF,
Mac CR, DOS/Windows CRLF.)

With the "new CRLF", I mean the following lines in the source, which you can see
in the patch:
 if ( (!m_composeHTML) || ((m_composeHTML) && (!htmlSig)) )
       sigOutput.AppendWithConversion(CRLF);
 else if (m_composeHTML)
 ...

The constant "dashes" that you output contains only "-- ", not the newline we
add separately. If you just skip the putput of "-- ", you'll end up with 2
newlines that we output, the "-- " from the user's file plus the newline from
the user's file plus the real sig. that's one too much, we should output only
one newline.

Comment 32

[Roland Mainz]

Just one idea: It may be better to search the mail body _backwards_ for the
signature delimiter (e.g. "^-- $" (<-- regexp :-)) ...

Comment 33

[Gervase Markham [:gerv]]

Yer wot? We aren't searching the mail body, we are searching the signature text.

Gerv

Comment 34

[Roland Mainz]

OK... what happens if the user uses his/her 10 claws to add a short signature
manually ?

Comment 35

[Daniel Bratell]

This patch won't catch that. I don't think it's very common to add the sig by 
hand and also including the "-- \n" marker so I wouldn't worry to much about 
that.

Comment 36

[Roland Mainz]

I assume noone here does it that way... :-) But some people _do_ it that way...
it's silly but users do that (for example... one of my coworkers here adds his
signature file manually (via DnD) each time because he has three different
ones).
We should the ~/.signature file and manually added signatures the same way to
avoid user confusion...

Comment 37

[Gervase Markham [:gerv]]

This patch does exactly the following:

_if_ the user has a signature file and _if_ that signature file is auto-included 
by Mozilla and _if_ that file begins "-- \n|\r" then we don't add a second 
instance of the sig sep.

What does this have to do with inserting the signature manually?

Please read the code! :-)

Gerv

Comment 38

[timeless]

Is it unreasonable for someone to want a signature like 'foo\nbar\n-- \nyodel"?

.appendWithConversion() of constants usually bothers me.  if you're going to 
change the line could you see about getting append(NS_LITERAL_STRING("-- \n")) 
or something instead? [you probably want NS_NAMED_LITERAL_STRING(wDashes,"-- 
")) and then just use something to add the appropriate newline marker 
|wDashes+PRUnichar('\n')|, ...]

if you end up changing lots of lines, this could be changed
     if ( (!m_composeHTML) || ((m_composeHTML) && (!htmlSig)) )
to (!m_composeHTML || !htmlSig), if not i'll go file a bug for abuse of logic 
operators and stuff... (it appears most other instances of this were fixed)

Comment 39

[Daniel Bratell]

Ah. Roland, I think you are right. We could be smarter. Unless it's too bad for
performance, we could search both the body and sig for "-- <newline>" and we
wouldn't even require it to be in the beginning of the sig file.

pseudo:

PRBool HasSigDelimiter(const nsAString& aString)
{
  PRBool hasSigDelimiter = PR_FALSE;
  nsReadingIterator<PRUnichar> stringStart, start, end, stringEnd;
  aString.BeginReading(start);
  aString.BeginReading(bodyStart);
  aString.EndReading(end);
  aString.EndReading(bodyEnd);

  if (!FindInReadable(NS_LITERAL_STRING("-- "), start, end))
    return PR_FALSE;

  // Check that it ends the right way
  if (end == bodyEnd ||  // or maybe "end != bodyEnd &&"
      (*end != '\r' && *end != '\n'))
    return PR_FALSE;

  // Check that it starts at a line start
  if (start != bodyStart &&
      *(--start) != '\r' &&
      *start != '\n')
    return PR_FALSE;

  return PR_TRUE;
}

...
...
PRBool hasSigDelimiter = HasSigDelimiter(sig) || HasSigDelimiter(msgBody);
...
...



This adds an extra pass over the string data though. Maybe we should only look
at the last 1 KB or so of the mail data?

So make that:

if (aString.Length() > 1024) 
start.advance(aString.Length()-1024); // Advance to the last 1 KB.

before the Search.

But Gerv, it's you who's working on this. Include it if you want and think it's
the right thing to do. We can always replace it later if wanted. I don't even
know if this code has access to the full message body.

Comment 40

[Ben Bucksch (:BenB)]

Roland,
If the user manually adds a signature, he won't use the "add signature file"
feature of Mozilla and this whole code won't be triggered at all (i.e. we won't
add dashes anyway).

gerv,
now that timeless mentions it, I realize that you use "-- \r" as literal, while
we defined similar constants above. Maybe you can use the latter, maybe not.

> (!m_composeHTML) || ((m_composeHTML) && (!htmlSig)) )
> to (!m_composeHTML || !htmlSig)

right.

Comment 41

[Roland Mainz]

Do we search only the first text/plain or text/html body ? What happens in the
multipart/alternative case where the parts are "text/html" || "text/plain" ?
(just quick&dumb questions without looking at the source... ;-( )

Comment 42

[Ben Bucksch (:BenB)]

Roland, I fail to understand what you are talking about. This bug is about: We
assume that the signature file does not include sigdash and thus add one before.
If the signature file already contains it, we end up with two. This bug is about
preventing this. What does this have to do with the body?

Comment 43

[Roland Mainz]

OK, slighly wrong question (+slightly OT in this bug...):
Does the code which checks if the mail body contains already a signature check
all parts of a multipart/alternative or only the first part or how does it work
?

Comment 44

[Daniel Bratell]

Ok, forget my last comment. Roland made me confused. Bad Roland. :-) 

But you could still use the code to check for the "-- \n" somewhere else but the 
beginning of the signature. As someone said, I could have a sig:

/Daniel

-- 
This is a funny text. Laugh!

Comment 45

[Ben Bucksch (:BenB)]

> OK, slighly wrong question (+slightly OT in this bug...):
> Does the code which checks if the mail body contains already a signature check
> all parts of a multipart/alternative or only the first part or how does it work
> ?

No, we don't check the body at all. Actually, if we send format=flowed, we strip
the trailing space in the body.

Comment 46

[Roland Mainz]

> No, we don't check the body at all. Actually, if we send format=flowed, we 
> strip the trailing space in the body.

Ouch. This kills manually added signatures. Should I open a seperate bug for
this ?

Comment 47

[Ben Bucksch (:BenB)]

Yes, we can discuss there then. (It might end up as wontfix, dunno.)

Comment 48

[Roland Mainz]

Why should that bug be "won't fix" ??

Comment 49

[Daniel Bratell]

No you shouldn't open a bug since there is no bug. The space after a "--" won't 
be stripped. Ben, you shouldn't start rumors like that. :-)

From PlainTextSerializer:

 if(!(mFlags & nsIDocumentEncoder::OutputPreformatted) &&
    (aSoftlinebreak || !mCurrentLine.EqualsWithConversion("-- "))) {
      // Remove SPACE:s from the end of the line.
     

Comment 50

[Gervase Markham [:gerv]]

Attachment: Patch v.2 - check both \n and \r

Comment 51

[Gervase Markham [:gerv]]

New patch attached. Looking for r=.

<thought>
Could it possibly by a symptom of Mozilla's current problems that, after this
bug has been dormant for months, a single line of code produces 23 comments? If
the ratio of code to discussion were the other way around, Mozilla would improve
a lot faster.
</thought>

Gerv

Comment 52

[Daniel Bratell]

You shouldn't assign the substring to another string. You should only keep a 
reference to it, and you don't even have to know that it's a 
nsDependentSubstring. nsDependentSubstring is one of those internal strings that 
shouldn't be visible more than necessary.

nsAString& firstFourChars = Substring(...); 

is fine. 

With that change you have my r= fwiw.

Comment 53

[Gervase Markham [:gerv]]

Attachment: r= ed patch - looking for sr=

Comment 54

[(not reading, please use [e-mail] instead)]

do you have some unneeded parenthesis in there?

you have:

+    if (!(firstFourChars.Equals(NS_LITERAL_STRING("-- \n")) ||
+         (firstFourChars.Equals(NS_LITERAL_STRING("-- \r"))))

shouldn't it be:

+    if (!(firstFourChars.Equals(NS_LITERAL_STRING("-- \n")) ||
+          firstFourChars.Equals(NS_LITERAL_STRING("-- \r")))

also, 4.x linux (which also did the "-- " check) did not care if the 4th char
was a new line or not.  if it saw "-- " it didn't insert the "-- \n".

seems like:

+ if (!(firstThreeChars.Equals(NS_LITERAL_STRING("-- "))))

would be enough.

> <thought>
> Could it possibly by a symptom of Mozilla's current problems that, after this
> bug has been dormant for months, a single line of code produces 23 comments?
> If the ratio of code to discussion were the other way around, Mozilla would 
> improve a lot faster.
> </thought>

we're just focused on more important bugs.  that's why this bug has rotted for
so long.

Comment 55

[Ben Bucksch (:BenB)]

> 4.x linux (which also did the "-- " check) did not care if the 4th char
> was a new line or not. 

That's a bug in 4.x then. The delimiter is defined as dash, dash, space, newline.

Comment 56

[Daniel Bratell]

Yes, sigs like:
--   Look, a bird!  -- 

ain't that obscure and should not trigger the "-- \n" removal.

Comment 57

[Gervase Markham [:gerv]]

seth: you are correct about the parenthesis. I'll fix that. Do I have sr= when I 
do? As Ben says, we should be checking for the full delimiter.

When I said "we should have more coding and less talking", this was a general 
point, and not aimed at you or this bug. The level of noise caused by any 
submitted patch is really high across the entirety of Bugzilla, and it doesn't 
depend on how important the bug is.

Gerv

Comment 58

[Gervase Markham [:gerv]]

Attachment: Patch v.4 with parentheses removed - looking for sr=

Comment 59

[Jean-Francois Ducarroz]

I cannot give you a SR but as the module owner, R=ducarroz for the last patch.

Comment 60

[Blake Ross]

Missed 0.9.3.

Comment 61

[Erb]

What I like about the Netscape 4.x way is that I can start my sig file with:
-- Erb

and get the "--" on the same line as my name, which is what I like to do
(is that strange?)

Comment 62

[(not reading, please use [e-mail] instead)]

sr=sspitzer

Comment 63

[Gervase Markham [:gerv]]

Checked in.

Gerv

Comment 64

[Gervase Markham [:gerv]]

Reopening. I had to back this out.

Gerv

Comment 65

[Bradley Baetz (:bbaetz)]

The nsAString& had to be changed to a const nsAString&, but I thought that we
have had issues in the past with old compilers and the lifetime of temps bound
to references.

dbaron?

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

+    nsAString& firstFourChars = Substring(sigData, 0, 4);

will crash on a build compiled with gcc 2.7.2.3 since it doesn't support the
rules for the lifetime of temporaries bound to references, but simply destroys
the temporary.

Comment 67

[Bradley Baetz (:bbaetz)]

Ignore my last comment, its the const foo& = a ? b : c; form that older
compilers have problems with.

Comment 68

[Gervase Markham [:gerv]]

Let's hope it stays in this time. :-) I built it right before I checked in, so
if it doesn't work then the world is upside down.

Gerv

Comment 69

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Again, that patch will NOT work on gcc 2.7.2.3 (it will crash).  And we have no
gcc 2.7.2.3 tinderbox right now...

Comment 70

[Gervase Markham [:gerv]]

There appears to have been some confusion here, which bbaetz has now
straightened out. I thought dbaron's comment had been addressed, but it seems
not.

bbaetz tells me using     
nsAutoString firstFourChars(Substring(sigData, 0, 4));
will do the trick. 

dbaron - is this OK with you?

Gerv

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's OK, but an unnecessary copy.  It would be preferable to use:

nsDependentSubstring firstFourChars(sigData, 0, 4);

Comment 72

[Gervase Markham [:gerv]]

Attachment: Patch B v.1

Comment 73

[Gervase Markham [:gerv]]

dbaron, bbaetz, blake: 

Could you have a look at this patch, please? It's the one-liner dbaron
recommended to fix gcc 2.7.2.3. 

Gerv

Comment 74

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks fine to me.

Comment 75

[(not reading, please use [e-mail] instead)]

sr=sspitzer

Comment 76

[Gervase Markham [:gerv]]

Die, dammit! :-)

Gerv

Comment 77

[esther]

Using build 2001-09-04 on win, mac and linux and using a created signature with 
"-- " (minus the quotes) as the first line, this is fixed.  When viewing the 
composed msg and receiving it, we don't add another "-- " to the signature.  
Verified.

Comment 78

[Alexander Skwar]

Sorry to tune into this "ancient" bug, but using the current builds (2001-12),
it's not acting like I'd expect it to.

I'd expect Mozilla to only prefix the "-- \n", if the signature file does not
contain a "-- \n" line.  I've got my .sig like this:

-- START --
Alexander Skwar
-- 
How to quote: http://learn.to/quote (german) http://quote.6x.to (en)
   iso-top.de - Die günstige Art an Linux Distributionen zu kommen
        -=» Suche Wohnung in Gelsenkirchen und Umgebung! «=- 
                       Uptime: 1 day 3 hours 50 minutes
-- END --

(Without the -- START -- and -- END --)

With other mailers (like mutt) or newsreaders (like pan, Xnews) I can use this
exact .sig, as they can either detect automatically that there already is a "--
\n" or can at least be configured so, that no "-- \n" is inserted.  That's the
right way to go, IMO.

Please reopen and change!

Comment 79

[Daniel Bratell]

Alexander, it's much better if you open a new bug for that issue. This one is 
already verified fixed for the feature it tracks (see the summary) and its much 
easier to work with many small bug reports than bugs that keep changing.

Comment 80

[Alexander Skwar]

Fine; see bug 113684