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

[Andras Mantia]

> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/mailcommon/filteraction.cpp, line 222
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42315#file42315line222>
> >
> >     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.
> 
> George  Metaxas wrote:
>     Well a FilterAction::changed signal would have been the best solution, but I thought that making FilterAction and its subclasses into a QObject would be a big change. This solution may introduce a circular dependency, but it's more of a soft dependency. If the caller is not a FilterActionWidget, then the only thing that will not work is the value update notification. I've seen that the filteraction.h file is being included in mobile/mail/mobilekernel.cpp, so I don't know if making it into a QObject will break anything.

If you have an #include, it is not soft dependency anymore... I really don't like this solution, and I prefer to have a signal instead. The only thing that will add that is a few KB per filter object. Yes, on mobile this might (slightly) matter, but I like clean code better. :) Or there is even another solution: make the createParamWidget return an specialized QWidget, eg. FilterParamWidget that has a changed signal. Then you connect the signals from the lineedit, combobox, etc. created *inside* this paramWidget to the signal of the paramWidget. The caller of the createParamWidget() than can use the signal of the returned widget.


- Andras


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


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/