[Kde-pim] Review Request: Remove all the signature blocks.
Ingo Klöcker
kloecker at kde.org
Sat Mar 28 16:42:46 GMT 2009
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/403/#review666
-----------------------------------------------------------
In general the solution is okay, but there are a few things that need to be improved.
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment423>
Make this variable const.
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment422>
Use QChar instead of QRegExp if you look for a single character. QString::indexOf( QChar ch, ... ) is much faster than QString::indexOf( const QRegExp &, ... ).
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment424>
Always declare variables as locally as possible, i.e. move the declaration of prefix, posSignatureBlock and posNewLine into the outer while-loop and the declaration of line into the inner while-loop.
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment425>
Do not start the search at the begin, but at the position where the last search stopped. I think
res.indexOf( SBDelimiterSearch, posDeletingStart - 1 )
should work (if you also change the initialization of posDeletingStart to 0).
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment426>
If this is the last line then mid() is called with a negative number less than -1. The behavior of QString::mid() is not documented for n < -1, so it might be good to handle the case of res.indexOf( '\n', posNewLine ) returning -1 explicitly.
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment427>
Hmm, I think this fails if a signature contains a '>' character somewhere. What you really want to check is whether the line starts with '>', i.e.
( prefix.isEmpty() && !line.startsWith( CommonReplySearch ) )
/trunk/KDE/kdepim/kmail/kmmessage.cpp
<http://reviewboard.kde.org/r/403/#comment428>
If line contains only prefix then line.mid( prefix.size(), 1 ) will return an empty string. This might be a problem if prefix.contains( empty string ) returns true.
Moreover this will fail if a signature is indented with space characters because the prefix usually consists of '>' and space characters.
I suggest to check whether the character following the prefix is a '>' (or rather is not a '>').
- Ingo
On 2009-03-27 10:15:18, Jaime Torres wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/403/
> -----------------------------------------------------------
>
> (Updated 2009-03-27 10:15:18)
>
>
> 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 945537
>
> Diff: http://reviewboard.kde.org/r/403/diff
>
>
> Testing
> -------
>
> #include <QtCore/QtCore>
> #include <iostream>
> #include <cassert>
>
> using namespace std;
>
> /*
> remove the Signature Blocks (SB) from prefix+"-- (end of line)" until a
> line that
> * does not starts with prefix or
> * starts with prefix+(any substring of prefix)
> @param msg. The message to remove the SB.
> @param clearSigned. Before a message is signed all trailing whitespace is
> removed. Therefore the signature separator loses the trailing space.
> */
> static QString stripSignature ( const QString & msg, bool clearSigned ) {
> // Following RFC 3676, only > before --
> // I prefer to not delete a SB instead of delete good mail content.
> QRegExp SBDelimiterSearch = clearSigned ?
> QRegExp( "(^|\n)[> ]*--\\s?\n" ) : QRegExp( "(^|\n)[> ]*-- \n" );
> // The regular expresion to look for prefix change
> QRegExp CommonReplySearch = QRegExp( "[>]" );
>
> QString res = msg;
> QString prefix; // the current prefix
> QString line; // the line to check if is part of the SB
> int posDeletingStart = -1;
> int posNewLine = -1;
> int posSignatureBlock = -1;
>
> // While there are SB delimiters
> while ( (posDeletingStart = res.indexOf( SBDelimiterSearch )) >= 0 )
> {
> // Look back for the line begining
> posSignatureBlock = res.indexOf( '-', posDeletingStart );
> // The prefix before "-- "$
> if (res[posDeletingStart]=='\n') ++posDeletingStart;
> prefix = res.mid( posDeletingStart, posSignatureBlock - posDeletingStart );
> posNewLine = res.indexOf( '\n', posSignatureBlock ) + 1;
>
> // now go to the end of the SB
> while ( posNewLine < res.size() && posNewLine > 0 )
> {
> // check when the SB ends
> line = res.mid( posNewLine, res.indexOf( '\n', posNewLine ) - posNewLine );
>
> // * does not starts with prefix or
> // * starts with prefix+(any substring of prefix)
> if ( (prefix.isEmpty() && line.indexOf( CommonReplySearch ) < 0) ||
> (!prefix.isEmpty() && line.startsWith( prefix ) &&
> !prefix.contains( line.mid( prefix.size(), 1 ) )) )
> {
> posNewLine = res.indexOf( '\n', posNewLine ) + 1;
> }
> else
> break; // end of the SB
> }
> // remove the SB or truncate when is the last SB
> if ( posNewLine > 0 )
> res.remove( posDeletingStart, posNewLine - posDeletingStart );
> else
> res.truncate( posDeletingStart );
> }
>
> return res;
> }
>
> int
> main ( int argc, const char *argv[] )
> {
>
> QStringList tests;
> tests << "text1\n-- \nSignature Block1\nSignature Block1\n\n"
> "> text2\n> -- \n> Signature Block 2\n> Signature Block 2\n"
> ">> text3 -- not a signature block\n>> text3\n"
> ">>> text4\n> -- \n> Signature Block 4\n> Signature Block 4\n"
> ">>-------------\n>>-- text5 --\n>>-------------------\n"
> ">>-- \n>> Signature Block 5\n"
> "text6\n-- \nSignature Block 6";
> tests << "text1\n"
> "> text2\n"
> ">> text3 -- not a signature block\n>> text3\n"
> ">>> text4\n"
> ">>-------------\n>>-- text5 --\n>>-------------------\n"
> "text6\n";
> tests << "text1\n"
> "> text2\n"
> ">> text3 -- not a signature block\n>> text3\n"
> ">>> text4\n"
> ">>-------------\n>>-- text5 --\n>>-------------------\n"
> "text6\n";
>
> tests << "text1\n-- \nSignature Block1\nSignature Block1\n\n"
> ">text2\n>-- \n>Signature Block 2\n>Signature Block 2\n"
> "> >text3\n> >text3\n> >-- \n>>Not Signature Block 3\n"
> "> > Not Signature Block 3\n"
> ">text4\n>-- \n>Signature Block 4\n>Signature Block 4\n"
> "text5\n-- \nSignature Block 5";
> tests << "text1\n"
> ">text2\n"
> ">>Not Signature Block 3\n> > Not Signature Block 3\n"
> ">text4\n"
> "text5\n";
> tests << "text1\n"
> ">text2\n"
> ">>Not Signature Block 3\n> > Not Signature Block 3\n"
> ">text4\n"
> "text5\n";
>
> tests << "Text 1\n-- \nFirst sign\n\n\n> From: bla\n"
> "> Texto 2\n\n> Aqui algo de texto.\n\n>> --\n"
> ">> Not Signature Block 2\n\n> Adios\n"
> "\n>> Texto 3\n\n>> --\n>> Not Signature block 3\n";
> tests << "Text 1\n> From: bla\n"
> "> Texto 2\n\n> Aqui algo de texto.\n\n>> --\n"
> ">> Not Signature Block 2\n\n> Adios\n"
> "\n>> Texto 3\n\n>> --\n>> Not Signature block 3\n";
> tests << "Text 1\n> From: bla\n"
> "> Texto 2\n\n> Aqui algo de texto.\n\n\n> Adios\n"
> "\n>> Texto 3\n\n";
>
> tests << "-- \n-- ACME, Inc\n-- Joe User\n-- PHB\n-- Tel.: 555 1234\n--";
> tests << "";
> tests << "";
>
> tests << "Text 1\n\n\n\n> From: bla\n"
> "> Texto 2\n\n> Aqui algo de texto.\n\n"
> ">> Not Signature Block 2\n\n> Adios\n"
> "\n>> Texto 3\n\n>> --\n>> Not Signature block 3\n";
> tests << "Text 1\n\n\n\n> From: bla\n"
> "> Texto 2\n\n> Aqui algo de texto.\n\n"
> ">> Not Signature Block 2\n\n> Adios\n"
> "\n>> Texto 3\n\n>> --\n>> Not Signature block 3\n";
> tests << "Text 1\n\n\n\n> From: bla\n"
> "> Texto 2\n\n> Aqui algo de texto.\n\n"
> ">> Not Signature Block 2\n\n> Adios\n"
> "\n>> Texto 3\n\n";
>
> tests << "Without Signature Blocks";
> tests << "Without Signature Blocks";
> tests << "Without Signature Blocks";
>
> QString t1, t2, t3;
> for ( int i = 0; i < 100; i++ )
> {
> t1.append( tests[9] );
> t2.append( tests[10] );
> t3.append( tests[11] );
> }
> tests << t1 << t2 << t3;
>
> QTime time = QTime::currentTime();
>
> for ( int j = 0; j < 200; j++ )
> {
> for ( int i = 0; i < 7; i++ )
> {
> QString res = stripSignature( tests[i * 3], j%2 );
> // cout << "result:\n*" << res.toStdString() << "*" << endl << "expected:\n*" << tests[i*3+1+(j%2)].toStdString() << "*" <<endl;
> assert( res == tests[i * 3 + 1 + (j%2)] );
> }
> }
>
> cout << time.elapsed() << 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