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

Proposed by Michael Terry
Status: Merged
Approved by: Ken VanDine
Approved revision: 1149
Merged at revision: 1180
Proposed branch: lp:~mterry/ubuntu-system-settings/location-three-options
Merge into: lp:ubuntu-system-settings
Prerequisite: lp:~mterry/ubuntu-system-settings/terms-typo
Diff against target: 229 lines (+107/-45)
3 files modified
wizard/Utils/system.cpp (+7/-3)
wizard/qml/Components/CheckableSetting.qml (+8/-3)
wizard/qml/Pages/50-location.qml (+92/-39)
To merge this branch: bzr merge lp:~mterry/ubuntu-system-settings/location-three-options
Reviewer Review Type Date Requested Status
Ken VanDine Approve
PS Jenkins bot continuous-integration Needs Fixing
Review via email: mp+239274@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Michael Terry (mterry) wrote :

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

Revision history for this message
Michael Terry (mterry) wrote :

OK, let's try now

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Ken VanDine (ken-vandine) wrote :

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.

Revision history for this message
Michael Terry (mterry) wrote :

Did you ever manage to test on mako?

Revision history for this message
Ken VanDine (ken-vandine) wrote :

Yes, tested on mako and worked great.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'wizard/Utils/system.cpp'
2--- wizard/Utils/system.cpp 2014-09-22 15:21:10 +0000
3+++ wizard/Utils/system.cpp 2014-10-22 22:38:11 +0000
4@@ -20,6 +20,7 @@
5 #include <QDBusInterface>
6 #include <QDBusPendingCall>
7 #include <QDBusPendingReply>
8+#include <QFile>
9 #include <QProcess>
10 #include <unistd.h>
11
12@@ -81,11 +82,14 @@
13 void System::getHereLicensePathFinished(QDBusPendingCallWatcher *watcher)
14 {
15 QDBusPendingReply<QVariant> reply = *watcher;
16+
17+ m_hereLicensePath = "";
18+
19 if (!reply.isError()) {
20 QVariant value = reply.argumentAt<0>();
21- m_hereLicensePath = value.toString();
22- } else {
23- m_hereLicensePath = "";
24+ if (QFile::exists(value.toString())) {
25+ m_hereLicensePath = value.toString();
26+ }
27 }
28
29 Q_EMIT hereLicensePathChanged();
30
31=== modified file 'wizard/qml/Components/CheckableSetting.qml'
32--- wizard/qml/Components/CheckableSetting.qml 2014-08-21 14:50:29 +0000
33+++ wizard/qml/Components/CheckableSetting.qml 2014-10-22 22:38:11 +0000
34@@ -27,9 +27,11 @@
35 property real leftMargin
36 property real rightMargin
37
38+ readonly property real labelOffset: label.x
39+
40 signal linkActivated(string link)
41
42- implicitHeight: label.height + units.gu(2)
43+ implicitHeight: Math.max(label.height, checkBox.height)
44
45 Item {
46 anchors.fill: parent
47@@ -39,7 +41,7 @@
48
49 anchors {
50 left: parent.left
51- verticalCenter: parent.verticalCenter
52+ top: parent.top
53 leftMargin: listItem.leftMargin
54 }
55
56@@ -59,7 +61,10 @@
57
58 Connections {
59 target: listItem.__mouseArea
60- onClicked: listItem.checked = !listItem.checked
61+ onClicked: {
62+ listItem.checked = !listItem.checked
63+ listItem.triggered(listItem.checked)
64+ }
65 }
66 }
67
68
69=== modified file 'wizard/qml/Pages/50-location.qml'
70--- wizard/qml/Pages/50-location.qml 2014-10-22 22:38:11 +0000
71+++ wizard/qml/Pages/50-location.qml 2014-10-22 22:38:11 +0000
72@@ -25,7 +25,15 @@
73 title: i18n.tr("Location")
74 forwardButtonSourceComponent: forwardButton
75
76- property bool hereInstalled: System.hereLicensePath !== "" && termsModel.count > 0
77+ property bool pathSet: System.hereLicensePath !== " " // single space means it's unassigned
78+ property bool countSet: false
79+ skipValid: pathSet && (System.hereLicensePath === "" || countSet)
80+ skip: skipValid && (System.hereLicensePath === "" || termsModel.count === 0)
81+
82+ Connections {
83+ target: termsModel
84+ onCountChanged: if (pathSet) countSet = true
85+ }
86
87 FolderListModel {
88 id: termsModel
89@@ -40,49 +48,79 @@
90 busType: QMenuModel.DBus.SessionBus
91 busName: "com.canonical.indicator.location"
92 objectPath: "/com/canonical/indicator/location"
93- property variant enabled: action("location-detection-enabled")
94+ property variant location: action("location-detection-enabled")
95+ property variant gps: action("gps-detection-enabled")
96 Component.onCompleted: start()
97 }
98
99 Column {
100 id: column
101 anchors.fill: content
102- spacing: units.gu(2)
103-
104- Label {
105- anchors.left: parent.left
106- anchors.right: parent.right
107- visible: hereInstalled
108- wrapMode: Text.Wrap
109- // TRANSLATORS: HERE is a trademark for Nokia's location service, you probably shouldn't translate it
110- text: i18n.tr("Ubuntu includes location services provided by HERE, enabling apps to pinpoint your location.")
111- }
112-
113- LocalComponents.CheckableSetting {
114- id: locationCheck
115- showDivider: false
116- text: i18n.tr("Allow apps to use your mobile and Wi-Fi networks to determine your location.")
117- checked: locationActionGroup.enabled.state
118- onTriggered: locationActionGroup.enabled.activate()
119- }
120-
121- LocalComponents.CheckableSetting {
122- id: termsCheck
123- showDivider: false
124- visible: hereInstalled
125- // TRANSLATORS: HERE is a trademark for Nokia's location service, you probably shouldn't translate it
126- text: i18n.tr("Accept the HERE <a href='terms.qml'>terms and conditions</a> to enable these services.")
127- onLinkActivated: pageStack.load(Qt.resolvedUrl("here-terms.qml"))
128- checked: System.hereEnabled
129- onTriggered: System.hereEnabled = checked
130- }
131-
132- Label {
133- anchors.left: parent.left
134- anchors.right: parent.right
135- visible: hereInstalled
136- wrapMode: Text.Wrap
137- text: i18n.tr("This service can be disabled at any time from the <b>System Settings</b> menu.")
138+ spacing: units.gu(3)
139+
140+ Label {
141+ anchors.left: parent.left
142+ anchors.right: parent.right
143+ wrapMode: Text.Wrap
144+ text: i18n.tr("Let the phone detect your location:")
145+ }
146+
147+ LocalComponents.CheckableSetting {
148+ id: gpsCheck
149+ showDivider: false
150+ text: i18n.tr("Using GPS only (less accurate)")
151+ onTriggered: {
152+ gpsCheck.checked = true;
153+ hereCheck.checked = false;
154+ nopeCheck.checked = false;
155+ }
156+ }
157+
158+ Column {
159+ anchors.left: parent.left
160+ anchors.right: parent.right
161+ height: childrenRect.height
162+
163+ LocalComponents.CheckableSetting {
164+ id: hereCheck
165+ showDivider: false
166+ text: i18n.tr("Using GPS, anonymized Wi-Fi and cellular network info (recommended)")
167+ checked: true
168+ onTriggered: {
169+ gpsCheck.checked = false;
170+ hereCheck.checked = true;
171+ nopeCheck.checked = false;
172+ }
173+ }
174+
175+ Label {
176+ anchors.left: parent.left
177+ anchors.leftMargin: hereCheck.labelOffset
178+ anchors.right: parent.right
179+ wrapMode: Text.Wrap
180+ linkColor: Theme.palette.normal.foregroundText
181+ // TRANSLATORS: HERE is a trademark for Nokia's location service, you probably shouldn't translate it
182+ text: i18n.tr("By selecting this option you agree to the Nokia HERE <a href='#'>terms and conditions</a>.")
183+ onLinkActivated: pageStack.load(Qt.resolvedUrl("here-terms.qml"))
184+ }
185+ }
186+
187+ LocalComponents.CheckableSetting {
188+ id: nopeCheck
189+ showDivider: false
190+ text: i18n.tr("Not at all")
191+ onTriggered: {
192+ gpsCheck.checked = false;
193+ hereCheck.checked = false;
194+ nopeCheck.checked = true;
195+ }
196+ }
197+
198+ Label {
199+ anchors.left: parent.left
200+ anchors.right: parent.right
201+ wrapMode: Text.Wrap
202+ text: i18n.tr("You can change your mind later in <b>System Settings</b>.")
203 }
204 }
205
206@@ -90,7 +128,22 @@
207 id: forwardButton
208 LocalComponents.StackButton {
209 text: i18n.tr("Continue")
210- onClicked: pageStack.next()
211+ onClicked: {
212+ var locationOn = gpsCheck.checked || hereCheck.checked;
213+ var gpsOn = gpsCheck.checked || hereCheck.checked;
214+ var hereOn = hereCheck.checked;
215+
216+ // location service doesn't currently listen to updateState
217+ // requests, so we activate the actions if needed.
218+ if (locationActionGroup.location.state != locationOn) {
219+ locationActionGroup.location.activate();
220+ }
221+ if (locationActionGroup.gps.state != gpsOn) {
222+ locationActionGroup.gps.activate();
223+ }
224+ System.hereEnabled = hereOn;
225+ pageStack.next()
226+ }
227 }
228 }
229 }

Subscribers

People subscribed via source and target branches