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


Thanks for working on it. A big patch, lots of comment. :)


/trunk/KDE/kdepim/kmail/kmfilterdlg.h
<http://svn.reviewboard.kde.org/r/6061/#comment9975>

    Why is this include needed?



/trunk/KDE/kdepim/kmail/kmfilterdlg.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment9974>

    Won't the mFilterList->slotApplyFilterChanges be called twice in this case, once because of buttonClicked(KDialog::Apply) signal and once because of the clicked() signal of the Apply button (slotapply calls mFilterList->slotApplyFilterChanges).



/trunk/KDE/kdepim/kmail/kmfilterdlg.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment9971>

    You have some slots from the above that do nothing else but call other slots. You should just connect to the other slots directly. And in that case one connect can go away (for widgetRemoved()).



/trunk/KDE/kdepim/kmail/kmfilterdlg.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment9972>

    This slot is not needed.



/trunk/KDE/kdepim/kmail/kmfilterdlg.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment9973>

    This isn't needed either.



/trunk/KDE/kdepim/kmail/kmfilterdlg.cpp
<http://svn.reviewboard.kde.org/r/6061/#comment9976>

    I'm undecided about this solution, as the signals are still emitted and slotUpdateDialog is still called. Of course without benchmarking I can't tell which is faster: doing some disconnects here and connects at the end of the function or using this guarding flag.



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

    why is it needed?



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

    use qobject_cast<FilterActionWidget*>(parent) or dynamic_cast instead. Same below.
    But in general I don't like that the FilterAction is now dependent on the FilterActionWidget. it is a circular dependency.
    A solution would be a FilterAction::changed signal, but FilterAction is not a QObject. I don't mind making it a QObject to have a cleaner code, unless somebody else complains.



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

    Please remove this.



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

    You can connect directly to the patternChanged signal


- Andras


On 2010-12-06 14:56:35, George  Metaxas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6061/
> -----------------------------------------------------------
> 
> (Updated 2010-12-06 14:56:35)
> 
> 
> 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.h 1204151 
>   /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp 1204151 
>   /trunk/KDE/kdepim/mailcommon/filteraction.cpp 1204151 
>   /trunk/KDE/kdepim/mailcommon/filteractionwidget.h 1204151 
>   /trunk/KDE/kdepim/mailcommon/filteractionwidget.cpp 1204151 
>   /trunk/KDE/kdepim/mailcommon/searchpatternedit.h 1204151 
>   /trunk/KDE/kdepim/mailcommon/searchpatternedit.cpp 1204151 
>   /trunk/KDE/kdepim/mailcommon/soundtestwidget.h 1204151 
>   /trunk/KDE/kdepim/mailcommon/soundtestwidget.cpp 1204151 
> 
> 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/