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

[Torgny Nyblom]

> On 2010-01-05 12:41:11, Thomas McGuire wrote:
> > /branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.h, line 988
> > <http://reviewboard.kde.org/r/2483/diff/3/?file=16519#file16519line988>
> >
> >     What is the reason for this, are there actually identifiers ending with a "@"? I don't really understand why this is needed here. If this is needed, can it be done in the base class instead, to avoid the overriding problem (see next)?
> >     
> >     Hmm, looking further it seems that SingleIdent::identifier() calls identifiers().first(), which essentially is Types::AddrSpec.asString(). So I suggest either changing Types::AddrSpec.asString()/addr_spec_as_string() to not include the @ if the domain is empty, or changing SingleIdent::identifier() to not include the @ when the domain is empty. As long as all tests still pass, of course.
> >     
> >     Also, this is dangerous: You have a method with the same name/signature in the base class, and you are overriding it, even though the base class method is _not_ virtual. When you have a pointer to SingleIdent, which is really a ContentID, then calling identifier() through the base class pointer will not call ContentID::identifier(), but SingleIdent::identifier().
> >     
> >     This is very unexpected behavior, and therefore overriding non-virtual methods should not be done, if possible.

As you notices this is because when the relaxed parse is used the domain part is empty.
I was actually first thinking about changing addr_spec_as_string(), but was unsure if it was desired to strip the '@'.

Will do that instead.


- Torgny


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


On 2010-01-05 09:10:07, Torgny Nyblom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2483/
> -----------------------------------------------------------
> 
> (Updated 2010-01-05 09:10:07)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Add the parse() and identifier() functions to ContentID class.
> The reason being that although 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 identifier() function is also almost the same as in the base class (4 lines so not that big duplication). The difference being that 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/