[Kde-pim] Review Request: Partial port of KMailCVT to Akonadi.

Sat Jan 16 16:28:52 GMT 2010

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

The basics look good already, but there is the usual heap of details below:

/branches/work/akonadi-ports/kdepim/kmailcvt/filter_mbox.cxx
<http://reviewboard.kde.org/r/2633/#comment3087>

    Minor coding style thing: Use CamelCase includes here, like #include <KMime/Message>, if possible

/branches/work/akonadi-ports/kdepim/kmailcvt/filter_mbox.cxx
<http://reviewboard.kde.org/r/2633/#comment3088>

    This will probably use too much memory when importing a huge mbox files, as all mails are kept in memory.

    I think it is better to import the messages one by one.

/branches/work/akonadi-ports/kdepim/kmailcvt/filter_mbox.cxx
<http://reviewboard.kde.org/r/2633/#comment3089>

    Your code has no handling for status flags (unread, new, todo, important, etc) anymore, it would be nice to keep that.

    Akonadi::Item has a method setFlags() to which you can pass flags.

    You can use the class MessageStatus from libkdepim for this: It has setStatusFromStr() and getStatusFlags(), which looks like it is needed here.

    Once you have a parsed KMime::Message, you can use getHeaderByType() to get the X-Status header.

/branches/work/akonadi-ports/kdepim/kmailcvt/filter_mbox.cxx
<http://reviewboard.kde.org/r/2633/#comment3090>

    You do not need to have pointers to an Akonadi::Collection. Akonadi::Collection is implicitly shared, which means you can and should pass it by value instead.

    This should be changed everywhere in this patch.

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.hxx
<http://reviewboard.kde.org/r/2633/#comment3092>

    This method should be const

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.hxx
<http://reviewboard.kde.org/r/2633/#comment3091>

    Should be m_rootCollection, to follow the naming scheme here.

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3095>

    Some of these paramters can be const it seems, like:

    const Akonadi::Collection &collection
    const KMime::Message::Ptr &message

    (yes, message is passed by reference here, that is always slightly faster than passing by value, even for implicitly shared classes like boost::shared_ptr)

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3094>

    In general, exec() is dangerous, as it starts a sub-eventloop which can have various interesting and surprising side effects.

    I think it is ok in this case, at least for straightforward porting, but keep in mind that it is generally better to use async execution of the jobs, like the code snippets in the API docs describe now.

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3093>

    "because %2" will not work, errorString() it a complete sentence.

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3096>

    I think you also need to set the mimetype of newSubCollection

/branches/work/akonadi-ports/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3097>

    Really? I think you can just use CollectionCreateJob::collection(), which returns the collection created by the job.

/branches/work/akonadi-ports/kdepim/kmailcvt/kmailcvt.cpp
<http://reviewboard.kde.org/r/2633/#comment3098>

    It is "Akonadi" :D

    We even had spelling mistakes in our offical API documentation :)

/branches/work/akonadi-ports/kdepim/kmailcvt/kselfilterpage.cpp
<http://reviewboard.kde.org/r/2633/#comment3099>

    The Akonadi includes should be included before the KDE includes

    The general rule for include order is:
    - Own headers first
    - Then kdepimlibs headers
    - Then kdelibs headers
    - Then qt headers
    - Then system headers

/branches/work/akonadi-ports/kdepim/kmailcvt/kselfilterpage.cpp
<http://reviewboard.kde.org/r/2633/#comment3100>

    Also call setAccessRightsFilter() to only get collections that are writable

- Thomas

On 2010-01-16 15:44:15, Matthew Leach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2633/
> -----------------------------------------------------------
> 
> (Updated 2010-01-16 15:44:15)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This patch will port over the KMailCVT application to use Akonadi instead of DBus calls to KMail. So far only the mbox filter, wizard and Filter class have been ported. I am submitting for review jsut to ensure that I am going about this the right way.
> 
> 
> Diffs
> -----
> 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/CMakeLists.txt 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/filter_mbox.cxx 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/filters.hxx 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/filters.cxx 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/kmailcvt.h 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/kmailcvt.cpp 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/kselfilterpage.cpp 1075418 
>   /branches/work/akonadi-ports/kdepim/kmailcvt/kselfilterpagedlg.ui 1075418 
> 
> Diff: http://reviewboard.kde.org/r/2633/diff
> 
> 
> Testing
> -------
> 
> Compiles and imports mbox files fine.
> 
> 
> Thanks,
> 
> Matthew
> 
>

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