Merge lp:~feng-kylin/unity8/AddTouchStateOnNavigation into lp:unity8

[handsome_feng]

Added touch state on navigation.

Added touch state on navigation.

* Are there any related MPs required for this MP to build/function as expected? Please list.

no

 * Did you perform an exploratory manual test run of your code change and any related functionality?

yes

 * Did you make sure that your branch does not contain spurious tags?

yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

n/a

 * If you changed the UI, has there been a design review?

no

[Mirco Müller (macslow)]

See inline-comments.

review: Needs Fixing

[Mirco Müller (macslow)]

Doh... never mind... just saw the Rectangle with height = units.dp(1) which acts are the divider.

I've still to run and see the change in action. Once that's done I'll approve it as the code looks fine to me.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1823
http://jenkins.qa.ubuntu.com/job/unity8-ci/5934/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/256
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/214
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/215
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/176
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/256
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/256/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21645

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5934/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Please add this checklist[1] to the description field of this merge proposal.

[1] - https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8

[Mirco Müller (macslow)]

Have a look at this photo...

* http://macslow.org/compare-dash-navigation.jpg

On the left it shows your branch running on a Nexus4, while on the left you see a bq Aquaris E4.5 running stock unity8. Looking at the photo provided by Design with the bug...

* https://launchpadlibrarian.net/187741760/touch-estate-example.jpg

... the left and right edges look cut off in your version? Is that ok with Design? It feels weird to me. Please check with them and have one of them comment here.

review: Needs Information

[handsome_feng (feng-kylin)]

> Have a look at this photo...
>
> * http://macslow.org/compare-dash-navigation.jpg
>
> On the left it shows your branch running on a Nexus4, while on the left you
> see a bq Aquaris E4.5 running stock unity8. Looking at the photo provided by
> Design with the bug...
>
> * https://launchpadlibrarian.net/187741760/touch-estate-example.jpg
>
> ... the left and right edges look cut off in your version? Is that ok with
> Design? It feels weird to me. Please check with them and have one of them
> comment here.

I am not sure whether the white space of both edges is needed or not. But since
the photo above is provided by the Designer, I will make it as the given example.

[Albert Astals Cid (aacid)]

Please clean your tags as described in https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8

review: Needs Fixing

[handsome_feng (feng-kylin)]

Seems the tags did not pushed to my branch...
sorry, can anyone tell me how to push it after I delete the
spurious tags ? Thank you in advance!

[Albert Astals Cid (aacid)]

Just run
 strip-tags.py lp:~feng-kylin/unity8/AddTouchStateOnNavigation

[handsome_feng (feng-kylin)]

> Just run
> strip-tags.py lp:~feng-kylin/unity8/AddTouchStateOnNavigation

Thank you! done.

[Albert Astals Cid (aacid)]

Tags are clean i'll leave the rest to Mirco

review: Abstain (tags are clean)

[Mirco Müller (macslow)]

Ok, looking good now.

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes, see http://macslow.org/compare-dash-navigation-2.jpg

* Did CI run pass? If not, please explain why.
No, because there were unrelated autopilot-failures.

* Did you make sure that the branch does not contain spurious tags?
Yes, all clean.

review: Approve

[Albert Astals Cid (aacid)]

I'd say the "allButton" also needs a selected.

I.e.
  make tryDash
  Click on root, rootChild2, allmiddle2
  Now open again the departments again by clicking on middle2
  Any reason allmiddle2 doesn't have a checkmark?

review: Needs Fixing

[handsome_feng (feng-kylin)]

> I'd say the "allButton" also needs a selected.
>
> I.e.
> make tryDash
> Click on root, rootChild2, allmiddle2
> Now open again the departments again by clicking on middle2
> Any reason allmiddle2 doesn't have a checkmark?

I am really not sure about this, I just followed the previously design.

[Mirco Müller (macslow)]

> > I'd say the "allButton" also needs a selected.
> >
> > I.e.
> > make tryDash
> > Click on root, rootChild2, allmiddle2
> > Now open again the departments again by clicking on middle2
> > Any reason allmiddle2 doesn't have a checkmark?
>
> I am really not sure about this, I just followed the previously design.

I tend to agree, since the initial UX-bug https://bugs.launchpad.net/ubuntu-ux/+bug/1383213, only mentions the grey colour for the selected state of an entry being missing and nothing about the checkmark for the "All"-category.

review: Abstain

[Albert Astals Cid (aacid)]

Ok

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1826
http://jenkins.qa.ubuntu.com/job/unity8-ci/6046/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/348
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-wily/76
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/284
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/285
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/252
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/348
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/348/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21865

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/6046/rebuild

review: Needs Fixing (continuous-integration)

=== modified file 'qml/Dash/DashNavigationList.qml'
--- qml/Dash/DashNavigationList.qml	2014-10-23 11:59:22 +0000
+++ qml/Dash/DashNavigationList.qml	2015-07-06 08:19:12 +0000
@@ -16,6 +16,7 @@
 
 import QtQuick 2.2
 import Ubuntu.Components 1.1
+import Ubuntu.Components.ListItems 0.1 as ListItem
 import "../Components"
 
 Item {
@@ -58,13 +59,9 @@
             id: column
             width: parent.width
 
-            // TODO: check if SDK ListItems could be used here
-            // and if not make them be useful since this is a quite common pattern
-
-            AbstractButton {
+            ListItem.Standard {
                 id: backButton
                 objectName: "backButton"
-                width: parent.width
                 visible: navigation && !navigation.isRoot || false
                 height: itemHeight
 
@@ -97,25 +94,11 @@
                     maximumLineCount: 2
                     elide: Text.ElideMiddle
                 }
-
-                Rectangle {
-                    anchors {
-                        bottom: parent.bottom
-                        left: parent.left
-                        right: parent.right
-                        leftMargin: units.gu(2)
-                        rightMargin: units.gu(2)
-                    }
-                    color: root.foregroundColor
-                    opacity: 0.2
-                    height: units.dp(1)
-                }
             }
 
-            AbstractButton {
+            ListItem.Standard {
                 id: allButton
                 objectName: "allButton"
-                width: parent.width
                 visible: navigation && (!navigation.isRoot || (!navigation.hidden && root.currentNavigation && !root.currentNavigation.isRoot && root.currentNavigation.parentNavigationId == navigation.navigationId)) || false
                 height: itemHeight
 
@@ -135,29 +118,17 @@
                     elide: Text.ElideMiddle
                 }
 
-                Rectangle {
-                    anchors {
-                        bottom: parent.bottom
-                        left: parent.left
-                        right: parent.right
-                        leftMargin: units.gu(2)
-                        rightMargin: units.gu(2)
-                    }
-                    color: root.foregroundColor
-                    opacity: 0.2
-                    height: units.dp(1)
-                }
-
                 onClicked: root.allNavigationClicked();
             }
 
             Repeater {
                 model: navigation && navigation.loaded ? navigation : null
                 clip: true
-                delegate: AbstractButton {
+                delegate: ListItem.Standard {
                     objectName: root.objectName + "child" + index
                     height: root.itemHeight
-                    width: root.width
+                    showDivider: index != navigation.count - 1
+                    selected: isActive
 
                     onClicked: root.enterNavigation(navigationId, hasChildren)
 
@@ -189,20 +160,6 @@
                         color: root.foregroundColor
                         visible: hasChildren || isActive
                     }
-
-                    Rectangle {
-                        anchors {
-                            bottom: parent.bottom
-                            left: parent.left
-                            right: parent.right
-                            leftMargin: units.gu(2)
-                            rightMargin: units.gu(2)
-                        }
-                        color: root.foregroundColor
-                        opacity: 0.1
-                        height: units.dp(1)
-                        visible: index != navigation.count - 1
-                    }
                 }
             }
         }