Merge lp:~mterry/ubuntu-system-settings/location-three-options into lp:ubuntu-system-settings

[Michael Terry]

Redesign location page according to latest design visuals / text. Allows GPS-only, GPS+HERE, or nothing at all.

Redesign location page according to latest design visuals / text. Allows GPS-only, GPS+HERE, or nothing at all.

Visuals: https://drive.google.com/a/canonical.com/folderview?id=0B8I8ZVKH-8SsM1QyMmhWbkpRLTg
Text (canonical when in disagreement): https://docs.google.com/a/canonical.com/document/d/12sLc0ClVx6W39meDx72xyFgGcCoU8PtT5y8OPpWgwyQ

== Checklist ==

 * Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
 Yes

 * Did you build your software in a clean sbuild/pbuilder chroot or ppa?
 Just QML

 * Did you build your software in a clean sbuild/pbuilder armhf chroot or ppa?
 Just QML

 * Has your component "TestPlan” been executed successfully on emulator, N4?
 Yes, krillin

 * Has a 5 minute exploratory testing run been executed on N4?
 Yes, krillin

 * If you changed the packaging (debian), did you subscribe a core-dev to this MP?
 NA

 * If you changed the UI, did you subscribe the design-reviewers to this MP?
 NA

 * What components might get impacted by your changes?
 Just wizard

 * Have you requested review by the teams of these owning components?
 Yah

[Michael Terry (mterry)]

Hrm, design wants the page to be skipped if HERE isn't installed, hold on.

[Michael Terry (mterry)]

OK, let's try now

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1148
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1674/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/6148
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/4176
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/866
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/5776
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/7400
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/7400/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14981
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/3537
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/4527
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/4527/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1674/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1149
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/1678/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/6159/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/4181
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-utopic-i386-ci/870
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/5785/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/7411
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/7411/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14990
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/3541
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/4534
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/4534/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/1678/rebuild

review: Needs Fixing (continuous-integration)

[Ken VanDine (ken-vandine)]

Code looks good, and tested all three scenarios on krillin with HERE installed. I need test without HERE on mako, then I'll approve it.

[Michael Terry (mterry)]

Did you ever manage to test on mako?

[Ken VanDine (ken-vandine)]

Yes, tested on mako and worked great.

review: Approve

=== modified file 'wizard/Utils/system.cpp'
--- wizard/Utils/system.cpp	2014-09-22 15:21:10 +0000
+++ wizard/Utils/system.cpp	2014-10-22 22:38:11 +0000
@@ -20,6 +20,7 @@
 #include <QDBusInterface>
 #include <QDBusPendingCall>
 #include <QDBusPendingReply>
+#include <QFile>
 #include <QProcess>
 #include <unistd.h>
 
@@ -81,11 +82,14 @@
 void System::getHereLicensePathFinished(QDBusPendingCallWatcher *watcher)
 {
     QDBusPendingReply<QVariant> reply = *watcher;
+
+    m_hereLicensePath = "";
+
     if (!reply.isError()) {
         QVariant value = reply.argumentAt<0>();
-        m_hereLicensePath = value.toString();
-    } else {
-        m_hereLicensePath = "";
+        if (QFile::exists(value.toString())) {
+            m_hereLicensePath = value.toString();
+        }
     }
 
     Q_EMIT hereLicensePathChanged();

=== modified file 'wizard/qml/Components/CheckableSetting.qml'
--- wizard/qml/Components/CheckableSetting.qml	2014-08-21 14:50:29 +0000
+++ wizard/qml/Components/CheckableSetting.qml	2014-10-22 22:38:11 +0000
@@ -27,9 +27,11 @@
     property real leftMargin
     property real rightMargin
 
+    readonly property real labelOffset: label.x
+
     signal linkActivated(string link)
 
-    implicitHeight: label.height + units.gu(2)
+    implicitHeight: Math.max(label.height, checkBox.height)
 
     Item {
         anchors.fill: parent
@@ -39,7 +41,7 @@
 
             anchors {
                 left: parent.left
-                verticalCenter: parent.verticalCenter
+                top: parent.top
                 leftMargin: listItem.leftMargin
             }
 
@@ -59,7 +61,10 @@
 
             Connections {
                 target: listItem.__mouseArea
-                onClicked: listItem.checked = !listItem.checked
+                onClicked: {
+                    listItem.checked = !listItem.checked
+                    listItem.triggered(listItem.checked)
+                }
             }
         }
 

=== modified file 'wizard/qml/Pages/50-location.qml'
--- wizard/qml/Pages/50-location.qml	2014-10-22 22:38:11 +0000
+++ wizard/qml/Pages/50-location.qml	2014-10-22 22:38:11 +0000
@@ -25,7 +25,15 @@
     title: i18n.tr("Location")
     forwardButtonSourceComponent: forwardButton
 
-    property bool hereInstalled: System.hereLicensePath !== "" && termsModel.count > 0
+    property bool pathSet: System.hereLicensePath !== " " // single space means it's unassigned
+    property bool countSet: false
+    skipValid: pathSet && (System.hereLicensePath === "" || countSet)
+    skip: skipValid && (System.hereLicensePath === "" || termsModel.count === 0)
+
+    Connections {
+        target: termsModel
+        onCountChanged: if (pathSet) countSet = true
+    }
 
     FolderListModel {
         id: termsModel
@@ -40,49 +48,79 @@
         busType: QMenuModel.DBus.SessionBus
         busName: "com.canonical.indicator.location"
         objectPath: "/com/canonical/indicator/location"
-        property variant enabled: action("location-detection-enabled")
+        property variant location: action("location-detection-enabled")
+        property variant gps: action("gps-detection-enabled")
         Component.onCompleted: start()
     }
 
     Column {
         id: column
         anchors.fill: content
-        spacing: units.gu(2)
-
-        Label {
-            anchors.left: parent.left
-            anchors.right: parent.right
-            visible: hereInstalled
-            wrapMode: Text.Wrap
-            // TRANSLATORS: HERE is a trademark for Nokia's location service, you probably shouldn't translate it
-            text: i18n.tr("Ubuntu includes location services provided by HERE, enabling apps to pinpoint your location.")
-        }
-
-        LocalComponents.CheckableSetting {
-            id: locationCheck
-            showDivider: false
-            text: i18n.tr("Allow apps to use your mobile and Wi-Fi networks to determine your location.")
-            checked: locationActionGroup.enabled.state
-            onTriggered: locationActionGroup.enabled.activate()
-        }
-
-        LocalComponents.CheckableSetting {
-            id: termsCheck
-            showDivider: false
-            visible: hereInstalled
-            // TRANSLATORS: HERE is a trademark for Nokia's location service, you probably shouldn't translate it
-            text: i18n.tr("Accept the HERE <a href='terms.qml'>terms and conditions</a> to enable these services.")
-            onLinkActivated: pageStack.load(Qt.resolvedUrl("here-terms.qml"))
-            checked: System.hereEnabled
-            onTriggered: System.hereEnabled = checked
-        }
-
-        Label {
-            anchors.left: parent.left
-            anchors.right: parent.right
-            visible: hereInstalled
-            wrapMode: Text.Wrap
-            text: i18n.tr("This service can be disabled at any time from the <b>System Settings</b> menu.")
+        spacing: units.gu(3)
+
+        Label {
+            anchors.left: parent.left
+            anchors.right: parent.right
+            wrapMode: Text.Wrap
+            text: i18n.tr("Let the phone detect your location:")
+        }
+
+        LocalComponents.CheckableSetting {
+            id: gpsCheck
+            showDivider: false
+            text: i18n.tr("Using GPS only (less accurate)")
+            onTriggered: {
+                gpsCheck.checked = true;
+                hereCheck.checked = false;
+                nopeCheck.checked = false;
+            }
+        }
+
+        Column {
+            anchors.left: parent.left
+            anchors.right: parent.right
+            height: childrenRect.height
+
+            LocalComponents.CheckableSetting {
+                id: hereCheck
+                showDivider: false
+                text: i18n.tr("Using GPS, anonymized Wi-Fi and cellular network info (recommended)")
+                checked: true
+                onTriggered: {
+                    gpsCheck.checked = false;
+                    hereCheck.checked = true;
+                    nopeCheck.checked = false;
+                }
+            }
+
+            Label {
+                anchors.left: parent.left
+                anchors.leftMargin: hereCheck.labelOffset
+                anchors.right: parent.right
+                wrapMode: Text.Wrap
+                linkColor: Theme.palette.normal.foregroundText
+                // TRANSLATORS: HERE is a trademark for Nokia's location service, you probably shouldn't translate it
+                text: i18n.tr("By selecting this option you agree to the Nokia HERE <a href='#'>terms and conditions</a>.")
+                onLinkActivated: pageStack.load(Qt.resolvedUrl("here-terms.qml"))
+            }
+        }
+
+        LocalComponents.CheckableSetting {
+            id: nopeCheck
+            showDivider: false
+            text: i18n.tr("Not at all")
+            onTriggered: {
+                gpsCheck.checked = false;
+                hereCheck.checked = false;
+                nopeCheck.checked = true;
+            }
+        }
+
+        Label {
+            anchors.left: parent.left
+            anchors.right: parent.right
+            wrapMode: Text.Wrap
+            text: i18n.tr("You can change your mind later in <b>System Settings</b>.")
         }
     }
 
@@ -90,7 +128,22 @@
         id: forwardButton
         LocalComponents.StackButton {
             text: i18n.tr("Continue")
-            onClicked: pageStack.next()
+            onClicked: {
+                var locationOn = gpsCheck.checked || hereCheck.checked;
+                var gpsOn = gpsCheck.checked || hereCheck.checked;
+                var hereOn = hereCheck.checked;
+
+                // location service doesn't currently listen to updateState
+                // requests, so we activate the actions if needed.
+                if (locationActionGroup.location.state != locationOn) {
+                    locationActionGroup.location.activate();
+                }
+                if (locationActionGroup.gps.state != gpsOn) {
+                    locationActionGroup.gps.activate();
+                }
+                System.hereEnabled = hereOn;
+                pageStack.next()
+            }
         }
     }
 }