[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: new kmail filter : add to address book
From:       "Thomas McGuire" <mcguire () kde ! org>
Date:       2009-07-28 8:35:25
Message-ID: 20090728083525.18602.42649 () localhost
[Download RAW message or body]


-----------------------------------------------------------
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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/


[prev in list] [next in list] [prev in thread] [next in thread]