[Kde-pim] Review Request: Improve parsing of Content-ID header

[Thomas McGuire]

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


Thanks for finding the problem with the inline image support!
I think however that as7BitString() here is wrong, so the comments below. If KMail wants the identifier without the angle brackets, it should use SingleIdent::identifier().

Btw, these are just assumptions, I'm not 100% sure.

Oh, and as always, some of the coding style in the tests could use more spaces at the inside of parenthesis :)


branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.cpp
<http://reviewboard.kde.org/r/2483/#comment2827>

    Some question about this function:
    
    1. Isn't this the same as SingleIdent::identifier(), which returns the identifier without angle brackets?
    
    2. AFAIK as7BitString() is used when assembling a message. Won't the assembled message have a wrong content-id then, because it is missing the angle brackets? Or am I missing something here?
    
    3. This looks like code duplication from Ident::as7BitString(). If the method here is really needed, can you refactor this so that both as7BitString() methods just call another helper method that does the work?



branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.cpp
<http://reviewboard.kde.org/r/2483/#comment2830>

    This function looks complex. Is the code duplicated somewhere?
    
    If so, it would be nice if it can be avoided.
    
    If not, also ok, we have a unit test for this now, so that's fine.



branches/work/akonadi-ports/kdepimlibs/kmime/tests/kmime_message_test.cpp
<http://reviewboard.kde.org/r/2483/#comment2828>

    Some of this QTextEdit-generated HTML junk can be removed, so that the test is clearer.



branches/work/akonadi-ports/kdepimlibs/kmime/tests/kmime_message_test.cpp
<http://reviewboard.kde.org/r/2483/#comment2829>

    As said above, I think as7BitString() needs to include the angle brackets, as otherwise the assembled message is wrong. But I'm not sure, this would need another test.
    
    Can SingleIdent::identifier() be used here in this test instead?


- Thomas


On 2010-01-04 06:30:34, Torgny Nyblom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2483/
> -----------------------------------------------------------
> 
> (Updated 2010-01-04 06:30:34)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Add the parse() and to7BitString() functions to ContentID class.
> The reason being that altho Content-Id should obey they same rules as Message-Id (localpart at hostpart) in practice it doesn't.
> 
> The added parse() function tries to use the same algorithm as is used for MessageID but if that fails a debug message is issued and a more relaxed algorithm is used (parseAtom, might not be the best alternative but it works for the test email I've got).
> 
> The added as7BitString() function is also almost the same as in the base class. The difference being that the enclosing '<' and '>' are not returned and if the relaxed parse() function was used to extract the header the domain part is not present and in that case the trailing '@' is discarded before returning,
> 
> 
> Diffs
> -----
> 
>   branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.h 1068282 
>   branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.cpp 1068282 
>   branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers_p.h 1068282 
>   branches/work/akonadi-ports/kdepimlibs/kmime/tests/kmime_message_test.h 1068282 
>   branches/work/akonadi-ports/kdepimlibs/kmime/tests/kmime_message_test.cpp 1068282 
> 
> Diff: http://reviewboard.kde.org/r/2483/diff
> 
> 
> Testing
> -------
> 
> Old unittest works, new unittest for the added functions work.
> KMail seem to get the expected values back.
> 
> 
> Thanks,
> 
> Torgny
> 
>

_______________________________________________
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/