[Kde-pim] Review Request: Remove all the signature blocks.
Jaime Torres
jtamate at gmail.com
Wed Mar 25 10:08:49 GMT 2009
> 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.
>
> Shai Berger wrote:
> 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
>
> Jaime Torres wrote:
> Modifying the regular expresion to "-- $", the sql comments are not Signature Blocks.
>
> A question:
> What do you prefer? To remove text replies or to not remove signature blocks completely?
> In the current implementation, if there is a reply block without signature, between parts with signature blocks, it is removed.
> Another possibility is to check only for prefix change (even in the case the signature contains > as a prefix), removing only part of the signature.
>
> I do not see a solution to remove them completely if they contains the common reply characters as part of them, like:
>
> text
> "-- " (The " are there only to remark the space)
> >This is a "not very good signature block" (I do not know if it is very common, anyway).
> >
> > Reply text
> >> Reply Reply text
> >> "-- "
> >> #Signature Block
>
Sorry for the noise. I do not see my comments in the reviewboard...
- Jaime
-----------------------------------------------------------
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/
More information about the kde-pim
mailing list