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

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Improve parsing of Content-ID header
From:       "Thomas McGuire" <mcguire () kde ! org>
Date:       2010-01-04 14:42:38
Message-ID: 20100104144238.2330.52984 () localhost
[Download RAW message or body]


-----------------------------------------------------------
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@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@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]