Review Request 108570: This patch add support for bulk operations in systemtray applet settings.

[Sebastian Kügler]

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


I've added a bunch of comments inline, those would need addressing. More important though: Could you attach a screenshot of the new UI so that this can be reviewed as well?


plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19881>

    stray whitespace



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19880>

    if (...) {
    Brackets of functions (not methods!) on the same line



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19882>

    Where do the magic numbers come from? Try to avoid them, otherwise add a comment.



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19883>

     brackets around ifs, even when it's only one line



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19884>

    brackets



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19885>

    we usually prefix members with m_, not sure it's done in this class, if it is, please add m_



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19886>

    stray whitespace



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19887>

    Again, magic numbers? Especially with geometry properties, this blows up as soon as font size changes.



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19892>

    i18nc comment won't hurt here



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19888>

    stray ws



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19889>

    brackets



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19890>

    hardcoded height, try using fontmetrics or KIconLoader sizes here, whichever fits



plasma/generic/applets/systemtray/ui/applet.cpp
<http://git.reviewboard.kde.org/r/108570/#comment19891>

    brackets



plasma/generic/applets/systemtray/ui/autohide.ui
<http://git.reviewboard.kde.org/r/108570/#comment19893>

    necessary to add width and height here?


- Sebastian Kügler


On Jan. 24, 2013, 5:34 a.m., Sandro Andrade wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108570/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2013, 5:34 a.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> -------
> 
> In page 'entries', user can select/deselect a specific or all entries and then change visibility and/or reset shortcut for all of them with a single combo selection/clear button click. Individually selecting all entries causes header's checkbox to automatically be checked. After selecting all entries (individually or by clicking in header's checkbox), a single entry deselection causes header's checkbox to automatically be unchecked. General suggestions and, in particular about UI usability, are appreciated.
> 
> 
> Diffs
> -----
> 
>   plasma/generic/applets/systemtray/ui/applet.h 0b4a869 
>   plasma/generic/applets/systemtray/ui/applet.cpp 09482d7 
>   plasma/generic/applets/systemtray/ui/autohide.ui 3b6efff 
> 
> Diff: http://git.reviewboard.kde.org/r/108570/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sandro Andrade
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130124/3f85cf60/attachment.htm>