[Kde-pim] Review Request: Fix/improve handling of Headers and Contents in KMime (long)
Constantin Berzan
exit3219 at gmail.com
Thu Jul 30 13:29:20 BST 2009
> On 2009-07-29 10:41:46, Thomas McGuire wrote:
> > /trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.cpp, line 458
> > <http://reviewboard.kde.org/r/1151/diff/1/?file=9224#file9224line458>
> >
> > Where did those 2 tests go?
I moved them around (in the .h and .cpp file) so that tests are run in a more down-to-up manner (i.e. simpler tests first), and failures are easier to catch. I know, I shouldn't have mixed code changes with aesthetic changes.
> On 2009-07-29 10:41:46, Thomas McGuire wrote:
> > /trunk/KDE/kdepimlibs/kmime/kmime_util_p.h, line 42
> > <http://reviewboard.kde.org/r/1151/diff/1/?file=9219#file9219line42>
> >
> > Hmm, is this binary compatible, is removeHeader exported?
This seems to be OK since that function is not exported.
I found another BC problem though: I removed the protected Content::headerInstance() function. I'll fix that (deprecate instead of remove).
> On 2009-07-29 10:41:46, Thomas McGuire wrote:
> > /trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.cpp, line 357
> > <http://reviewboard.kde.org/r/1151/diff/1/?file=9224#file9224line357>
> >
> > why did you change the test data here, does the old one not work?
> > for example, I think unquoted charset parameter is valid.
This bytearray contains the data that we expect KMime to generate. The test was changed in [1] to test that assemble() doesn't think minor changes (case, quotes) means the header actually changed. Since with this approach assemble() no longer checks old headers, I basically undid that change.
[1] http://websvn.kde.org/trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.cpp?r1=984397&r2=995663
- Constantin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1151/#review1835
-----------------------------------------------------------
On 2009-07-28 13:07:54, Constantin Berzan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1151/
> -----------------------------------------------------------
>
> (Updated 2009-07-28 13:07:54)
>
>
> Review request for KDE PIM, Thomas McGuire, Andras Mantia, and Tom Albers.
>
>
> Summary
> -------
>
> This is a rather huge patch attempting to fix a number of problems in KMime. (You will also find small unrelated changes, some refactoring, and some TODO questions.)
>
> (1) The problem with headers:
> --------------------------------
> Before: Headers were handled in an inconsistent and broken manner.
> Examples:
> * Content-ID and a bunch of others were not supported at all (assemble() didn't know about them).
> * Headers were not parsed from the head in parse(), but only when a particular header was asked for. This meant that after removeHeader(some_header), headerByType(type_of_header_removed) still returned the header which was supposed to be removed.
> * There was a lot of duplication and unnecessary complexity with separate handling of headers in KMime::Content, KMime::Message and KMime::NewsArticle.
> * There wasn't any way of having more than one header of a particular type (e.g. Resent-From:), other than manually playing with the head QByteArray.
>
> After: Headers are handled uniformly in KMime::Content, and no special handling is necessary in Message or NewsArticle. MIME headers (Content-*), RFC5322 headers (From: etc.), and custom headers (X-*) all work. Each header class has a virtual clone() method for making a copy of the given header. A HeaderFactory is used when parsing, so that headers of appropriate type are created. appendHeader() and prependHeader() Content methods allow setting multiple headers of a given type.
>
> (2) The problem with singlepart<->multipart Content conversion:
> ------------------------------------------------------------------
> Before: Consequences of this automatic conversion led to bugs.
> Examples:
> * Say c is single-part. Headers::Base *h = c->contentType(); c->addContent(bla); Now the h pointer no longer belongs to c, but to c->contents().first(), because c was converted to multipart.
> * Say c is multipart/mixed, with 0 sub-contents. c->addContent(alpha); c->addContent(beta); c->removeContent(beta); alpha has just been deleted, because c has been converted to singple-part.
>
> After: In all cases that I can think of, the developer can create a single-part or a multipart content, depending on what he or she wants. The automatic conversion is a source of bugs and confusion, with very little benefit / convenience. But we cannot change this behaviour and stay backwards-compatible, right? :-(
> All I did was put big warnings in the API docs.
>
> (3) The problem with signatures and corrupted content:
> ---------------------------------------------------------
> Before: Calling assemble() on a Message changed it in slight ways (header order, where header folding occurs, Content-[Tt]ype capitalization, etc). This broke signature checking. András has added a fix for this, where assembleHeaders() would not change the head of the Content unless some header has actually been modified. IMHO, this is not the best possible solution. Problems with this are:
> * It's slow: Every header has to be checked in assembleHeaders().
> * It's easy to break: For example, calling c->contentDescription() will silently create an empty Content-Description header in c. It is difficult to handle such empty headers correctly in assembleHeaders(), getHeaderByType(), setHeader(), as well as when the headers have to be moved around in addContent()/removeContent().
> * Ideally, assemble() should not bother with message integrity. Assembling a message means creating it from scratch.
>
> After: I started with this clear picture of KMime::Content: There is a string representation (a big QByteArray holding the head and body of the content), and there is a broken-down representation (a list of headers and a list of subcontents). parse() updates the broken-down representation from the string representation. assemble() updates the string representation from the broken-down representation. This distinction has been inconsistent before (true for subcontents but not true for headers). Making parse() and assemble() do what they are supposed to do may actually break some applications, but I think fixing this will make everything clearer. In particular, before one could do:
> Content *c = new Content; c->setContent(something); c->getSomeHeader();
> Whereas now the header will not be available until parse() is called.
> For signature checking, I have added setFrozen() / isFrozen() methods to KMime::Content. Freezing a content guarantees that parse() and assemble() will never change the string representation returned by encodedContent().
>
> ---
> Thanks for reading. I appreciate any comments or suggestions.
>
>
> Diffs
> -----
>
> /trunk/KDE/kdepimlibs/kmime/CMakeLists.txt 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_codecs.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_content.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_content.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_content_p.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_header_parsing.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_header_parsing.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_headerfactory.h PRE-CREATION
> /trunk/KDE/kdepimlibs/kmime/kmime_headerfactory.cpp PRE-CREATION
> /trunk/KDE/kdepimlibs/kmime/kmime_headers.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_headers.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_message.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_message_p.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_newsarticle.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_util.h 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_util.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/kmime_util_p.h 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/CMakeLists.txt 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/headertest.h 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/headertest.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.h 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_content_test.cpp 1003373
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_headerfactorytest.h PRE-CREATION
> /trunk/KDE/kdepimlibs/kmime/tests/kmime_headerfactorytest.cpp PRE-CREATION
>
> Diff: http://reviewboard.kde.org/r/1151/diff
>
>
> Testing
> -------
>
> I added some unit tests, and the message composer in kdepimlibs seems to have no problems.
>
> It would be great if big users of KMime such as the mailreader and Mailody could try this and point out any potential problems. That is the reason I left plenty of kDebug()s in the code.
>
>
> Thanks,
>
> Constantin
>
>
_______________________________________________
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