[Kde-pim] Re: Review Request: Fix the Apply button behaviour in the Configure Filters dialog (Bug 123548)

[Andras Mantia]

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


In general it is ok, but not Ship It yet, see below.
One common issue is the style guide not being followed, mainly spaces around parentheses. I'm somewhat immune to such issues, but the style guide policy in form of Thomas and nowadays Tobias will come after you.See http://pim.kde.org/development/coding-korganizer.php (don't be worried, it is valid for the whole pim).


/trunk/KDE/kdepim/mailcommon/filteraction.h
<http://svn.reviewboard.kde.org/r/6061/#comment10009>

    You don't (should not) have to declare inherited signals again. Same for the other cases.



/trunk/KDE/kdepim/mailcommon/filteraction.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment10010>

    Instead of doing this, please add as a last argument to the FilterAction (and its derivates) constructor a QObject* parent = 0 and use QObject(parent). 
    If it is a QObject, lets make it a correct one.



/trunk/KDE/kdepim/mailcommon/filteraction.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment10011>

    No need for QObject:: we are now in a QObject. :)


- Andras


On 2010-12-07 21:57:30, George  Metaxas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6061/
> -----------------------------------------------------------
> 
> (Updated 2010-12-07 21:57:30)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> Currently, the Apply button in the Configure Dialog is always enabled. This is because the button was simply added to the dialog and left unconnected to the dialog's logic. This patch implements the behaviour of the Apply button, by enabling it when a value in the dialog has changed. Apart from connecting an appropriate signal of each widget in the dialog to the Apply button logic, some signal/slot implementation was required in various classes of the MailCommon namespace, to enable the notification of a value change in some of the provided widgets (e.g. SoundTestWidget, SearchPatternEdit) or classes (e.g. FilterAction and subclasses).
> 
> 
> This addresses bug 123548.
>     https://bugs.kde.org/show_bug.cgi?id=123548
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp 1204467 
>   /trunk/KDE/kdepim/mailcommon/filteraction.h 1204467 
>   /trunk/KDE/kdepim/kmail/kmfilterdlg.h 1204467 
>   /trunk/KDE/kdepim/mailcommon/filteraction.cpp 1204467 
>   /trunk/KDE/kdepim/mailcommon/filteractionwidget.h 1204467 
>   /trunk/KDE/kdepim/mailcommon/filteractionwidget.cpp 1204467 
>   /trunk/KDE/kdepim/mailcommon/searchpatternedit.h 1204467 
>   /trunk/KDE/kdepim/mailcommon/searchpatternedit.cpp 1204467 
>   /trunk/KDE/kdepim/mailcommon/soundtestwidget.h 1204467 
>   /trunk/KDE/kdepim/mailcommon/soundtestwidget.cpp 1204467 
> 
> Diff: http://svn.reviewboard.kde.org/r/6061/diff
> 
> 
> Testing
> -------
> 
> Tested the dialog by adding/removing/changing filters, altering filter order, adding/removing/altering filter actions, adding/removing/altering filter criteria.
> 
> 
> Thanks,
> 
> George
> 
>

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