[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/#review1816
-----------------------------------------------------------

Ship it!


Looks good overall, please commit.
I've again made some comments, please fix those (either before or after committing).

Thanks!


/trunk/KDE/kdepim/kmail/kmfilterdlg.h
<http://reviewboard.kde.org/r/1038/#comment1177>

    No abbreviations please, use something more descriptive



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

    This grid layout seem to increase the spacing between the different actions, probably need to set the spacing of the layout to 0 here. Or is it the margin?
    Hmm, I think it is actually the margin.



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

    The combobox is aligned on the top, but maybe it looks nicer if it was centered vertically? See you third screenshot, the action selection combobox is in the same line as the first line of the add to addressbook widget, I don't think that is nice.



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

    This line is duplicated a lot, can you move that into a small seperate helper function?
    Same with the other gl->addWidget() line, that adds the actual widget.
    the helper functions should probably also include the delete line.



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

    that comment should be moved above the 2 lines above


- Thomas


On 2009-07-27 10:10:23, Bruno Bigras wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1038/
> -----------------------------------------------------------
> 
> (Updated 2009-07-27 10:10:23)
> 
> 
> 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 1002846 
>   /trunk/KDE/kdepim/kmail/kmfilterdlg.h 1002846 
>   /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp 1002846 
> 
> Diff: http://reviewboard.kde.org/r/1038/diff
> 
> 
> Testing
> -------
> 
> I tested this successfully when receiving emails and when sending to multiples recipients.
> 
> 
> Screenshots
> -----------
> 
> before
>   http://reviewboard.kde.org/r/1038/s/143/
> after
>   http://reviewboard.kde.org/r/1038/s/144/
> after-QGridLayout
>   http://reviewboard.kde.org/r/1038/s/148/
> Proposed solution
>   http://reviewboard.kde.org/r/1038/s/154/
> 
> 
> 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/