Merge lp:~tpeeters/ubuntu-ui-toolkit/75-sectionsScroll into lp:ubuntu-ui-toolkit/staging
| Status: | Merged |
|---|---|
| Approved by: | Andrea Bernabei on 2016-03-02 |
| Approved revision: | 1884 |
| Merged at revision: | 1879 |
| Proposed branch: | lp:~tpeeters/ubuntu-ui-toolkit/75-sectionsScroll |
| Merge into: | lp:ubuntu-ui-toolkit/staging |
| Diff against target: |
202 lines (+90/-29) 2 files modified
src/Ubuntu/Components/Themes/Ambiance/1.3/SectionsStyle.qml (+34/-28) tests/unit_x11/tst_components/tst_sections.qml (+56/-1) |
| To merge this branch: | bzr merge lp:~tpeeters/ubuntu-ui-toolkit/75-sectionsScroll |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| ubuntu-sdk-build-bot | continuous-integration | Approve on 2016-03-02 | |
| Ubuntu SDK team | 2016-03-01 | Pending | |
|
Review via email:
|
|||
Commit Message
Properly disable left/right arrows on sections when at the beginning/end of the list.
| Andrea Bernabei (faenil) wrote : | # |
PASSED: Continuous integration, rev:1874
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1874
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Andrea Bernabei (faenil) wrote : | # |
I left more comments.
- you can avoid setting "enabled" as Icon is not a component with an active input handler anyway.
- I vote against changing the hovering logic, hoveringLeft should be true when the mouse hovers over the left arrow, even if the arrow is currently disabled, because the mouse is effectively hovering over it.
- instead of changing the semantics of hoveringLeft/Right, I think it would be better to handle this in onPressed, where you can check for hoveringLeft/Right and atXBeginning/End
PASSED: Continuous integration, rev:1874
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1874
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1874
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Tim Peeters (tpeeters) wrote : | # |
> I left more comments.
I replied to the inline comments inline.
>
> - you can avoid setting "enabled" as Icon is not a component with an active
> input handler anyway.
>
> - I vote against changing the hovering logic, hoveringLeft should be true when
> the mouse hovers over the left arrow, even if the arrow is currently disabled,
> because the mouse is effectively hovering over it.
I will change the logic to only check the mouse position when the mouse is pressed. It is not needed to check it whenever the mouse moves.
>
> - instead of changing the semantics of hoveringLeft/Right, I think it would be
> better to handle this in onPressed, where you can check for hoveringLeft/Right
> and atXBeginning/End
Right :) That's what I commented below (I didn't read this yet).
- 1875. By Tim Peeters on 2016-03-01
-
improve left/right button hover detection and action
- 1876. By Tim Peeters on 2016-03-01
-
remove enabled binding
- 1877. By Tim Peeters on 2016-03-01
-
clean
- 1878. By Tim Peeters on 2016-03-01
-
comment
| Andrea Bernabei (faenil) wrote : | # |
thanks.
I liked the hovering logic more, but it's your choice ;)
Maybe rename pressed(mouse) to "contains(mouse)"?
| Andrea Bernabei (faenil) wrote : | # |
also, it's missing unit tests
| Andrea Bernabei (faenil) wrote : | # |
actually, no, I think the logic you changed is broken.
Not updating onPositionChanged means that you can press on right arrow, then move your mouse to the left arrow, then release, and that will trigger a click on the *right* arrow
- 1879. By Tim Peeters on 2016-03-01
-
rename pressed() to contains()
- 1880. By Tim Peeters on 2016-03-01
-
fix logic
- 1881. By Tim Peeters on 2016-03-01
-
update comment
PASSED: Continuous integration, rev:1875
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1875
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1875
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1875
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1875
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1881
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1881
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1881
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1881
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1881
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
- 1882. By Tim Peeters on 2016-03-02
-
add unit test
FAILED: Continuous integration, rev:1882
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
| Andrea Bernabei (faenil) wrote : | # |
commented, it would be good to also have a test for press + release on the other icon, since that was a bug that was about to get released :)
PASSED: Continuous integration, rev:1882
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1882
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1882
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:1882
https:/
Executed test runs:
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
- 1883. By Tim Peeters on 2016-03-02
-
add unit test
- 1884. By Tim Peeters on 2016-03-02
-
add another unit test
| Andrea Bernabei (faenil) wrote : | # |
lgtm! thanks :)
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:1884
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
FAILURE: https:/
None: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1884
https:/
Executed test runs:
None: https:/
Click here to trigger a rebuild:
https:/
just a quick comment, but I'd have to read the logic again to see if the change makes sense.