Merge lp:~nick-dedekind/unity/phablet-greeter-indicators into lp:unity/phablet

Proposed by Nick Dedekind on 2013-04-05
Status: Merged
Approved by: Michał Sawicz on 2013-05-23
Approved revision: 540
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
Reviewer Review Type Date Requested Status
Michał Sawicz Approve on 2013-05-23
PS Jenkins bot (community) continuous-integration Approve on 2013-05-20
Albert Astals Cid (community) 2013-04-05 Needs Fixing on 2013-04-23
Katie Taylor 2013-04-22 Pending
Review via email: mp+157330@code.launchpad.net

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.

To post a comment you must log in.
Michael Terry (mterry) wrote :

Is this WIP for a particular reason?

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.

Albert Astals Cid (aacid) wrote :

qmluitests don't pass

review: Needs Fixing
Michał Sawicz (saviq) wrote :

The below tests don't pass:
    qmltestrunner.Panel::test_search_click_when_disabled
    qmltestrunner.Panel::test_search_click_when_enabled
    qmltestrunner.SearchIndicator::test_hideUp
    qmltestrunner.SearchIndicator::test_show

review: Needs Fixing
538. By Nick Dedekind on 2013-05-13

merged with trunk

539. By Nick Dedekind on 2013-05-13

Fixed some test regression

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/unity/phablet-tease-launcher

Michał Sawicz (saviq) wrote :

23 + // because of dynamic assignment of states, we need to assign state after completion.
24 + Component.onCompleted: state = "initial"

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.hideUp()
170 + searchIndicator.state = searchIndicator.hideUp_state()

177 - searchIndicator.show()
178 + searchIndicator.state = searchIndicator.show_state()

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.

review: Needs Fixing
Nick Dedekind (nick-dedekind) wrote :

> 23 + // because of dynamic assignment of states, we need to assign state
> after completion.
> 24 + Component.onCompleted: state = "initial"
>
> 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.hideUp()
> 170 + searchIndicator.state = searchIndicator.hideUp_state()
>
> 177 - searchIndicator.show()
> 178 + searchIndicator.state = searchIndicator.show_state()
>
> 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

540. By Nick Dedekind on 2013-05-20

Updated for review comments

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.onCompleted: state = "initial"
>>
>> 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.hideUp()
>> 170 + searchIndicator.state = searchIndicator.hideUp_state()
>>
>> 177 - searchIndicator.show()
>> 178 + searchIndicator.state = searchIndicator.show_state()
>>
>> 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.onCompleted: state = "initial"
> >>
> >> 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.hideUp()
> >> 170 + searchIndicator.state = searchIndicator.hideUp_state()
> >>
> >> 177 - searchIndicator.show()
> >> 178 + searchIndicator.state = searchIndicator.show_state()
> >>
> >> 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://bugs.launchpad.net/touch-preview-images/+bug/1181249

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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 }

Subscribers

People subscribed via source and target branches