Review Request: Enable Apply button on Lockout configuration dialog

Aaron J. Seigo aseigo at kde.org
Mon Feb 14 02:46:30 CET 2011


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100652/#review1423
-----------------------------------------------------------


a simpler approach might be to just connect each checkbox's toggled(bool) signal to settingsModified. there is a certain prettiness to letting the one method handle it, however. the signal "changed()" should be made more descriptive, however. perhaps configurationUiChanged() or something similar that is very, very clear as to what it is there for :)


plasma/generic/applets/lock_logout/lockout.cpp
<http://git.reviewboard.kde.org/r/100652/#comment1186>

    should only be emitted if m_changed


- Aaron J.


On Feb. 14, 2011, 12:59 a.m., Romário Rios wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100652/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2011, 12:59 a.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This patch enables the Apply button on the configuration dialog of Lockout.
> 
> 
> Diffs
> -----
> 
>   plasma/generic/applets/lock_logout/lockout.h 84eb9b4 
>   plasma/generic/applets/lock_logout/lockout.cpp 0af405d 
> 
> Diff: http://git.reviewboard.kde.org/r/100652/diff
> 
> 
> Testing
> -------
> 
> I'm not sure about the naming, but it works just fine.
> 
> 
> Thanks,
> 
> Romário
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110214/ddc983dc/attachment.htm 


More information about the Plasma-devel mailing list