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

[Nick Dedekind]

Indicator panel now available in greeter

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

Is this WIP for a particular reason?

[Nick Dedekind (nick-dedekind)]

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

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

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

FAILED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/689/
Executed test runs:
    UNSTABLE: http://s-jenkins:8080/job/unity-phablet-qmluitests/501
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-armhf-ci/691
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-armhf-ci/691/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-i386-ci/689
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/565
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/565/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/569
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner/1218

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/689/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/719/
Executed test runs:
    UNSTABLE: http://s-jenkins:8080/job/unity-phablet-qmluitests/543
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-armhf-ci/721
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-armhf-ci/721/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-i386-ci/719
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/595
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/595/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/599
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner/1262

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/719/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

qmluitests don't pass

review: Needs Fixing

[Michał Sawicz (saviq)]

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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/747/
Executed test runs:
    UNSTABLE: http://s-jenkins:8080/job/unity-phablet-qmluitests/582
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-armhf-ci/749
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-armhf-ci/749/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-quantal-i386-ci/747
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/623
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/623/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/627
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner/1295

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/747/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/852/
Executed test runs:
    UNSTABLE: http://s-jenkins:8080/job/unity-phablet-qmluitests/772
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/728/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/732/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/852/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:537
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/880/
Executed test runs:
    FAILURE: http://s-jenkins:8080/job/unity-phablet-qmluitests/809/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/756/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/760/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/880/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:539
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/898/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/unity-phablet-qmluitests/832
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/774
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/774/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/778

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/898/rebuild

review: Approve (continuous-integration)

[Katie Taylor (katie-t)]

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

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

> 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

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:540
http://jenkins.qa.ubuntu.com/job/unity-phablet-ci/975/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/unity-phablet-qmluitests/944
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/851
        deb: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-armhf-ci/851/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-raring-i386-ci/855
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner/1554

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-phablet-ci/975/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

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

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

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

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

=== modified file 'Panel/Indicators.qml'
--- Panel/Indicators.qml	2013-04-23 10:23:39 +0000
+++ Panel/Indicators.qml	2013-05-20 14:15:37 +0000
@@ -66,8 +66,6 @@
         }
     }
 
-    state: "initial"
-
     function handlePress() {
         menuContent.hideAll()
         menuContent.activateContent()
@@ -267,9 +265,17 @@
         }
     }
 
-    //FIXME: when 'states' is changed, "state" is reset to the empty string. Need to find
-    //a way to detect this and restore the correct state again.
-    states: (pinnedMode) ? pinnedModeStates : offScreenModeStates
+    // We start with pinned states. (default pinnedMode: true)
+    states: pinnedModeStates
+    // because of dynamic assignment of states, we need to assign state after completion.
+    Component.onCompleted: state = "initial"
+
+    // changing states will reset state to "".
+    onPinnedModeChanged: {
+        var last_state = state;
+        states = (pinnedMode) ? pinnedModeStates : offScreenModeStates;
+        state = last_state;
+    }
 
     property list<State> offScreenModeStates: [
         State {

=== modified file 'Panel/Panel.qml'
--- Panel/Panel.qml	2013-04-12 08:37:35 +0000
+++ Panel/Panel.qml	2013-05-20 14:15:37 +0000
@@ -25,7 +25,7 @@
     property real indicatorsMenuWidth: (shell.width > units.gu(60)) ? units.gu(40) : shell.width
     property alias indicators: indicatorsMenu
     property bool fullscreenMode: false
-    property alias searchEnabled: search.enabled
+    property bool searchVisible: true
 
     readonly property real separatorLineHeight: leftSeparatorLine.height
     readonly property real __panelMinusSeparatorLineHeight: panelHeight - separatorLineHeight
@@ -115,16 +115,6 @@
         pinnedMode: !fullscreenMode
 
         property real unitProgress: (indicatorRevealer.closedValue - progress) / (indicatorRevealer.closedValue - indicatorRevealer.openedValue)
-
-        onStateChanged: {
-            if (parent.width < indicatorsMenu.width + search.width) {
-                if (state != "initial") {
-                    search.hideUp()
-                    return;
-                }
-            }
-            search.show()
-        }
     }
 
     PanelSeparatorLine {
@@ -172,6 +162,20 @@
     SearchIndicator {
         id: search
         objectName: "search"
+        enabled: root.searchVisible
+
+        state: {
+            if (parent.width < indicatorsMenu.width + width) {
+                if (indicatorsMenu.state != "initial") {
+                    return "hiddenUp";
+                }
+            }
+            if (root.searchVisible) {
+                return "visible";
+            }
+
+            return "hiddenUp";
+        }
 
         width: units.gu(13)
         height: __panelMinusSeparatorLineHeight

=== modified file 'Panel/SearchIndicator.qml'
--- Panel/SearchIndicator.qml	2013-04-08 15:36:12 +0000
+++ Panel/SearchIndicator.qml	2013-05-20 14:15:37 +0000
@@ -27,14 +27,6 @@
 
     signal clicked
 
-    function show() {
-        search.state = "visible"
-    }
-
-    function hideUp() {
-        search.state = "hiddenUp"
-    }
-
     // eater
     MouseArea {
         anchors.fill: parent

=== modified file 'Shell.qml'
--- Shell.qml	2013-05-17 11:00:27 +0000
+++ Shell.qml	2013-05-20 14:15:37 +0000
@@ -383,10 +383,9 @@
             indicatorsMenuWidth: parent.width > units.gu(60) ? units.gu(40) : parent.width
             indicators {
                 hides: [launcher]
-                available: !greeter.shown
             }
             fullscreenMode: shell.fullscreenMode
-            searchEnabled: !greeter.shown
+            searchVisible: !greeter.shown
 
             InputFilterArea {
                 anchors.fill: parent

=== modified file 'tests/qmltests/Panel/tst_Panel.qml'
--- tests/qmltests/Panel/tst_Panel.qml	2013-04-17 13:01:10 +0000
+++ tests/qmltests/Panel/tst_Panel.qml	2013-05-20 14:15:37 +0000
@@ -235,9 +235,9 @@
             }
         }
 
-        function test_search_click_when_enabled() {
+        function test_search_click_when_visible() {
             panel.fullscreenMode = false;
-            panel.searchEnabled = true;
+            panel.searchVisible = true;
 
             var search_indicator = findChild(panel, "search");
             verify(search_indicator != undefined);
@@ -249,9 +249,9 @@
             compare(search_clicked, true, "Clicking search indicator while it was enabled did not emit searchClicked signal")
         }
 
-        function test_search_click_when_disabled() {
+        function test_search_click_when_not_visible() {
             panel.fullscreenMode = false;
-            panel.searchEnabled = false
+            panel.searchVisible = false
 
             var search_indicator = findChild(panel, "search");
             verify(search_indicator != undefined);
@@ -260,7 +260,7 @@
                        1, 1,
                        Qt.LeftButton, Qt.NoModifier , 0);
 
-            compare(search_clicked, false, "Clicking search indicator while it was enabled emitted searchClicked signal")
+            compare(search_clicked, false, "Clicking search indicator while it was not visible emitted searchClicked signal")
         }
     }
 }

=== modified file 'tests/qmltests/Panel/tst_SearchIndicator.qml'
--- tests/qmltests/Panel/tst_SearchIndicator.qml	2013-04-17 13:01:10 +0000
+++ tests/qmltests/Panel/tst_SearchIndicator.qml	2013-05-20 14:15:37 +0000
@@ -49,14 +49,14 @@
 
         function test_hideUp() {
             var container = findChild(searchIndicator, "container")
-            searchIndicator.hideUp()
+            searchIndicator.state = "hiddenUp"
             tryCompare(container, "opacity", 0)
             tryCompare(container, "y", -container.height)
         }
 
         function test_show() {
             var container = findChild(searchIndicator, "container")
-            searchIndicator.show()
+            searchIndicator.state = "visible"
             tryCompare(container, "opacity", 1)
             tryCompare(container, "y", 0)
         }