'Re: [Kde-pim] Review Request: Fix/improve handling of Headers and'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Fix/improve handling of Headers and
From:       "Andras Mantia" <amantia () kde ! org>
Date:       2009-07-29 9:26:50
Message-ID: 20090729092650.19795.75643 () localhost
[Download RAW message or body]

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

/trunk/KDE/kdepimlibs/kmime/kmime_content.cpp
<http://reviewboard.kde.org/r/1151/#comment1193>

    That's the part I can really comment: this changes the behavior back, so calling \
assemble() breaks the message integrity. In Akonadi assemble() is used to retrieve \
the message from the payload, the reason was I think that theoretically you could \
store a message constructed from parts (and not with setContent) in the database. If \
you don't call assemble when retrieving, those messages are not "complete". Calling \
always assemble() is a way to go, but that breaks the message integrity without the \
extra checks I added.  An idea would be to have a dirty flag (set if a header was \
modified/added/removed programatically) and than we can check in payloadData() if \
there is a need to call assemble() or not.  As it is in your patch, I think it is not \
good.

/trunk/KDE/kdepimlibs/kmime/kmime_content.cpp
<http://reviewboard.kde.org/r/1151/#comment1194>

    What happened with this code? Are you 100% sure it is not needed?

/trunk/KDE/kdepimlibs/kmime/kmime_content.cpp
<http://reviewboard.kde.org/r/1151/#comment1195>

    Sincerely, I don't like using macro functions, I'd rather keep the old way of \
having one distinct function per header. I know it is used in other places in KMime, \
so take it just as an opinion.

- Andras

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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 