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

[Torgny Nyblom]

> On 2010-01-04 14:42:44, Thomas McGuire wrote:
> > branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.cpp, line 1932
> > <http://reviewboard.kde.org/r/2483/diff/2/?file=16370#file16370line1932>
> >
> >     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?

1: Overloaded the identifier() function. It strips the extra '@' if it's found.
2 & 3: Removed this overload as you are as usual correct :)
 


> On 2010-01-04 14:42:44, Thomas McGuire wrote:
> > branches/work/akonadi-ports/kdepimlibs/kmime/kmime_headers.cpp, line 1951
> > <http://reviewboard.kde.org/r/2483/diff/2/?file=16370#file16370line1951>
> >
> >     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.

Well duplicated and duplicated... I think most parse() functions share mostly the same head and tail parts. This one is no different.


> On 2010-01-04 14:42:44, Thomas McGuire wrote:
> > branches/work/akonadi-ports/kdepimlibs/kmime/tests/kmime_message_test.cpp, line 389
> > <http://reviewboard.kde.org/r/2483/diff/2/?file=16373#file16373line389>
> >
> >     Some of this QTextEdit-generated HTML junk can be removed, so that the test is clearer.

Gone


> On 2010-01-04 14:42:44, Thomas McGuire wrote:
> > branches/work/akonadi-ports/kdepimlibs/kmime/tests/kmime_message_test.cpp, line 418
> > <http://reviewboard.kde.org/r/2483/diff/2/?file=16373#file16373line418>
> >
> >     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?

Replaced as indicated above.


- Torgny


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


On 2010-01-04 18:48:28, Torgny Nyblom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2483/
> -----------------------------------------------------------
> 
> (Updated 2010-01-04 18:48:28)
> 
> 
> 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/