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

George Metaxas gmetal31 at gmail.com
Mon Dec 6 21:08:05 GMT 2010 


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/kmail/kmfilterdlg.h, line 37
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42313#file42313line37>
> >
> >     Why is this include needed?

I don't know how this line was added to the code. I was under the impression that it was merged after an SVN update, but it seems I am wrong. Same goes for another boost include as you pointed out. I will remove both.


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp, line 370
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42314#file42314line370>
> >
> >     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()).

I created the slots (widgetAdded and widgetRemoved) and "implemented" them separately, in case they are required in the future. I will remove them, as they are of no use presently. 


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp, line 400
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42314#file42314line400>
> >
> >     This slot is not needed.

OK


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp, line 406
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42314#file42314line406>
> >
> >     This isn't needed either.

OK


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/mailcommon/filteraction.cpp, line 55
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42315#file42315line55>
> >
> >     why is it needed?

Not needed. Will remove it.


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp, line 429
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42314#file42314line429>
> >
> >     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.

The flag is currently only used when the dialog is initialising. At first I thought that I may have to disconnect all signals upon selecting a new filter, so this variable would indeed be much more useful. I could remove the variable and just connect the signals that notify of a value change at the end of the KMFilterDlg constructor, but I don't really like that solution.


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/mailcommon/filteraction.cpp, line 2251
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42315#file42315line2251>
> >
> >     Please remove this.

OK.


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/mailcommon/searchpatternedit.cpp, line 484
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42319#file42319line484>
> >
> >     You can connect directly to the patternChanged signal

OK.


> 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.

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.


> On 2010-12-06 18:14:30, Andras Mantia wrote:
> > /trunk/KDE/kdepim/kmail/kmfilterdlg.cpp, line 345
> > <http://svn.reviewboard.kde.org/r/6061/diff/1/?file=42314#file42314line345>
> >
> >     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).

Yup. I missed that. Will fix it.


- George


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

More information about the kde-pim mailing list