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

[Thomas McGuire]

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


Some minor stuff again, mostly cosmetics + the things we discussed on IRC.


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

    kdepimlibs includes should come before kdelibs includes, so the kmime_message.h include and the collection.h include should be moved



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

    Please don't make style changes to unrelated code parts in the future, that makes reviewing the patch harder.



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

    make "line" const here.



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

    Should this be && instead of &?



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

    Leave away the "== true" here, I think the coding style guidelines say that.



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

    What happens if the user selects an empty file as import source? Will it crash here?
    (Granted, the old code does the same...)
    
    Also, percentage can be const.



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

    Fix the indentation here.



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

    This code duplication here should be removed somehow, the entire code block is the same as above



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

    Minor coding style thing: add a space inside "(("
    And make it const



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

    You should use i18np() to handle plural cases correctly, so that the text is also correct when only importing 1 message.
    
    And add some spaces for the coding style :) (you'll get used to it)



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

    The parameter should be const and passed by reference.
    
    And add spaces.



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

    Pass baseCollection by reference here, it is slightly faster.



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

    Remove the trailing whitespace here



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

    coding style: More spaces! :)
    And a newline between the two functions.



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

    again, subCollection should be const and a reference.


- Thomas


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