Re: Review Request 129405: KCollapsibleGroupBox: don't hide widgets, override focus policy instead

2016-11-15 Thread Elvis Angelaccio 

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

(Updated Nov. 15, 2016, 4:27 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
Edmundson.


Changes
---

Submitted with commit 1076908c344674d2be4f4825f18f6f3900a09620 by Elvis 
Angelaccio to branch master.


Repository: kwidgetsaddons


Description
---

Currently it's not possible to hide widgets in a KCollapsibleGroupBox, because 
the groupbox hides all chidren on collapse event and shows them on expand event.

The rationale was explained in this comment: "when collapsed hide contents to 
save resources and more importantly get it out the focus chain"

The focus chain problem can be solved by overriding/restoring the focus policy 
of children widgets, without changing their visibility.


Diffs
-

  autotests/kcollapsiblegroupbox_test.h 
b7f538217f480ea48bc28f098c7968fe21dda676 
  autotests/kcollapsiblegroupbox_test.cpp 
4c458c4f700e498c178a637c29d6cc78ab8c267c 
  src/kcollapsiblegroupbox.cpp 273110e995cb25f28a815cb28125c4678ca2ab28 
  tests/kcollapsiblegroupboxtest.cpp 5a2900e63e42fa81f909c7d1fff0d07033edb025 

Diff: https://git.reviewboard.kde.org/r/129405/diff/


Testing
---

Test app has been expanded with an hidden checkbox

* Make sure the new checkbox stays hidden after expanding the groupbox.
* With both groups collapsed, make sure the tab-focus goes from the first group 
to the second one (skipping the 7 checkboxes in the first group).


File Attachments


Before: hidden checkbox is visible
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/25c50bd2-aa00-42ea-b9b5-38f3d1abddff__Spectacle.d20127.png
After: hidden checkbox is hidden
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/815551e3-9987-4851-8025-50d44989f600__Spectacle.W20858.png


Thanks,

Elvis Angelaccio

Re: Review Request 129405: KCollapsibleGroupBox: don't hide widgets, override focus policy instead

2016-11-15 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129405/#review100839
---


Fix it, then Ship it!





src/kcollapsiblegroupbox.cpp (line 47)


why int and not Qt::FocusPolicy?

it'll save you casting later.


- David Edmundson


On Nov. 15, 2016, 11:06 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129405/
> ---
> 
> (Updated Nov. 15, 2016, 11:06 a.m.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Edmundson.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Currently it's not possible to hide widgets in a KCollapsibleGroupBox, 
> because the groupbox hides all chidren on collapse event and shows them on 
> expand event.
> 
> The rationale was explained in this comment: "when collapsed hide contents to 
> save resources and more importantly get it out the focus chain"
> 
> The focus chain problem can be solved by overriding/restoring the focus 
> policy of children widgets, without changing their visibility.
> 
> 
> Diffs
> -
> 
>   autotests/kcollapsiblegroupbox_test.h 
> b7f538217f480ea48bc28f098c7968fe21dda676 
>   autotests/kcollapsiblegroupbox_test.cpp 
> 4c458c4f700e498c178a637c29d6cc78ab8c267c 
>   src/kcollapsiblegroupbox.cpp 273110e995cb25f28a815cb28125c4678ca2ab28 
>   tests/kcollapsiblegroupboxtest.cpp 5a2900e63e42fa81f909c7d1fff0d07033edb025 
> 
> Diff: https://git.reviewboard.kde.org/r/129405/diff/
> 
> 
> Testing
> ---
> 
> Test app has been expanded with an hidden checkbox
> 
> * Make sure the new checkbox stays hidden after expanding the groupbox.
> * With both groups collapsed, make sure the tab-focus goes from the first 
> group to the second one (skipping the 7 checkboxes in the first group).
> 
> 
> File Attachments
> 
> 
> Before: hidden checkbox is visible
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/25c50bd2-aa00-42ea-b9b5-38f3d1abddff__Spectacle.d20127.png
> After: hidden checkbox is hidden
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/815551e3-9987-4851-8025-50d44989f600__Spectacle.W20858.png
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 129405: KCollapsibleGroupBox: don't hide widgets, override focus policy instead

2016-11-15 Thread Heiko Tietze 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129405/#review100856
---


Ship it!




Looks good from the usability POV, hidden items should kept invisible whether 
or not expanded. 

Some random thoughts: Enabled state should be respected correctly; as you write 
"focus policy" make sure the tab sequence works well. And finally we should 
define a behaviour when all children are hidden: I'd recommend to still show 
the expander and just accept the possible zero content. Alignment is relevant 
too meaning the anchored control must not have a large space to the collapsed 
list with hidden items.

- Heiko Tietze


On Nov. 15, 2016, 11:06 vorm., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129405/
> ---
> 
> (Updated Nov. 15, 2016, 11:06 vorm.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Edmundson.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Currently it's not possible to hide widgets in a KCollapsibleGroupBox, 
> because the groupbox hides all chidren on collapse event and shows them on 
> expand event.
> 
> The rationale was explained in this comment: "when collapsed hide contents to 
> save resources and more importantly get it out the focus chain"
> 
> The focus chain problem can be solved by overriding/restoring the focus 
> policy of children widgets, without changing their visibility.
> 
> 
> Diffs
> -
> 
>   autotests/kcollapsiblegroupbox_test.h 
> b7f538217f480ea48bc28f098c7968fe21dda676 
>   autotests/kcollapsiblegroupbox_test.cpp 
> 4c458c4f700e498c178a637c29d6cc78ab8c267c 
>   src/kcollapsiblegroupbox.cpp 273110e995cb25f28a815cb28125c4678ca2ab28 
>   tests/kcollapsiblegroupboxtest.cpp 5a2900e63e42fa81f909c7d1fff0d07033edb025 
> 
> Diff: https://git.reviewboard.kde.org/r/129405/diff/
> 
> 
> Testing
> ---
> 
> Test app has been expanded with an hidden checkbox
> 
> * Make sure the new checkbox stays hidden after expanding the groupbox.
> * With both groups collapsed, make sure the tab-focus goes from the first 
> group to the second one (skipping the 7 checkboxes in the first group).
> 
> 
> File Attachments
> 
> 
> Before: hidden checkbox is visible
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/25c50bd2-aa00-42ea-b9b5-38f3d1abddff__Spectacle.d20127.png
> After: hidden checkbox is hidden
>   
> https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/815551e3-9987-4851-8025-50d44989f600__Spectacle.W20858.png
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 129405: KCollapsibleGroupBox: don't hide widgets, override focus policy instead

2016-11-15 Thread Elvis Angelaccio 

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

(Updated Nov. 15, 2016, 11:06 a.m.)


Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
Edmundson.


Changes
---

Added screenshots before/after, but they just show that the last checkbox is 
hidden as expected. Unfortunately screenshots cannot catch the focus with the 
Tab key.


Repository: kwidgetsaddons


Description
---

Currently it's not possible to hide widgets in a KCollapsibleGroupBox, because 
the groupbox hides all chidren on collapse event and shows them on expand event.

The rationale was explained in this comment: "when collapsed hide contents to 
save resources and more importantly get it out the focus chain"

The focus chain problem can be solved by overriding/restoring the focus policy 
of children widgets, without changing their visibility.


Diffs
-

  autotests/kcollapsiblegroupbox_test.h 
b7f538217f480ea48bc28f098c7968fe21dda676 
  autotests/kcollapsiblegroupbox_test.cpp 
4c458c4f700e498c178a637c29d6cc78ab8c267c 
  src/kcollapsiblegroupbox.cpp 273110e995cb25f28a815cb28125c4678ca2ab28 
  tests/kcollapsiblegroupboxtest.cpp 5a2900e63e42fa81f909c7d1fff0d07033edb025 

Diff: https://git.reviewboard.kde.org/r/129405/diff/


Testing
---

Test app has been expanded with an hidden checkbox

* Make sure the new checkbox stays hidden after expanding the groupbox.
* With both groups collapsed, make sure the tab-focus goes from the first group 
to the second one (skipping the 7 checkboxes in the first group).


File Attachments (updated)


Before: hidden checkbox is visible
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/25c50bd2-aa00-42ea-b9b5-38f3d1abddff__Spectacle.d20127.png
After: hidden checkbox is hidden
  
https://git.reviewboard.kde.org/media/uploaded/files/2016/11/15/815551e3-9987-4851-8025-50d44989f600__Spectacle.W20858.png


Thanks,

Elvis Angelaccio

Re: Review Request 129405: KCollapsibleGroupBox: don't hide widgets, override focus policy instead

2016-11-15 Thread Heiko Tietze 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129405/#review100850
---



Picture, or it didn't happen :-) (before/after would be nice)

- Heiko Tietze


On Nov. 15, 2016, 9:53 vorm., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129405/
> ---
> 
> (Updated Nov. 15, 2016, 9:53 vorm.)
> 
> 
> Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
> Edmundson.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> ---
> 
> Currently it's not possible to hide widgets in a KCollapsibleGroupBox, 
> because the groupbox hides all chidren on collapse event and shows them on 
> expand event.
> 
> The rationale was explained in this comment: "when collapsed hide contents to 
> save resources and more importantly get it out the focus chain"
> 
> The focus chain problem can be solved by overriding/restoring the focus 
> policy of children widgets, without changing their visibility.
> 
> 
> Diffs
> -
> 
>   autotests/kcollapsiblegroupbox_test.h 
> b7f538217f480ea48bc28f098c7968fe21dda676 
>   autotests/kcollapsiblegroupbox_test.cpp 
> 4c458c4f700e498c178a637c29d6cc78ab8c267c 
>   src/kcollapsiblegroupbox.cpp 273110e995cb25f28a815cb28125c4678ca2ab28 
>   tests/kcollapsiblegroupboxtest.cpp 5a2900e63e42fa81f909c7d1fff0d07033edb025 
> 
> Diff: https://git.reviewboard.kde.org/r/129405/diff/
> 
> 
> Testing
> ---
> 
> Test app has been expanded with an hidden checkbox
> 
> * Make sure the new checkbox stays hidden after expanding the groupbox.
> * With both groups collapsed, make sure the tab-focus goes from the first 
> group to the second one (skipping the 7 checkboxes in the first group).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

Re: Review Request 129405: KCollapsibleGroupBox: don't hide widgets, override focus policy instead

2016-11-15 Thread Elvis Angelaccio 

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

(Updated Nov. 15, 2016, 9:53 a.m.)


Review request for KDE Frameworks, KDE Usability, Christoph Feck, and David 
Edmundson.


Changes
---

Also add unit test.


Repository: kwidgetsaddons


Description
---

Currently it's not possible to hide widgets in a KCollapsibleGroupBox, because 
the groupbox hides all chidren on collapse event and shows them on expand event.

The rationale was explained in this comment: "when collapsed hide contents to 
save resources and more importantly get it out the focus chain"

The focus chain problem can be solved by overriding/restoring the focus policy 
of children widgets, without changing their visibility.


Diffs (updated)
-

  autotests/kcollapsiblegroupbox_test.h 
b7f538217f480ea48bc28f098c7968fe21dda676 
  autotests/kcollapsiblegroupbox_test.cpp 
4c458c4f700e498c178a637c29d6cc78ab8c267c 
  src/kcollapsiblegroupbox.cpp 273110e995cb25f28a815cb28125c4678ca2ab28 
  tests/kcollapsiblegroupboxtest.cpp 5a2900e63e42fa81f909c7d1fff0d07033edb025 

Diff: https://git.reviewboard.kde.org/r/129405/diff/


Testing
---

Test app has been expanded with an hidden checkbox

* Make sure the new checkbox stays hidden after expanding the groupbox.
* With both groups collapsed, make sure the tab-focus goes from the first group 
to the second one (skipping the 7 checkboxes in the first group).


Thanks,

Elvis Angelaccio