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

[Sandro Andrade]

> On Jan. 24, 2013, 1:29 p.m., Aaron J. Seigo wrote:
> > While usually we don't want to have visual elements hiding and 
> > disappearing, in this case I wonder if it would make it more 
> > evident/obvious what the bottom items are for (and remove the visual 
> > clutter when they aren't needed). Yes, that would be rather "web app like" 
> > (for lack of a better term), but it may work nicely here.
> > 
> > The phrase "All selected items" .. this might get rather long depending on 
> > the translation? Perhaps just i18np("1 item", "%1 items", itemCount)?
> > 
> > It took me a moment to realize what the [<] button was for; ah, yes, to 
> > remove all key combinations. Seeing as those can't be set all at once, does 
> > it make sense to allow them to all be reset? If it is kept, I'd recommend 
> > something more obvious, such as a button with e.g. "No shortcut"

I took some time investigating some UI patterns for bulk operations until I 
came up with this one (not sure if it's the more appropriate in this case, I 
only thought it'd nice to keep columns aligned). I'll try hide/show the bulk 
table accordingly. As for the [<] bulk action I think it could be useful when 
you have already assigned a bunch of shortcuts and would like to re-assign them 
from a clean setup. But yes, I can remove it since the main motivation for that 
is really the bulk setting of visibility.


- Sandro


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


On Jan. 24, 2013, 1:11 p.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, 1:11 p.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.cpp 09482d7 
>   plasma/generic/applets/systemtray/ui/autohide.ui 3b6efff 
> 
> Diff: http://git.reviewboard.kde.org/r/108570/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Initial setup
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions1.png
> Setting all visibilities to 'Auto'
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions2.png
> All visibilities adjusted to 'Auto'
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions3.png
> Setting two specific item visibilities to 'Always Visible'
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions4.png
> 
> 
> Thanks,
> 
> Sandro Andrade
> 
>

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

[Aaron J. Seigo]

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


While usually we don't want to have visual elements hiding and disappearing, in 
this case I wonder if it would make it more evident/obvious what the bottom 
items are for (and remove the visual clutter when they aren't needed). Yes, 
that would be rather "web app like" (for lack of a better term), but it may 
work nicely here.

The phrase "All selected items" .. this might get rather long depending on the 
translation? Perhaps just i18np("1 item", "%1 items", itemCount)?

It took me a moment to realize what the [<] button was for; ah, yes, to remove 
all key combinations. Seeing as those can't be set all at once, does it make 
sense to allow them to all be reset? If it is kept, I'd recommend something 
more obvious, such as a button with e.g. "No shortcut" 

- Aaron J. Seigo


On Jan. 24, 2013, 4:11 p.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, 4:11 p.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.cpp 09482d7 
>   plasma/generic/applets/systemtray/ui/autohide.ui 3b6efff 
> 
> Diff: http://git.reviewboard.kde.org/r/108570/diff/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> Initial setup
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions1.png
> Setting all visibilities to 'Auto'
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions2.png
> All visibilities adjusted to 'Auto'
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions3.png
> Setting two specific item visibilities to 'Always Visible'
>   
> http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions4.png
> 
> 
> Thanks,
> 
> Sandro Andrade
> 
>

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

[Sandro Andrade]

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

(Updated Jan. 24, 2013, 1:11 p.m.)


Review request for kde-workspace.


Changes
---

Add new diff


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 (updated)
-

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


File Attachments


Initial setup
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions1.png
Setting all visibilities to 'Auto'
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions2.png
All visibilities adjusted to 'Auto'
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions3.png
Setting two specific item visibilities to 'Always Visible'
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions4.png


Thanks,

Sandro Andrade

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

[Sandro Andrade]

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

(Updated Jan. 24, 2013, 1:04 p.m.)


Review request for kde-workspace.


Changes
---

All issues fixed (see comments below). Screenshots attached. New diff uploaded.


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


File Attachments (updated)


Initial setup
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions1.png
Setting all visibilities to 'Auto'
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions2.png
All visibilities adjusted to 'Auto'
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions3.png
Setting two specific item visibilities to 'Always Visible'
  
http://git.reviewboard.kde.org/media/uploaded/files/2013/01/24/systemtray-bulkactions4.png


Thanks,

Sandro Andrade

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

[Sandro Andrade]

> On Jan. 24, 2013, 10:15 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/systemtray/ui/applet.cpp, line 111
> > 
> >
> > Where do the magic numbers come from? Try to avoid them, otherwise add 
> > a comment.

These are basically to horizontally align header's checkbox to each item's 
checkbox. Fixed by getting the checkbox rectangle from current app style and 
painting the checkbox vertically centralized in header section. A fixed left 
margin is used but that is font/style-independent. That matches checkbox 
position for items.


> On Jan. 24, 2013, 10:15 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/systemtray/ui/applet.cpp, line 569
> > 
> >
> > Again, magic numbers? Especially with geometry properties, this blows 
> > up as soon as font size changes.

Fixed again by setting size hint to QRect's size acquired from app-styled 
checkbox.


> On Jan. 24, 2013, 10:15 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/systemtray/ui/applet.cpp, line 587
> > 
> >
> > hardcoded height, try using fontmetrics or KIconLoader sizes here, 
> > whichever fits

Fixed to use headerview's height + appropriate padding.


> On Jan. 24, 2013, 10:15 a.m., Sebastian Kügler wrote:
> > plasma/generic/applets/systemtray/ui/autohide.ui, line 9
> > 
> >
> > necessary to add width and height here?

Reverted


- Sandro


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


On Jan. 24, 2013, 2: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, 2: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
> 
>

Re: 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


stray whitespace



plasma/generic/applets/systemtray/ui/applet.cpp


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



plasma/generic/applets/systemtray/ui/applet.cpp


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



plasma/generic/applets/systemtray/ui/applet.cpp


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



plasma/generic/applets/systemtray/ui/applet.cpp


brackets



plasma/generic/applets/systemtray/ui/applet.cpp


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


stray whitespace



plasma/generic/applets/systemtray/ui/applet.cpp


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



plasma/generic/applets/systemtray/ui/applet.cpp


i18nc comment won't hurt here



plasma/generic/applets/systemtray/ui/applet.cpp


stray ws



plasma/generic/applets/systemtray/ui/applet.cpp


brackets



plasma/generic/applets/systemtray/ui/applet.cpp


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



plasma/generic/applets/systemtray/ui/applet.cpp


brackets



plasma/generic/applets/systemtray/ui/autohide.ui


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