[Kde-pim] Review Request: Remove all the signature blocks.

[Shai Berger]

> On 2009-03-24 06:49:19, Thomas McGuire wrote:
> > What happens in your case if "-- " is contained in the mail body somewhere and not meant as a separator? Like in "And then I did this -- can you believe it!?". I think you should only accept "-- " as a signature separator if it is either at the start of the mail or within a quoted text (quoted text: only >, | and ' ' before the signature)
> > 
> > The test code you wrote is an ideal candidate for writing unit tests for. Since the code is not trivial, please add a small unit tests, with those manual tests you did and the "-- in body" case I described above.
> > 
> > See http://techbase.kde.org/Development/Tutorials/Unittests.
> > Also see the utiltest and mimelibtest of KMail as an example on how to write tests.
> > 
> > Now, please change some coding style:
> > 2 spaces indentation, no tabs please
> > Spaces inside parenthesis and around operators
> > No variable names with underscores (_), use camelCase instead
> > 0!=prefix.size() => !prefix.isEmpty()
> > do .. while true => forever
> > next_prefix=""; => nextPrefix.clear()
> > "\n" => '\n'
> 
> Shai Berger wrote:
>     If I understand the code correctly, "--" will only be considered as a separator if it is the last thing on the line (except for whitespace), but Thomas is right: You need to check for nothing-but-quotation-marks before the separator too.
>     
>     The example you might want to keep in mind is a "framed" SQL comment block:
>     
>     ---------------------------------------
>     -- And here we do some fancy selects --
>     -- Just to show how cool we are      --
>     ---------------------------------------
>     
>     The current code, as I understand it, would remove all these lines.
> 
> Thomas McGuire wrote:
>     The SQL comment block will be deleted, yes. 
>     There is no way around that, "-- " is the signature separator, and we can't detect the difference to a comment block. I think.

The Wikipedia page[1] linked from bug #72316 says: """...must be delimited from the body of the message by a single line consisting of exactly two hyphens, followed by a space, followed by the end of line (i.e., "-- \r\n")""". In this, it echoes its own source RFC3673[2] -- the sig is indicated by a separator line, and none of the lines in the SQL comment block is a proper separator line.

In other words, "-- " is not the signature separator -- /^-- \r\n/ is, and exceptions should only apply to quoting IMHO.

[1] http://en.wikipedia.org/wiki/Signature_block
[2] http://tools.ietf.org/html/rfc3676#section-4.3


- Shai


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/403/#review586
-----------------------------------------------------------


On 2009-03-23 09:18:28, Jaime Torres wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/403/
> -----------------------------------------------------------
> 
> (Updated 2009-03-23 09:18:28)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Remove all the signature blocks.
> The algorithm is:
> Look for the prefixes before the "-- " marks (i.e. the text before it in the line).
> foreach line, if the line contains "-- ", start to delete lines until the prefix changes.
> 
> (it should also work with c code if the -- is not followed by a space).
> 
> 
> This addresses bug 72316.
>     https://bugs.kde.org/show_bug.cgi?id=72316
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/kmmessage.cpp 940552 
> 
> Diff: http://reviewboard.kde.org/r/403/diff
> 
> 
> Testing
> -------
> 
> If checked with the following code:
> const QString mail="text1\n-- \nSignature Block1\nSignature Block1\n\n> text2\n"\
>                    "> -- \n> Signature Block 2\n> Signature Block 2\n"\
>                    ">> text3\n>> text3\n>> -- \n>> Signature Block 3\n>> Signature Block 3\n"\
>                    "> text4\n> -- \n> Signature Block 4\n"\
>                    "text5\n-- \nSignature Block 5\n";
> const QString mail1="text1\n-- \nSignature Block1\nSignature Block1\n\n"\
> 				   ">text2\n>-- \n>Signature Block 2\n>Signature Block 2\n" \
>                    ">>text3\n>>text3\n>>-- \n>>Signature Block 3\n>>Signature Block 3\n"\
>                    ">text4\n>-- \n>Signature Block 4\n>Signature Block 4\n"\
>                    "text5\n-- \nSignature Block 5\n";
>                    
> const QString mail2="Text 1\n-- \nFirst sign\n\n\n> From: bla\n"\
> "> Texto 2\n\n> Aquí algo de texto.\n\n> --\n> Signature Block 2\n\n> Adios\n"\
> "\n>> Texto 3\n\n>> --\n>> Signature block 3\n";
> 
> const QString mail3="-- \n-- ACME, Inc\n-- Joe User\n-- PHB\n-- Tel.: 555 1234\n--";
> 
> 	QString res=stripSignature(mail,1);
> 	cout<<"res:"<<res.toStdString()<<endl;
> 	res=stripSignature(mail1,1);
> 	cout<<"res:"<<res.toStdString()<<endl;
> 	res=stripSignature(mail2,1);
> 	cout<<"res:"<<res.toStdString()<<endl;
> 	res=stripSignature(mail3,1);
> 	cout<<"res:"<<res.toStdString()<<endl;
> 
> 
> Thanks,
> 
> Jaime
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/