[Kde-pim] Review Request: Review for the filter changes (already in master), for inclusion to 4.9 branch

Thu Jul 5 11:46:28 BST 2012

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


kmail/kmcommands.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12005>

    maybe
    mRequiredParts( requiredPart )
    for consistency?


kmail/kmcommands.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12006>

    maybe
    mRequiredPart( requiredPart )
    for consistency


mailcommon/filter/filteraction.h
<http://git.reviewboard.kde.org/r/105445/#comment12012>

    see comment on searchpattern.h


mailcommon/filter/filteractionaddtoaddressbook.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12011>

    trailing whitespace


mailcommon/filter/filteractioncopy.h
<http://git.reviewboard.kde.org/r/105445/#comment12015>

    does this still exist?


mailcommon/filter/filteractioncopy.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12013>

    just remove?


mailcommon/filter/filteractioncopy.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12014>

    remove?


mailcommon/filter/filteractionmove.h
<http://git.reviewboard.kde.org/r/105445/#comment12017>

    unnecessary change?


mailcommon/filter/filteractionplaysound.h
<http://git.reviewboard.kde.org/r/105445/#comment12018>

    unnecessary change?


mailcommon/filter/filteractionplaysound.h
<http://git.reviewboard.kde.org/r/105445/#comment12019>

    does this still exist


mailcommon/filter/filteractionstatus.h
<http://git.reviewboard.kde.org/r/105445/#comment12020>

    unnecessary change?


mailcommon/filter/filteractionstatus.h
<http://git.reviewboard.kde.org/r/105445/#comment12021>

    does this still exist


mailcommon/filter/filteractionunsetstatus.h
<http://git.reviewboard.kde.org/r/105445/#comment12022>

    trailing whitespace


mailcommon/filter/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12023>

    maybe also rename to requiredPart for consistency?


mailcommon/filter/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12024>

    see above


mailcommon/filter/itemcontext.h
<http://git.reviewboard.kde.org/r/105445/#comment12025>

    requiredPart?


mailcommon/filter/itemcontext.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12026>

    that doesn't look correct.
    what if mRequestedPart == Header?


mailcommon/filter/kmfilterdialog.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12027>

    rename to requiredPart?


mailcommon/filter/kmfilterdialog.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12028>

    not sure about the coding style, but it looks like & should be with the argument name, not with the type


mailcommon/filter/mailfilter.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12029>

    static_cast


mailcommon/searchpattern.h
<http://git.reviewboard.kde.org/r/105445/#comment12010>

    Looking through the subclass implementations, it seems that a suitable default implementation could return Envelope


mailcommon/searchpattern.h
<http://git.reviewboard.kde.org/r/105445/#comment12007>

    trailing whitespace


mailcommon/searchpattern.h
<http://git.reviewboard.kde.org/r/105445/#comment12008>

    trailing whitespace


mailcommon/searchpattern.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12003>

    not sure about coding style in KMail, but I think even single line bodies should have block brackets


mailcommon/searchpattern.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12002>

    else if?


mailfilteragent/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12030>

    requiredPart?


mailfilteragent/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12031>

    requiredPart?


mailfilteragent/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12032>

    requiredPart?


mailfilteragent/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12033>

    requiredPart?


mailfilteragent/filtermanager.h
<http://git.reviewboard.kde.org/r/105445/#comment12034>

    requiredPart?


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12035>

    trailing whitespace


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12036>

    kDebug?


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12037>

    trailing whitespace


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12038>

    requiredPart?


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12039>

    requiredPart?
    
    * goes with the argument name, not with the type


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12040>

    since context doesn't need payload store, shouldn't this always be true?


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12041>

    Item &item
    requiredPart?
    QString &accountId


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12042>

    trailing whitespace


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12043>

    requiredPart?


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12044>

    requiredPart?


mailfilteragent/filtermanager.cpp
<http://git.reviewboard.kde.org/r/105445/#comment12045>

    trailing whitespace


- Kevin Krammer


On July 4, 2012, 9:15 p.m., Andras Mantia wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105445/
> -----------------------------------------------------------
> 
> (Updated July 4, 2012, 9:15 p.m.)
> 
> 
> Review request for KDEPIM and Volker Krause.
> 
> 
> Description
> -------
> 
> Fixes the mail loss bug in filtering (by removing the setCacheOnly(true) from the change recorder) and rewrites filter handling, so only the needed payload is downloaded. Adds a check to avoid changing an item with incomplete payload.
> Touches lots of files because of the fine-tuned payload downloading, that needed internal API changes. 
> 
> 
> Diffs
> -----
> 
>   kmail/kmcommands.h 6dbc2a6 
>   kmail/kmcommands.cpp b6edc27 
>   kmail/kmmainwidget.cpp 3688e55 
>   mailcommon/filter/filteraction.h cd6e18c 
>   mailcommon/filter/filteraction.cpp b4a348a 
>   mailcommon/filter/filteractionaddheader.h 4690ca3 
>   mailcommon/filter/filteractionaddheader.cpp 53f4c2c 
>   mailcommon/filter/filteractionaddtag.h 7b75a8f 
>   mailcommon/filter/filteractionaddtag.cpp 55114c9 
>   mailcommon/filter/filteractionaddtoaddressbook.h 61f793f 
>   mailcommon/filter/filteractionaddtoaddressbook.cpp f2d56f6 
>   mailcommon/filter/filteractionbeep.h 5dcf5cc 
>   mailcommon/filter/filteractionbeep.cpp 0c76ac2 
>   mailcommon/filter/filteractioncopy.h 4291a97 
>   mailcommon/filter/filteractioncopy.cpp 699b9ce 
>   mailcommon/filter/filteractiondelete.h 76881cf 
>   mailcommon/filter/filteractiondelete.cpp e11978e 
>   mailcommon/filter/filteractionexec.h 5436fb0 
>   mailcommon/filter/filteractionexec.cpp d3d9653 
>   mailcommon/filter/filteractionforward.h 0051a68 
>   mailcommon/filter/filteractionforward.cpp 4da32ff 
>   mailcommon/filter/filteractionmove.h 22f4541 
>   mailcommon/filter/filteractionmove.cpp 3f7d715 
>   mailcommon/filter/filteractionpipethrough.h 5135fa9 
>   mailcommon/filter/filteractionpipethrough.cpp 0313a20 
>   mailcommon/filter/filteractionplaysound.h 6e12a83 
>   mailcommon/filter/filteractionplaysound.cpp f3ad74c 
>   mailcommon/filter/filteractionredirect.h 48a823c 
>   mailcommon/filter/filteractionredirect.cpp 5f6f586 
>   mailcommon/filter/filteractionremoveheader.h 77365fc 
>   mailcommon/filter/filteractionremoveheader.cpp 90315ba 
>   mailcommon/filter/filteractionreplyto.h f8655d9 
>   mailcommon/filter/filteractionreplyto.cpp 97502bd 
>   mailcommon/filter/filteractionrewriteheader.h 825e82d 
>   mailcommon/filter/filteractionrewriteheader.cpp 6d7c3e4 
>   mailcommon/filter/filteractionsendfakedisposition.h be25071 
>   mailcommon/filter/filteractionsendfakedisposition.cpp 4225f57 
>   mailcommon/filter/filteractionsendreceipt.h bab1294 
>   mailcommon/filter/filteractionsendreceipt.cpp 3728555 
>   mailcommon/filter/filteractionsetidentity.h c054a1e 
>   mailcommon/filter/filteractionsetidentity.cpp 8405964 
>   mailcommon/filter/filteractionsetstatus.h 53f64ba 
>   mailcommon/filter/filteractionsetstatus.cpp 3da6102 
>   mailcommon/filter/filteractionsettransport.h 5c8b063 
>   mailcommon/filter/filteractionsettransport.cpp 601db46 
>   mailcommon/filter/filteractionstatus.h 7867235 
>   mailcommon/filter/filteractionstatus.cpp f99b28a 
>   mailcommon/filter/filteractionunsetstatus.h df5651d 
>   mailcommon/filter/filteractionunsetstatus.cpp 7413aa0 
>   mailcommon/filter/filterimporter/filterimporterevolution.cpp ca996ab 
>   mailcommon/filter/filterimporter/filterimporterprocmail.cpp 67055b0 
>   mailcommon/filter/filterimporter/filterimportersylpheed.cpp d23e14f 
>   mailcommon/filter/filtermanager.h 4f069dd 
>   mailcommon/filter/filtermanager.cpp 0f4101b 
>   mailcommon/filter/itemcontext.h bb4f7cc 
>   mailcommon/filter/itemcontext.cpp 43df66a 
>   mailcommon/filter/kmfilterdialog.h 3247cb4 
>   mailcommon/filter/kmfilterdialog.cpp 20b8d91 
>   mailcommon/filter/mailfilter.h 1444393 
>   mailcommon/filter/mailfilter.cpp 594c38b 
>   mailcommon/searchpattern.h dcda954 
>   mailcommon/searchpattern.cpp e50f253 
>   mailcommon/tests/searchpatterntest.cpp 703a572 
>   mailfilteragent/filtermanager.h 4b65af0 
>   mailfilteragent/filtermanager.cpp 142d978 
>   mailfilteragent/mailfilteragent.h d44f068 
>   mailfilteragent/mailfilteragent.cpp 13bd323 
> 
> Diff: http://git.reviewboard.kde.org/r/105445/diff/
> 
> 
> Testing
> -------
> 
> Works for me. :)
> 
> 
> Thanks,
> 
> Andras Mantia
> 
>

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