[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-22 23:38:43
Message-ID: 20090722233843.16188.37806 () localhost
[Download RAW message or body]
-----------------------------------------------------------
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@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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic