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


Thanks for cleaning this up, looks very good now.
I have made some minor remarks below.

Apart from the minor remarks, there is a bigger issue though: This new filter action makes the widgets of other filter actions bigger as well, since they are all contained in a QStackedWidget.
This can be best seen when using the "Mark As" filter action, the combobox is huge.

This needs a solution before it can be committed. I'm no expert with layouts, does anybody else have an idea how this can be solved? The QStackedWidget should somehow change size dynamically, and not just take the size of the biggest widget: When using "Mark As" it should use less vertical space than when using "Add to addressbook".

Sorry again for the late review; I'm afraid I won't have time to review your other patch today, too late at night for that :(


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

    space between list and ) please



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

    Now there is no reason to name the variable "it" anymore.



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

    Maybe a better object name, given that there are 2 comboboxes?



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

    Change this to:
    gridlayout->addWidget( cb, 0, 0, 2, 1, Qt::AlignVCenter );
    
    Then the combobox will even appear in the center.



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

    Don't re-use the same variable twice, create a new one with a new name.


- Thomas


On 2009-07-17 03:25:30, Bruno Bigras wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1038/
> -----------------------------------------------------------
> 
> (Updated 2009-07-17 03:25:30)
> 
> 
> 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 998013 
> 
> 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/
> 
> 
> 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/