[Kde-pim] Review Request: kmail - please add ability to display and sort by date received

[Thomas McGuire]

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


Hi Eric,

sorry for the late reply, I have been too busy with university.

I think the patch and the rationale behind it are fine, after reading your explanation and thinking about it a bit more. The good thing about the patch is that it doesn't affect users by default, it has to explicitly enabled in the UI and is therefore not intrusive. So after making some improvements to the patch, I would add it to KMail and KMime.

The quality of the patch is also good, well done.

The KMime part of the patch needs some improvements, code in public libraries like KMime should have high standards. I think the biggest problem is that the code in some places assumes that there is only one Received header, although in reality mails have multiple Received headers. The other thing that needs some attention is the parsing code, it should be a real parse(), and at the same time be based on Structured instead of Unstructured.
To make sure that parsing and assembling of mails with Received header_s_ works, please add some unit tests for that to KMime. You need to enable KDE4_BUILD_TESTS in CMake for that.

If you have any questions about all that, please contact, I shall answer faster the next time. Awaiting your next patch now! :)

I added some comments inline below as well, please look at them.


trunk/KDE/kdepimlibs/kmime/kmime_headers.h
<http://reviewboard.kde.org/r/4337/#comment6108>

    @since 4.6 missing



trunk/KDE/kdepimlibs/kmime/kmime_headers.h
<http://reviewboard.kde.org/r/4337/#comment6107>

    Shouldn't this inherit Generics::Structured instead? The received field has a well defined grammar, see RFC2822, section 3.6.7. "Trace fields".



trunk/KDE/kdepimlibs/kmime/kmime_headers.h
<http://reviewboard.kde.org/r/4337/#comment6111>

    Some important method in this class that should be overridden from the base class are missing, for example as7BitString().
    
    This means adding a Received header to a KMime::Message object and then later assembling the message would lose the header, for example.



trunk/KDE/kdepimlibs/kmime/kmime_headers.h
<http://reviewboard.kde.org/r/4337/#comment6106>

    remove this empty line



trunk/KDE/kdepimlibs/kmime/kmime_headers.cpp
<http://reviewboard.kde.org/r/4337/#comment6110>

    Please use real parsing here, instead of stuff like section().
    See the parse() method of other subclasses of Structured for an example.



trunk/KDE/kdepimlibs/kmime/kmime_headers_p.h
<http://reviewboard.kde.org/r/4337/#comment6105>

    You can remove this line



trunk/KDE/kdepimlibs/kmime/kmime_message.h
<http://reviewboard.kde.org/r/4337/#comment6104>

    Add a "@since 4.6" please



trunk/KDE/kdepimlibs/kmime/kmime_message.h
<http://reviewboard.kde.org/r/4337/#comment6109>

    This says it returns "the" received headers. But mails have multiple of those headers, what does it do in this case?


- Thomas


On 2010-06-16 01:03:47, Eric Sanford wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4337/
> -----------------------------------------------------------
> 
> (Updated 2010-06-16 01:03:47)
> 
> 
> Review request for KDE PIM and Thomas McGuire.
> 
> 
> Summary
> -------
> 
> KMail should have the ability to extract the date from the Received header field, display the date received and sort by date received. A significant number of emails have incorrect or missing Date header fields. When sorting by Date in KMail, these drop to the bottom as Unknown or appear at the top if the Date header field contains a future date. This is a basic patch to extract the date from the Received header field, add a Date Received column in KMail classic theme, and sort by Date Received in classic theme. This is industry-standard for comparable email applications.
> 
> 
> This addresses bug 227942.
>     https://bugs.kde.org/show_bug.cgi?id=227942
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepim/messagelist/core/item.h 1137450 
>   trunk/KDE/kdepim/messagelist/core/item.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/core/item_p.h 1137450 
>   trunk/KDE/kdepim/messagelist/core/manager.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/core/model.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/core/sortorder.h 1137450 
>   trunk/KDE/kdepim/messagelist/core/sortorder.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/core/theme.h 1137450 
>   trunk/KDE/kdepim/messagelist/core/theme.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/core/themedelegate.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/core/view.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/storagemodel.cpp 1137450 
>   trunk/KDE/kdepim/messagelist/utils/themeeditor.cpp 1137450 
>   trunk/KDE/kdepim/nepomuk_email_feeder/messageanalyzer.cpp 1137450 
>   trunk/KDE/kdepimlibs/kmime/kmime_headers.h 1137426 
>   trunk/KDE/kdepimlibs/kmime/kmime_headers.cpp 1137426 
>   trunk/KDE/kdepimlibs/kmime/kmime_headers_p.h 1137426 
>   trunk/KDE/kdepimlibs/kmime/kmime_message.h 1137426 
>   trunk/KDE/kdepimlibs/kmime/kmime_message.cpp 1137426 
> 
> Diff: http://reviewboard.kde.org/r/4337/diff
> 
> 
> Testing
> -------
> 
> This patch has been tested on openSUSE 11.2 and openSUSE 11.3 Milestone 7. The attached patch is currently being tested on openSUSE 11.3 Milestone 7, with KDE executables and libraries built from trunk/KDE (revision 1137426) source code for kdelibs, kdepimlibs, kdebase and kdepim, and trunk/kdesupport source code for attica, soprano, polkit-qt, and akonadi.
> 
> 
> Thanks,
> 
> Eric
> 
>

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