[Kde-pim] Review Request 109325: fixing tests in messageviewer
Sandro Knauß
mail at sandroknauss.de
Mon Mar 11 01:54:20 GMT 2013
> On March 7, 2013, 6:36 a.m., Laurent Montel wrote:
> > messageviewer/objecttreeparser.cpp, line 3111
> > <http://git.reviewboard.kde.org/r/109325/diff/1/?file=117580#file117580line3111>
> >
> > Before to fix objecttreeparser which is a special class, you need to show a bug report about it.
> > you replace a += to = it will change a lot of thing I think. so you need to show bug about it thanks.
> >
>
> Sandro Knauß wrote:
> The tests themself:
> http://build.kde.org/job/kdepim_master/1212/testReport/junit/(root)/TestSuite/messageviewer_objecttreeparsertest/
> [...]
> Actual (otp.plainTextContent()): This is the encapsulating message.This is the encapsulating message.
>
> Expected (QString( "This is the encapsulating message." )): This is the encapsulating message.
> [...]
> I think it is not intended to repeat the message.
>
> No it isn't used a lot:
> cd kdepim && grep -rl 'plainTextContent\W'
>
> messageviewer/objecttreeparser.cpp
> messageviewer/objecttreeparser.h
> messageviewer/tests/unencryptedmessagetest.cpp
> messageviewer/tests/objecttreeparsertest.cpp
> composereditor-ng/composereditor.cpp ( not related )
> composereditor-ng/composereditor.h ( not related )
> messagecomposer/composerviewbase.cpp ( no tests available )
> templateparser/templateparser.cpp: ( tested via templateparsertest )
> templateparser/tests/templateparsertest.cpp: QVERIFY( !otp.plainTextContent().isEmpty() );
> blogilo/src/postentry.h ( not related )
> blogilo/src/postentry.cpp ( not related )
>
> So there are one two places, where it used
> * Unfortuantelly templateparsertest is broken *grmpf*
> * I can't say what will be happen is messagecomposer/composerviewbase.cpp.
>
>
>
> Laurent Montel wrote:
> For me fix and potential break kmail/messageviewer to fix a unittest is not correct.
> until now nobody reported a bug about it.
> I prefere a unittest broken that a messageviewer/messagecomposer broken.
> until you prouve that it's correct and not break it, I don't want that you commit it.
> Regards.
>
> Sandro Knauß wrote:
> I just want to mention the documentation of ObjectTreeParser::plainTextContent(). It should not return the text of the message twice.
> /**
> * The text of the message, ie. what would appear in the
> * composer's text editor if this was edited or replied to.
> * This is usually the content of the first text/plain MIME part.
> */
> QString plainTextContent() const { return mPlainTextContent; }
>
> I'll try to make sure that messagecomposer/composerviewbase.cpp and templateparser/tests/templateparsertest.cpp won't break.
>
> Sandro Knauß wrote:
> Ok now have looked up the chain form templateparser to objecttreeparser changes. There is only one path to objecttrreparser. Here are the relevat lines:
> TemplateParser::TemplateParser
> mEmptySource = new MessageViewer::EmptySource; //htmlWriter() = 0
> mEmptySource->setAllowDecryption( mAllowDecryption );
> mOtp = new MessageViewer::ObjectTreeParser( mEmptySource );
>
> TemplateParser::processWithTemplate
> mOtp->parseObjectTree( mOrigMsg.get() );
>
> TemplateParser::plainMessageText
> QString result = mOtp->plainTextContent();
>
> Because htmlWriter() is 0, ObjectTreeParser::writeBodyString won't let us to ObjectTreeParser::writeBodyStr, where the changes of mPlainTextContent are:
>
> void ObjectTreeParser::writeBodyString( const QByteArray & bodyString,
> const QString & fromAddress,
> const QTextCodec * codec,
> ProcessResult & result,
> bool decorate )
> {
> // FIXME: This is wrong, it means that inline PGP messages will not be decrypted when there is no
> // HTML writer. Even if there would be a HTML writer, the decrypted inline PGP text is not
> // added to the textual content.
> // The solution would be to remove this if statement and make writeBodyStr() add the
> // decrypted string to the textual content as well, and removing any manual modifictions
> // of the textual content by callers of this method.
>
> kDebug() << "writeBodyString";
> if ( !htmlWriter() )
> return;
>
> [...]
>
> }
>
> For me this patch can't harm templateparser.
>
> Same is true for messagecomposer/composerviewbase.cpp:
>
> void Message::ComposerViewBase::updateTemplate ( const KMime::Message::Ptr& msg )
> {
> // First, we copy the message and then parse it to the object tree parser.
> // The otp gets the message text out of it, in textualContent(), and also decrypts
> // the message if necessary.
> KMime::Content *msgContent = new KMime::Content;
> msgContent->setContent( msg->encodedContent() );
> msgContent->parse();
> MessageViewer::EmptySource emptySource;
> MessageViewer::ObjectTreeParser otp( &emptySource );//All default are ok
> otp.parseObjectTree( msgContent );
> // Set the HTML text and collect HTML images
> if ( !otp.htmlContent().isEmpty() ) {
> m_editor->setHtml( otp.htmlContent() );
> emit enableHtml();
> collectImages( msg.get() );
> } else {
> m_editor->setPlainText( otp.plainTextContent() );
> [...]
> }
>
> Here the relevat part of messageviewer/objecttreeemptysource.cpp:
>
> HtmlWriter * EmptySource::htmlWriter()
> {
> return 0;
> }
>
>
> -> My patch at objecttreeparser is harmless: qed
Btw. Because TemplateParser::processWithTemplate isn't affected from this patch is the reason for bug #298911 - Reply to decrypted mail quotes encrypted text
- Sandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109325/#review28745
-----------------------------------------------------------
On March 6, 2013, 11:33 p.m., Sandro Knauß wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109325/
> -----------------------------------------------------------
>
> (Updated March 6, 2013, 11:33 p.m.)
>
>
> Review request for KDEPIM.
>
>
> Description
> -------
>
> fixed messageviewer-objecttreeparsertest
> fixed messageviewer-unencryptedmessagetest
>
> one problem is still occures: gpgsm will open a pinentry window that has to be closed via mouse click.
>
>
> Diffs
> -----
>
> messageviewer/objecttreeparser.cpp d41e77f65adc20374b2ec358a80d9aebf6e9fed4
> messageviewer/tests/objecttreeparsertest.cpp ded141a6444a98392096984ad8f29fc4ea2b2210
> messageviewer/tests/unencryptedmessagetest.cpp 6b3dc7649b171df1746a8b43842da0690127a6bb
>
> Diff: http://git.reviewboard.kde.org/r/109325/diff/
>
>
> Testing
> -------
>
> cd messageviewer && make test
> Running tests...
> Test project /home/kdetest/kde/build/kdepim/messageviewer
> Start 1: messageviewer-htmlquotecolorertest
> 1/5 Test #1: messageviewer-htmlquotecolorertest ..... Passed 0.61 sec
> Start 2: messageviewer-objecttreeparsertest
> 2/5 Test #2: messageviewer-objecttreeparsertest ..... Passed 0.54 sec
> Start 3: messageviewer-rendertest
> 3/5 Test #3: messageviewer-rendertest ...............***Failed 0.99 sec
> Start 4: messageviewer-unencryptedmessagetest
> 4/5 Test #4: messageviewer-unencryptedmessagetest ... Passed 2.79 sec
> Start 5: accessiblemailwebviewtest
> 5/5 Test #5: accessiblemailwebviewtest .............. Passed 0.41 sec
>
> 80% tests passed, 1 tests failed out of 5
>
> Total Test time (real) = 5.36 sec
>
> The following tests FAILED:
> 3 - messageviewer-rendertest (Failed)
>
>
> one problem is still occures: gpgsm will open a pinentry window that has to be closed via mouse click.
>
>
> Thanks,
>
> Sandro Knauß
>
>
_______________________________________________
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