[Kde-pim] Review Request: new kmail filter : add to address book

[Thomas McGuire]

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


Thanks for picking up this old patch!
I admit I haven't actually tried it yet, but I found some nitpicks, can you have a look at them and the resubmit it?


/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1044>

    Please use CamelCase headers.
    But is this actually needed?



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1054>

    updateResourceMaps() would be a better name.



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1045>

    Inconsistent: FooString and BarStr



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1046>

    Capitalization: To->to



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1055>

    "Default" instead of "default"?
    Also, "From" should have context as well.



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1047>

    That is not a nice name for a category, should be something in proper English.



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1057>

    Shouldn't the maps be cleared here, so that there are no duplicate resources when calling this multiple times?



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1049>

    list can be const



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1048>

    missing consts
    Easiest to make this a foreach() with consts.



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1056>

    const



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1058>

    I would prefer foreach() for better readability.



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1065>

    Shouldn't this return ErrorButGoOn or whatever the code for that is?



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1059>

    This looks messy? Why not use a grid layout instead of all there complicated nested layouts?



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1062>

    in resource->in addressbook



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1061>

    preferred resource->preferred addressbook



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1060>

    instead of <default>, make that "the default addressbook."



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1051>

    No endl needed. no function name in the kDebug() needed either, automatically added by kDebug().



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1064>

    Again, kmail-autoadd is not a nice category name.
    Please also centralize this, so that the string "kmail-autoadd" doesn't appear twice in the code



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1053>

    this will crash if argsStr doesn't have that many components.



/trunk/KDE/kdepim/kmail/kmfilteraction.cpp
<http://reviewboard.kde.org/r/1038/#comment1052>

    "" -> QString()


- Thomas


On 2009-07-16 00:54:18, Bruno Bigras wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1038/
> -----------------------------------------------------------
> 
> (Updated 2009-07-16 00:54:18)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This is a port of a commit by Christian Schaarschmidt https://bugs.kde.org/show_bug.cgi?id=47333#c19 from the kdepim-3.5.5+ branch.
> 
> It add an "add to address book" filter action.
> 
> I tried to make the ui look better when the window is not maximized (see the two screenshots) but the first combo box (the one to select "From", "To", "CC", "BCC") don't look so good in the center.
> 
> 
> This addresses bug 47333.
>     https://bugs.kde.org/show_bug.cgi?id=47333
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/kmfilteraction.cpp 997552 
> 
> Diff: http://reviewboard.kde.org/r/1038/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> before
>   http://reviewboard.kde.org/r/1038/s/143/
> after
>   http://reviewboard.kde.org/r/1038/s/144/
> 
> 
> Thanks,
> 
> Bruno
> 
>

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