Merge lp:~nick-dedekind/unity/phablet-greeter-indicators into lp:unity/phablet
- phablet-greeter-indicators
- Merge into phablet
Status: | Merged |
---|---|
Approved by: | Michał Sawicz |
Approved revision: | no longer in the source branch. |
Merged at revision: | 686 |
Proposed branch: | lp:~nick-dedekind/unity/phablet-greeter-indicators |
Merge into: | lp:unity/phablet |
Diff against target: |
177 lines (+34/-33) 6 files modified
Panel/Indicators.qml (+11/-5) Panel/Panel.qml (+15/-11) Panel/SearchIndicator.qml (+0/-8) Shell.qml (+1/-2) tests/qmltests/Panel/tst_Panel.qml (+5/-5) tests/qmltests/Panel/tst_SearchIndicator.qml (+2/-2) |
To merge this branch: | bzr merge lp:~nick-dedekind/unity/phablet-greeter-indicators |
Related bugs: | |
Related blueprints: |
UnityNext Panel Indicators Work
(Undefined)
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michał Sawicz | Approve | ||
PS Jenkins bot (community) | continuous-integration | Approve | |
Albert Astals Cid (community) | Needs Fixing | ||
Katie Taylor | Pending | ||
Review via email:
|
Commit message
Indicator panel now available in greeter
Description of the change
Indicator panel now available in greeter (sans Search bar)
=======
Fixed some state change issues causing initial opening of dash not to have search bar (when removed from greeter).
Caused by state nor being initialized to "initial" by the dynamic assignment of the states.

Michael Terry (mterry) wrote : | # |

Nick Dedekind (nick-dedekind) wrote : | # |
From Saviq:
Only thing I'm not sure of here is that it's possible to tap on the indicators to open them, looks to me like a potential "phone in pocket" issue.
Maybe we should only leave the drag gesture when over greeter?

Nick Dedekind (nick-dedekind) wrote : | # |
> Is this WIP for a particular reason?
No, was just waiting on design review on your branch from Katie.
But since we've moved here then I've added the design review here as well.

Nick Dedekind (nick-dedekind) wrote : | # |
> From Saviq:
>
> Only thing I'm not sure of here is that it's possible to tap on the indicators
> to open them, looks to me like a potential "phone in pocket" issue.
>
> Maybe we should only leave the drag gesture when over greeter?
Problem with disabling tap is that you won't be able to access the device overview page anymore.

PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:537
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:537
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

Albert Astals Cid (aacid) wrote : | # |
qmluitests don't pass

Michał Sawicz (saviq) wrote : | # |
The below tests don't pass:
qmltestrunn
qmltestrunn
qmltestrunn
qmltestrunn

PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:537
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:537
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://

PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:537
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://

PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:539
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

Katie Taylor (katie-t) wrote : | # |
I don't think we should have tap to open on indicators in the greeter. It feels like a gesture that is too easy and would be prone to lots of accidental taps.
Tapping in the top part of the greeter screen should trigger a hint that the indicator menus open. (hint = reveal a small section of the indicator menu, and then fairly quickly hide it.)
This is similar to what we plan for the launcher, see Michael Zanetti's ~mzanetti/

Michał Sawicz (saviq) wrote : | # |
23 + // because of dynamic assignment of states, we need to assign state after completion.
24 + Component.
Is this really true? Why would « state: "initial" » not work?
=====
43 - property alias searchEnabled: search.enabled
44 + property bool searchVisible: true
Why this rename?
=====
94 - function show() {
95 - search.state = "visible"
96 + function show_state() {
97 + return "visible"
98 }
99
100 - function hideUp() {
101 - search.state = "hiddenUp"
102 + function hideUp_state() {
103 + return "hiddenUp"
104 }
169 - searchIndicator
170 + searchIndicator
177 - searchIndicator
178 + searchIndicator
This is weird, why the _state() methods at all? Either direct names or readonly props would suffice, no?
=====
116 + available: true
This is the default.

Nick Dedekind (nick-dedekind) wrote : | # |
> 23 + // because of dynamic assignment of states, we need to assign state
> after completion.
> 24 + Component.
>
> Is this really true? Why would « state: "initial" » not work?
>
Yes, it's a bit of a quirk with QML. It seems the dynamic assignments are computed after the static ones.
ie.
states: pinnedModeStates
is assigned after
state: "initial"
no matter where in the code it's placed.
so, when states property is assigned after state, it clears the state "initial".
> =====
>
> 43 - property alias searchEnabled: search.enabled
> 44 + property bool searchVisible: true
>
> Why this rename?
>
Because it's visiblily vs enabling. Previously search was always visible, only it was disabled. Now it's not visible at all.
> =====
>
> 94 - function show() {
> 95 - search.state = "visible"
> 96 + function show_state() {
> 97 + return "visible"
> 98 }
> 99
> 100 - function hideUp() {
> 101 - search.state = "hiddenUp"
> 102 + function hideUp_state() {
> 103 + return "hiddenUp"
> 104 }
>
> 169 - searchIndicator
> 170 + searchIndicator
>
> 177 - searchIndicator
> 178 + searchIndicator
>
> This is weird, why the _state() methods at all? Either direct names or
> readonly props would suffice, no?
>
Done.
Guess I'm not a fan of setting state by string from external object. :(
> =====
>
> 116 + available: true
>
> This is the default.
Done

PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:540
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://

Michał Sawicz (saviq) wrote : | # |
W dniu 20.05.2013 16:01, Nick Dedekind pisze:
>> 23 + // because of dynamic assignment of states, we need to assign state
>> after completion.
>> 24 + Component.
>>
>> Is this really true? Why would « state: "initial" » not work?
>>
>
> Yes, it's a bit of a quirk with QML. It seems the dynamic assignments are computed after the static ones.
>
> ie.
> states: pinnedModeStates
> is assigned after
> state: "initial"
> no matter where in the code it's placed.
>
> so, when states property is assigned after state, it clears the state "initial".
I could even imagine they're evaluated in alphabetical order ;). After
all you're not supposed to rely on that order.
>> 94 - function show() {
>> 95 - search.state = "visible"
>> 96 + function show_state() {
>> 97 + return "visible"
>> 98 }
>> 99
>> 100 - function hideUp() {
>> 101 - search.state = "hiddenUp"
>> 102 + function hideUp_state() {
>> 103 + return "hiddenUp"
>> 104 }
>>
>> 169 - searchIndicator
>> 170 + searchIndicator
>>
>> 177 - searchIndicator
>> 178 + searchIndicator
>>
>> This is weird, why the _state() methods at all? Either direct names or
>> readonly props would suffice, no?
>>
>
> Done.
> Guess I'm not a fan of setting state by string from external object. :(
Well, then the previous approach (with hideUp(), show()) was one that
didn't do it, no? .state = .show_state() is IMO the same as .state =
"visible" if show_state() just returns "visible". If you want to avoid
that, a show() function that sets the state to "visible" internally was
the right way, no? Why do we need to change the hideUp() and show()
methods at all?
--
Michał Sawicz <email address hidden>
Canonical Services Ltd.

Nick Dedekind (nick-dedekind) wrote : | # |
> W dniu 20.05.2013 16:01, Nick Dedekind pisze:
> >> 23 + // because of dynamic assignment of states, we need to assign
> state
> >> after completion.
> >> 24 + Component.
> >>
> >> Is this really true? Why would « state: "initial" » not work?
> >>
> >
> > Yes, it's a bit of a quirk with QML. It seems the dynamic assignments are
> computed after the static ones.
> >
> > ie.
> > states: pinnedModeStates
> > is assigned after
> > state: "initial"
> > no matter where in the code it's placed.
> >
> > so, when states property is assigned after state, it clears the state
> "initial".
>
> I could even imagine they're evaluated in alphabetical order ;). After
> all you're not supposed to rely on that order.
>
>
> >> 94 - function show() {
> >> 95 - search.state = "visible"
> >> 96 + function show_state() {
> >> 97 + return "visible"
> >> 98 }
> >> 99
> >> 100 - function hideUp() {
> >> 101 - search.state = "hiddenUp"
> >> 102 + function hideUp_state() {
> >> 103 + return "hiddenUp"
> >> 104 }
> >>
> >> 169 - searchIndicator
> >> 170 + searchIndicator
> >>
> >> 177 - searchIndicator
> >> 178 + searchIndicator
> >>
> >> This is weird, why the _state() methods at all? Either direct names or
> >> readonly props would suffice, no?
> >>
> >
> > Done.
> > Guess I'm not a fan of setting state by string from external object. :(
>
> Well, then the previous approach (with hideUp(), show()) was one that
> didn't do it, no? .state = .show_state() is IMO the same as .state =
> "visible" if show_state() just returns "visible". If you want to avoid
> that, a show() function that sets the state to "visible" internally was
> the right way, no? Why do we need to change the hideUp() and show()
> methods at all?
>
Because the search bar state is dependant on the searchVisible flag and the width of the panel, search & indicatorbar, not the state of the Inidcators.
Thought it better to have the state determination in a single place than to have multiple entry points.
I could have had connections to the change of various items which determine the state of the search call into a function to re-interprets that state. But it's a bit messy.
> --
> Michał Sawicz <email address hidden>
> Canonical Services Ltd.

Nick Dedekind (nick-dedekind) wrote : | # |
FYI, I believe the change for the search bar states will fix https:/
Need to confirm though.

Michał Sawicz (saviq) wrote : | # |
I'm not buying the s/enabled/visible/ change, but that's fine ;)
We need to later hide the search label when dash isn't focused (and even later - interface with the app to see if it supports search to determine whether to hide or not).
Also it feels like it's the search label that will hide when there's too many indicators, and it should be the other way 'round - it's the overflow indicators that should hide.
Aaand also the SEARCH label will need to get replaced (slide in from the bottom) with corresponding MUSIC, APPS, etc., or the actual search query (in double quotes).
Aaaand it should also be possible to edge-swipe the search entry down (should on the phone, not only tap on it...
Let me add those as WIs, so we don't lose them :)
Preview Diff
1 | === modified file 'Panel/Indicators.qml' |
2 | --- Panel/Indicators.qml 2013-04-23 10:23:39 +0000 |
3 | +++ Panel/Indicators.qml 2013-05-20 14:15:37 +0000 |
4 | @@ -66,8 +66,6 @@ |
5 | } |
6 | } |
7 | |
8 | - state: "initial" |
9 | - |
10 | function handlePress() { |
11 | menuContent.hideAll() |
12 | menuContent.activateContent() |
13 | @@ -267,9 +265,17 @@ |
14 | } |
15 | } |
16 | |
17 | - //FIXME: when 'states' is changed, "state" is reset to the empty string. Need to find |
18 | - //a way to detect this and restore the correct state again. |
19 | - states: (pinnedMode) ? pinnedModeStates : offScreenModeStates |
20 | + // We start with pinned states. (default pinnedMode: true) |
21 | + states: pinnedModeStates |
22 | + // because of dynamic assignment of states, we need to assign state after completion. |
23 | + Component.onCompleted: state = "initial" |
24 | + |
25 | + // changing states will reset state to "". |
26 | + onPinnedModeChanged: { |
27 | + var last_state = state; |
28 | + states = (pinnedMode) ? pinnedModeStates : offScreenModeStates; |
29 | + state = last_state; |
30 | + } |
31 | |
32 | property list<State> offScreenModeStates: [ |
33 | State { |
34 | |
35 | === modified file 'Panel/Panel.qml' |
36 | --- Panel/Panel.qml 2013-04-12 08:37:35 +0000 |
37 | +++ Panel/Panel.qml 2013-05-20 14:15:37 +0000 |
38 | @@ -25,7 +25,7 @@ |
39 | property real indicatorsMenuWidth: (shell.width > units.gu(60)) ? units.gu(40) : shell.width |
40 | property alias indicators: indicatorsMenu |
41 | property bool fullscreenMode: false |
42 | - property alias searchEnabled: search.enabled |
43 | + property bool searchVisible: true |
44 | |
45 | readonly property real separatorLineHeight: leftSeparatorLine.height |
46 | readonly property real __panelMinusSeparatorLineHeight: panelHeight - separatorLineHeight |
47 | @@ -115,16 +115,6 @@ |
48 | pinnedMode: !fullscreenMode |
49 | |
50 | property real unitProgress: (indicatorRevealer.closedValue - progress) / (indicatorRevealer.closedValue - indicatorRevealer.openedValue) |
51 | - |
52 | - onStateChanged: { |
53 | - if (parent.width < indicatorsMenu.width + search.width) { |
54 | - if (state != "initial") { |
55 | - search.hideUp() |
56 | - return; |
57 | - } |
58 | - } |
59 | - search.show() |
60 | - } |
61 | } |
62 | |
63 | PanelSeparatorLine { |
64 | @@ -172,6 +162,20 @@ |
65 | SearchIndicator { |
66 | id: search |
67 | objectName: "search" |
68 | + enabled: root.searchVisible |
69 | + |
70 | + state: { |
71 | + if (parent.width < indicatorsMenu.width + width) { |
72 | + if (indicatorsMenu.state != "initial") { |
73 | + return "hiddenUp"; |
74 | + } |
75 | + } |
76 | + if (root.searchVisible) { |
77 | + return "visible"; |
78 | + } |
79 | + |
80 | + return "hiddenUp"; |
81 | + } |
82 | |
83 | width: units.gu(13) |
84 | height: __panelMinusSeparatorLineHeight |
85 | |
86 | === modified file 'Panel/SearchIndicator.qml' |
87 | --- Panel/SearchIndicator.qml 2013-04-08 15:36:12 +0000 |
88 | +++ Panel/SearchIndicator.qml 2013-05-20 14:15:37 +0000 |
89 | @@ -27,14 +27,6 @@ |
90 | |
91 | signal clicked |
92 | |
93 | - function show() { |
94 | - search.state = "visible" |
95 | - } |
96 | - |
97 | - function hideUp() { |
98 | - search.state = "hiddenUp" |
99 | - } |
100 | - |
101 | // eater |
102 | MouseArea { |
103 | anchors.fill: parent |
104 | |
105 | === modified file 'Shell.qml' |
106 | --- Shell.qml 2013-05-17 11:00:27 +0000 |
107 | +++ Shell.qml 2013-05-20 14:15:37 +0000 |
108 | @@ -383,10 +383,9 @@ |
109 | indicatorsMenuWidth: parent.width > units.gu(60) ? units.gu(40) : parent.width |
110 | indicators { |
111 | hides: [launcher] |
112 | - available: !greeter.shown |
113 | } |
114 | fullscreenMode: shell.fullscreenMode |
115 | - searchEnabled: !greeter.shown |
116 | + searchVisible: !greeter.shown |
117 | |
118 | InputFilterArea { |
119 | anchors.fill: parent |
120 | |
121 | === modified file 'tests/qmltests/Panel/tst_Panel.qml' |
122 | --- tests/qmltests/Panel/tst_Panel.qml 2013-04-17 13:01:10 +0000 |
123 | +++ tests/qmltests/Panel/tst_Panel.qml 2013-05-20 14:15:37 +0000 |
124 | @@ -235,9 +235,9 @@ |
125 | } |
126 | } |
127 | |
128 | - function test_search_click_when_enabled() { |
129 | + function test_search_click_when_visible() { |
130 | panel.fullscreenMode = false; |
131 | - panel.searchEnabled = true; |
132 | + panel.searchVisible = true; |
133 | |
134 | var search_indicator = findChild(panel, "search"); |
135 | verify(search_indicator != undefined); |
136 | @@ -249,9 +249,9 @@ |
137 | compare(search_clicked, true, "Clicking search indicator while it was enabled did not emit searchClicked signal") |
138 | } |
139 | |
140 | - function test_search_click_when_disabled() { |
141 | + function test_search_click_when_not_visible() { |
142 | panel.fullscreenMode = false; |
143 | - panel.searchEnabled = false |
144 | + panel.searchVisible = false |
145 | |
146 | var search_indicator = findChild(panel, "search"); |
147 | verify(search_indicator != undefined); |
148 | @@ -260,7 +260,7 @@ |
149 | 1, 1, |
150 | Qt.LeftButton, Qt.NoModifier , 0); |
151 | |
152 | - compare(search_clicked, false, "Clicking search indicator while it was enabled emitted searchClicked signal") |
153 | + compare(search_clicked, false, "Clicking search indicator while it was not visible emitted searchClicked signal") |
154 | } |
155 | } |
156 | } |
157 | |
158 | === modified file 'tests/qmltests/Panel/tst_SearchIndicator.qml' |
159 | --- tests/qmltests/Panel/tst_SearchIndicator.qml 2013-04-17 13:01:10 +0000 |
160 | +++ tests/qmltests/Panel/tst_SearchIndicator.qml 2013-05-20 14:15:37 +0000 |
161 | @@ -49,14 +49,14 @@ |
162 | |
163 | function test_hideUp() { |
164 | var container = findChild(searchIndicator, "container") |
165 | - searchIndicator.hideUp() |
166 | + searchIndicator.state = "hiddenUp" |
167 | tryCompare(container, "opacity", 0) |
168 | tryCompare(container, "y", -container.height) |
169 | } |
170 | |
171 | function test_show() { |
172 | var container = findChild(searchIndicator, "container") |
173 | - searchIndicator.show() |
174 | + searchIndicator.state = "visible" |
175 | tryCompare(container, "opacity", 1) |
176 | tryCompare(container, "y", 0) |
177 | } |
Is this WIP for a particular reason?