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

Thu Jan 21 15:08:14 GMT 2010

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

Thanks for the patch again. This looks very promising, the Akonadi usage seems correct in all places.
I added tons of comments about little things again. Sorry, I'm known to be a bit pedantic about this. it will surely settle down after you got used to the KDE style more. I hope I'm not discouraging you by the mass of comments, but again, almost all of them were about minor things.

Some bigger things included (I've added that to the comments as well):
- Wrong user-visible texts or i18n
- duplicated code in addMessage() and addMessage_fastImport()
- Handling of the "next" button when no collection is selected
- duplicate checking does not check message ids for existing messages in the current folder
- compiler warnings

I'm ok if you commit this patch now and then work on these points with additional commits, but I'm also ok if you want an additional review here on reviewboard with a new version of the patch. Chose what fits you best.

Great work overall, especially since this is a bigger part of work and you're still a newcomer :)

/trunk/KDE/kdepim/kmailcvt/CMakeLists.txt
<http://reviewboard.kde.org/r/2633/#comment3193>

    This can be removed now, right?

/trunk/KDE/kdepim/kmailcvt/CMakeLists.txt
<http://reviewboard.kde.org/r/2633/#comment3194>

    This is also not needed anymore.

/trunk/KDE/kdepim/kmailcvt/filters.hxx
<http://reviewboard.kde.org/r/2633/#comment3166>

    includes should be camelcase includes, e.g. <Akonadi/Collection>

/trunk/KDE/kdepim/kmailcvt/filters.hxx
<http://reviewboard.kde.org/r/2633/#comment3164>

    parse instead of prase, I guess?

/trunk/KDE/kdepim/kmailcvt/filters.hxx
<http://reviewboard.kde.org/r/2633/#comment3165>

    space missing :)

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3167>

    Move this up to the other includes. And make it <QFile>

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3169>

    const Akonadi::Collection _&_collection

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3188>

    getHeaderByType is deprecated and will produce a compiler warning.
    Use headyByType() instead.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3170>

    const correctness: use const_iterator together with constBegin() and constEnd().

    Also, we usually use "it" for iterators, not "i", so it would be nice to be consistent.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3171>

    Add a space between ) and {
    Also in some other places in the code.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3172>

    folderList should be const.
    Add spaces.
    Use '/' instead of "/", that is slightly faster (and our code checker Krazy will complain otherwise). Same in other places.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3173>

    spaces

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3174>

    remove trailing whitespace.
    Best is to set your edit to remove trailing whitespace automatically when editing.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3181>

    because %1 will not work, errorString() returns a full sentence.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3175>

    trailing whitespace

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3189>

    setParent() is deprecated, use setParentCollection() instead.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3176>

    trailing whitespace

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3180>

    because %1 will not work, errorString() returns a full sentence.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3182>

    remove the two spaces between "&" and "msgStatusFlags". And make spaces at msgPath consistent

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3190>

    please add a Q_UNUSED( msgStatusFlags ); to the body of the function to shut up the compiler warning.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3178>

    i18n() call missing, this will not be translatable.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3179>

    make the iterators const correct

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3195>

    The duplicate message id checking code needs to do more: It needs to check the message ids in the existing messages in the current folder as well.

    That means you need to have an ItemFetchJob to fetch all messages, iterate over them and add their message id to the map.
    The item fetch job should fetch the headers only, not the complete message; that can be set using itemFetchScope().fetchPayloadPart( Akonadi::MessagePart::Header ) on the job.

/trunk/KDE/kdepim/kmailcvt/filters.cxx
<http://reviewboard.kde.org/r/2633/#comment3191>

    Most of the code here is duplicated in addMessage() as well.
    Please refactor this so that there is no code duplication anymore, since code duplication is a maintenance problem in the long term.

/trunk/KDE/kdepim/kmailcvt/kmailcvt.h
<http://reviewboard.kde.org/r/2633/#comment3168>

    Remove trailing whitespace here

/trunk/KDE/kdepim/kmailcvt/kmailcvt.cpp
<http://reviewboard.kde.org/r/2633/#comment3183>

    I would argue kmmessagebox is not a Qt include :)
    And it is also not a camelcase header.

/trunk/KDE/kdepim/kmailcvt/kmailcvt.cpp
<http://reviewboard.kde.org/r/2633/#comment3184>

    Please remove this function entirely, it is no longer needed.
    On the KMail side, the function dbusResetAddMessage() only cleared some maps.
    The maps/hashes kept by KMailCVT don't need to be cleared since it will exit anyway.
    So the whole function is not needed anymore.

/trunk/KDE/kdepim/kmailcvt/kmailcvt.cpp
<http://reviewboard.kde.org/r/2633/#comment3192>

    From a UI point of view, it would be much nicer to disable the next button until a collection is selected. That way, the user doesn't wonder why the button doesn't work.

    Also, consider setting the collection requester to the inbox collection by default, see SpecialMailCollections in kdepimlibs/akonadi/kmime.

/trunk/KDE/kdepim/kmailcvt/kselfilterpage.cpp
<http://reviewboard.kde.org/r/2633/#comment3185>

    Akonadi includes should come before KDE includes and be camelcase includes.

/trunk/KDE/kdepim/kmailcvt/kselfilterpagedlg.ui
<http://reviewboard.kde.org/r/2633/#comment3186>

    "Akonadi collection" is a very technical term and shouldn't be presented to the user.
    Use "folder" instead.

/trunk/KDE/kdepim/kmailcvt/kselfilterpagedlg.ui
<http://reviewboard.kde.org/r/2633/#comment3187>

    Again, please no "Akonadi" and "collection" in user-visible strings

- Thomas

On 2010-01-21 00:06:44, Matthew Leach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2633/
> -----------------------------------------------------------
> 
> (Updated 2010-01-21 00:06:44)
> 
> 
> 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
> -----
> 
>   /trunk/KDE/kdepim/kmailcvt/kmailcvt.h 1077831 
>   /trunk/KDE/kdepim/kmailcvt/kmailcvt.cpp 1077831 
>   /trunk/KDE/kdepim/kmailcvt/kselfilterpage.cpp 1077831 
>   /trunk/KDE/kdepim/kmailcvt/kselfilterpagedlg.ui 1077831 
>   /trunk/KDE/kdepim/kmailcvt/filters.cxx 1077831 
>   /trunk/KDE/kdepim/kmailcvt/filters.hxx 1077831 
>   /trunk/KDE/kdepim/kmailcvt/CMakeLists.txt 1077831 
> 
> 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/