Merge lp:~ahayzen/ubuntu-weather-app/reboot-fix-ap-location-detect into lp:ubuntu-weather-app

Proposed by Andrew Hayzen on 2015-07-26
Status: Merged
Approved by: Victor Thompson on 2015-07-27
Approved revision: 79
Merged at revision: 70
Proposed branch: lp:~ahayzen/ubuntu-weather-app/reboot-fix-ap-location-detect
Merge into: lp:ubuntu-weather-app
Diff against target: 413 lines (+155/-28)
10 files modified
app/components/CurrentLocation.qml (+18/-5)
app/ubuntu-weather-app.qml (+25/-4)
app/ui/LocationsPage.qml (+3/-2)
app/ui/SettingsPage.qml (+5/-0)
app/ui/settings/LocationPage.qml (+36/-0)
debian/changelog (+2/-0)
po/com.ubuntu.weather.pot (+19/-11)
tests/autopilot/ubuntu_weather_app/databases/CMakeLists.txt (+1/-1)
tests/autopilot/ubuntu_weather_app/databases/location_added.conf (+3/-0)
tests/autopilot/ubuntu_weather_app/tests/__init__.py (+43/-5)
To merge this branch: bzr merge lp:~ahayzen/ubuntu-weather-app/reboot-fix-ap-location-detect
Reviewer Review Type Date Requested Status
Victor Thompson 2015-07-26 Approve on 2015-07-27
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve on 2015-07-27
Review via email: mp+265908@code.launchpad.net

Commit message

* Add auto detect location option in the settings
* Set auto detect location to false when running autopilot

Description of the change

* Add auto detect location option in the settings
* Set auto detect location to false when running autopilot

To post a comment you must log in.
71. By Andrew Hayzen on 2015-07-26

* Fixes for PEP8

72. By Andrew Hayzen on 2015-07-26

* Add changelog

Victor Thompson (vthompson) wrote :

There's a few things we'll need to fix.

1. Currently you only hide the current location from the LocationsPage. However, the location is still in the horizontal listview.
2. Perhaps because of #1, when you click on a city from the LocationsPage you get the wrong one.
3. If PositionSource is inactive, do we still need to also check the settings.autoDetectLocation to prevent adding a new location? Won't we not get an update when it's inactive?
4. Let's change "Auto detect location" to "Detect current location"
5. I'm on the fence about this one, but would "Privacy" be better labeled as "Location"? I understand why we'd call it "Privacy", but perhaps "Location" would make it seem less likely to be malicious.
6. Do we need to explicitly refresh when we change the autoDetectLocation setting? Won't getting a new location from PositionSource do this for us?

review: Needs Fixing
73. By Andrew Hayzen on 2015-07-26

* Change to detectCurrentLocation and use lazy refresh

Andrew Hayzen (ahayzen) wrote :

1) This is working for me please retest
2) As in response to #1
3) Not sure which case you are referring to?
4) Changed to "Detect current location"
5) Changed to "Location"
6) I've changed it to lazy refresh, but we need to call the refresh to ensure in the true->false case it is removed from the location list

74. By Andrew Hayzen on 2015-07-26

* Fix for wrong currentIndex when auto detect location is false and selecting from the LocationsPage

Victor Thompson (vthompson) wrote :

Currently the swipe to remove and the reordering (and delete) of the locations in the LocationsPage seems to have broken with this MP.

review: Needs Fixing
75. By Andrew Hayzen on 2015-07-26

* Fixes for move and remove when without location

Andrew Hayzen (ahayzen) wrote :

I believe this has been fixed, please test both cases of with/without the auto location :-)

Victor Thompson (vthompson) wrote :

When there are no locations and we have location detection off, using the bottom edge is a bit weird. This probably wasn't introduced by your MP, but in the future we should consider disabling the bottom edge hint when we show the button to manually add.

Things to fix:
1. IMO we should make the new setting checkbox toggle when the list item is pressed, rather than requiring the user tap the checkbox. If you have a different opinion let me know.
2. When I try to delete a location using the multiselect I get the following error and the location(s) are not removed:

file:///opt/click.ubuntu.com/com.ubuntu.weather/3.0.75/share/qml/ubuntu-weather-app.qml:288: TypeError: Cannot read
property 'db' of undefined^

review: Needs Fixing
Victor Thompson (vthompson) wrote :

For that last item, I think it only happens if you remove the last location--so it's probably the same offset issue as the others.

76. By Andrew Hayzen on 2015-07-26

* Selecting the listitem now flips the control

77. By Andrew Hayzen on 2015-07-26

* Fix for setting not being saved when selecting the listitem portion

78. By Andrew Hayzen on 2015-07-27

* Fix for multiselect breaking

Andrew Hayzen (ahayzen) wrote :

Multiselect delete has been fixed and the checkbox is not flipped when you click *anywhere* in the listitem, please retest :-)

79. By Andrew Hayzen on 2015-07-27

* Change to actually removing the old detected location from the db

Victor Thompson (vthompson) wrote :

lgtm! AP passes locally and on the device.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/components/CurrentLocation.qml'
2--- app/components/CurrentLocation.qml 2015-06-18 01:42:03 +0000
3+++ app/components/CurrentLocation.qml 2015-07-27 00:58:09 +0000
4@@ -41,7 +41,7 @@
5 }
6
7 function searchResponseHandler(msgObject) {
8- if (!msgObject.error) {
9+ if (!msgObject.error && settings.detectCurrentLocation) {
10 console.log("Loc to add:", JSON.stringify(msgObject.result.locations[0]))
11 storage.updateCurrentLocation(msgObject.result.locations[0])
12 }
13@@ -51,7 +51,7 @@
14 PositionSource {
15 id: currentPosition
16 updateInterval: 1000
17- active: true
18+ active: settings.detectCurrentLocation
19
20 onPositionChanged: {
21 var coord = currentPosition.position.coordinate
22@@ -75,9 +75,22 @@
23 onCountChanged: {
24 // Update the currentLocation if one is found and it does not match the stored location
25 if (count > 0 && currentLocation.string !== geocodeModel.get(0).address.city) {
26- var loc = geocodeModel.get(0)
27- currentLocation.string = loc.address.city
28- searchForLocation(loc.coordinate.latitude, loc.coordinate.longitude)
29+ search();
30+ }
31+ }
32+
33+ function search() {
34+ var loc = geocodeModel.get(0)
35+ currentLocation.string = loc.address.city
36+ searchForLocation(loc.coordinate.latitude, loc.coordinate.longitude)
37+ }
38+ }
39+
40+ Connections {
41+ target: settings
42+ onDetectCurrentLocationChanged: {
43+ if (settings.detectCurrentLocation) {
44+ geocodeModel.search();
45 }
46 }
47 }
48
49=== modified file 'app/ubuntu-weather-app.qml'
50--- app/ubuntu-weather-app.qml 2015-07-25 22:06:50 +0000
51+++ app/ubuntu-weather-app.qml 2015-07-27 00:58:09 +0000
52@@ -132,6 +132,7 @@
53 */
54 property int current: 0
55
56+ property bool detectCurrentLocation: true
57 property int refreshInterval: 1800
58 property string precipUnits
59 property string service
60@@ -142,6 +143,17 @@
61 property bool addedCurrentLocation: false
62 property bool migrated: false
63
64+ onDetectCurrentLocationChanged: {
65+ if (!detectCurrentLocation) {
66+ if (addedCurrentLocation) {
67+ storage.removeLocation(-1); // indexes are increased by 1
68+ addedCurrentLocation = false;
69+ }
70+
71+ refreshData();
72+ }
73+ }
74+
75 Component.onCompleted: {
76 if (units === "") { // No settings so load defaults
77 console.debug("No settings, using defaults")
78@@ -233,8 +245,10 @@
79
80 function moveLocation(from, to) {
81 // Indexes are offset by 1 to account for current location
82- from += 1
83- to += 1
84+ if (settings.addedCurrentLocation) {
85+ from += 1
86+ to += 1
87+ }
88
89 // Update settings to respect new changes
90 if (from === settings.current) {
91@@ -253,7 +267,10 @@
92 // Remove a location from the list
93 function removeLocation(index) {
94 // Indexes are offset by 1 to account for current location
95- index += 1
96+ if (settings.addedCurrentLocation) {
97+ index += 1
98+ }
99+
100 if (settings.current >= index) { // Update settings to respect new changes
101 settings.current -= settings.current;
102 }
103@@ -279,7 +296,11 @@
104 var locations = []
105
106 for (i=0; i < indexes.length; i++) {
107- locations.push(locationsList[indexes[i] + 1].db.id)
108+ if (settings.addedCurrentLocation) {
109+ locations.push(locationsList[indexes[i] + 1].db.id)
110+ } else {
111+ locations.push(locationsList[indexes[i]].db.id)
112+ }
113 }
114
115 storage.clearMultiLocation(locations);
116
117=== modified file 'app/ui/LocationsPage.qml'
118--- app/ui/LocationsPage.qml 2015-06-28 19:04:43 +0000
119+++ app/ui/LocationsPage.qml 2015-07-27 00:58:09 +0000
120@@ -50,7 +50,7 @@
121 removable: true
122 thisPage: locationsPage
123
124- onRemoved: storage.removeMultiLocations(selectedItems.slice())
125+ onRemoved: storage.removeMultiLocations(selectedItems.slice());
126 }
127 ]
128
129@@ -72,7 +72,7 @@
130 left: parent.left
131 right: parent.right
132 }
133- height: settings.addedCurrentLocation ? units.gu(8) : units.gu(0)
134+ height: settings.addedCurrentLocation && settings.detectCurrentLocation ? units.gu(8) : units.gu(0)
135 interactive: false
136 model: currentLocationModel
137 delegate: WeatherListItem {
138@@ -160,6 +160,7 @@
139
140 onItemClicked: {
141 settings.current = index + 1;
142+
143 pageStack.pop()
144 }
145 onReorder: {
146
147=== modified file 'app/ui/SettingsPage.qml'
148--- app/ui/SettingsPage.qml 2015-06-28 22:54:14 +0000
149+++ app/ui/SettingsPage.qml 2015-07-27 00:58:09 +0000
150@@ -42,5 +42,10 @@
151 title: i18n.tr("Refresh Interval")
152 onClicked: mainPageStack.push(Qt.resolvedUrl("settings/RefreshIntervalPage.qml"))
153 }
154+
155+ StandardListItem {
156+ title: i18n.tr("Location")
157+ onClicked: mainPageStack.push(Qt.resolvedUrl("settings/LocationPage.qml"))
158+ }
159 }
160 }
161
162=== added file 'app/ui/settings/LocationPage.qml'
163--- app/ui/settings/LocationPage.qml 1970-01-01 00:00:00 +0000
164+++ app/ui/settings/LocationPage.qml 2015-07-27 00:58:09 +0000
165@@ -0,0 +1,36 @@
166+/*
167+ * Copyright (C) 2015 Canonical Ltd
168+ *
169+ * This file is part of Ubuntu Weather App
170+ *
171+ * Ubuntu Weather App is free software: you can redistribute it and/or modify
172+ * it under the terms of the GNU General Public License version 3 as
173+ * published by the Free Software Foundation.
174+ *
175+ * Ubuntu Weather App is distributed in the hope that it will be useful,
176+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
177+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
178+ * GNU General Public License for more details.
179+ *
180+ * You should have received a copy of the GNU General Public License
181+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
182+ */
183+
184+import QtQuick 2.4
185+import Ubuntu.Components 1.2
186+import Ubuntu.Components.ListItems 1.0 as ListItem
187+
188+Page {
189+ title: i18n.tr("Location")
190+
191+ ListItem.Standard {
192+ control: CheckBox {
193+ checked: settings.detectCurrentLocation
194+
195+ onCheckedChanged: settings.detectCurrentLocation = checked;
196+ }
197+ text: i18n.tr("Detect current location")
198+
199+ onClicked: control.checked = !control.checked
200+ }
201+}
202
203=== modified file 'debian/changelog'
204--- debian/changelog 2015-07-25 21:53:40 +0000
205+++ debian/changelog 2015-07-27 00:58:09 +0000
206@@ -10,6 +10,8 @@
207
208 [ Andrew Hayzen ]
209 * Add mocked locations for autopilot and add a test using the data
210+ * Add setting to disable auto detecting location
211+ * When running under autopilot do not auto detect location
212
213 -- Victor Thompson <victor.thompson@gmail.com> Mon, 01 Jun 2015 20:11:23 -0500
214
215
216=== modified file 'po/com.ubuntu.weather.pot'
217--- po/com.ubuntu.weather.pot 2015-07-18 19:08:21 +0000
218+++ po/com.ubuntu.weather.pot 2015-07-27 00:58:09 +0000
219@@ -8,7 +8,7 @@
220 msgstr ""
221 "Project-Id-Version: ubuntu-weather-app\n"
222 "Report-Msgid-Bugs-To: \n"
223-"POT-Creation-Date: 2015-07-18 18:04+0100\n"
224+"POT-Creation-Date: 2015-07-27 01:55+0100\n"
225 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
226 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
227 "Language-Team: LANGUAGE <LL@li.org>\n"
228@@ -18,31 +18,31 @@
229 "Content-Transfer-Encoding: 8bit\n"
230 "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n"
231
232-#: ../app/components/DayDelegate.qml:193
233+#: ../app/components/DayDelegate.qml:199
234 msgid "Chance of rain"
235 msgstr ""
236
237-#: ../app/components/DayDelegate.qml:202
238+#: ../app/components/DayDelegate.qml:205
239 msgid "Winds"
240 msgstr ""
241
242-#: ../app/components/DayDelegate.qml:209
243+#: ../app/components/DayDelegate.qml:212
244 msgid "UV Index"
245 msgstr ""
246
247-#: ../app/components/DayDelegate.qml:214
248+#: ../app/components/DayDelegate.qml:217
249 msgid "Pollen"
250 msgstr ""
251
252-#: ../app/components/DayDelegate.qml:220
253+#: ../app/components/DayDelegate.qml:223
254 msgid "Humidity"
255 msgstr ""
256
257-#: ../app/components/DayDelegate.qml:226
258+#: ../app/components/DayDelegate.qml:229
259 msgid "Sunrise"
260 msgstr ""
261
262-#: ../app/components/DayDelegate.qml:232
263+#: ../app/components/DayDelegate.qml:235
264 msgid "Sunset"
265 msgstr ""
266
267@@ -66,11 +66,11 @@
268 msgid "Cancel selection"
269 msgstr ""
270
271-#: ../app/ubuntu-weather-app.qml:168
272+#: ../app/ubuntu-weather-app.qml:184
273 msgid "Searching for current location..."
274 msgstr ""
275
276-#: ../app/ubuntu-weather-app.qml:177
277+#: ../app/ubuntu-weather-app.qml:193
278 msgid "Add a manual location"
279 msgstr ""
280
281@@ -122,7 +122,7 @@
282 msgid "Units"
283 msgstr ""
284
285-#: ../app/ui/SettingsPage.qml:37 ../app/ui/settings/DataProviderPage.qml:24
286+#: ../app/ui/SettingsPage.qml:37 ../app/ui/settings/DataProviderPage.qml:25
287 msgid "Data Provider"
288 msgstr ""
289
290@@ -130,10 +130,18 @@
291 msgid "Refresh Interval"
292 msgstr ""
293
294+#: ../app/ui/SettingsPage.qml:47 ../app/ui/settings/LocationPage.qml:24
295+msgid "Location"
296+msgstr ""
297+
298 #: ../app/ui/settings/DataProviderPage.qml:37
299 msgid "Provider"
300 msgstr ""
301
302+#: ../app/ui/settings/LocationPage.qml:32
303+msgid "Detect current location"
304+msgstr ""
305+
306 #: ../app/ui/settings/RefreshIntervalPage.qml:30
307 #: ../app/ui/settings/RefreshIntervalPage.qml:31
308 #: ../app/ui/settings/RefreshIntervalPage.qml:32
309
310=== modified file 'tests/autopilot/ubuntu_weather_app/databases/CMakeLists.txt'
311--- tests/autopilot/ubuntu_weather_app/databases/CMakeLists.txt 2015-07-25 13:52:18 +0000
312+++ tests/autopilot/ubuntu_weather_app/databases/CMakeLists.txt 2015-07-27 00:58:09 +0000
313@@ -1,6 +1,6 @@
314 # make the database files visible on qtcreator
315 file(GLOB PYTHON_TEST_DATABASE_FILES
316 RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}
317- *.ini)
318+ *.ini *.conf)
319
320 add_custom_target(ubuntu-weather-app_PYTHONTestDatabaseFiles ALL SOURCES ${PYTHON_TEST_DATABASE_FILES})
321
322=== added file 'tests/autopilot/ubuntu_weather_app/databases/location_added.conf'
323--- tests/autopilot/ubuntu_weather_app/databases/location_added.conf 1970-01-01 00:00:00 +0000
324+++ tests/autopilot/ubuntu_weather_app/databases/location_added.conf 2015-07-27 00:58:09 +0000
325@@ -0,0 +1,3 @@
326+[weatherSettings]
327+detectCurrentLocation=false
328+migrated=true
329
330=== modified file 'tests/autopilot/ubuntu_weather_app/tests/__init__.py'
331--- tests/autopilot/ubuntu_weather_app/tests/__init__.py 2015-07-23 00:50:12 +0000
332+++ tests/autopilot/ubuntu_weather_app/tests/__init__.py 2015-07-27 00:58:09 +0000
333@@ -223,7 +223,7 @@
334
335 return result
336
337- def load_vars(self):
338+ def load_database_vars(self):
339 self.app_dir = os.path.join(
340 os.environ.get('HOME'),
341 ".local/share/com.ubuntu.weather")
342@@ -235,30 +235,68 @@
343 os.path.join(os.path.dirname(__file__), '..', 'files'))
344
345
346-class UbuntuWeatherAppTestCase(BaseTestCaseWithPatchedHome, DatabaseMixin):
347+class SettingsMixin(object):
348+
349+ """
350+ Helper functions for dealing with the settings file
351+ """
352+
353+ def create_settings_with_location_added(self):
354+ logger.debug("Creating settings with location added")
355+
356+ if not os.path.exists(self.settings_dir):
357+ os.makedirs(self.settings_dir)
358+
359+ shutil.copyfile(self.settings_location_added, self.settings_filepath)
360+
361+ self.assertThat(
362+ lambda: os.path.exists(self.settings_filepath),
363+ Eventually(Equals(True)))
364+
365+ def load_settings_vars(self):
366+ self.db_dir = os.path.abspath(
367+ os.path.join(os.path.dirname(__file__), '..', 'databases'))
368+ self.settings_dir = os.path.join(
369+ os.environ.get('HOME'),
370+ ".config/com.ubuntu.weather")
371+ self.settings_filepath = os.path.join(self.settings_dir,
372+ 'com.ubuntu.weather.conf')
373+ self.settings_location_added = os.path.join(self.db_dir,
374+ "location_added.conf")
375+
376+
377+class UbuntuWeatherAppTestCase(BaseTestCaseWithPatchedHome, DatabaseMixin,
378+ SettingsMixin):
379
380 """Base test case that launches the ubuntu-weather-app."""
381
382 def setUp(self):
383 super(UbuntuWeatherAppTestCase, self).setUp()
384
385- self.load_vars()
386+ self.load_database_vars()
387 self.create_blank_db()
388
389+ self.load_settings_vars()
390+ self.create_settings_with_location_added()
391+
392 self.app = UbuntuWeatherApp(self.launcher())
393
394
395 class UbuntuWeatherAppTestCaseWithData(BaseTestCaseWithPatchedHome,
396- DatabaseMixin):
397+ DatabaseMixin,
398+ SettingsMixin):
399
400 """Base test case that launches the ubuntu-weather-app with data."""
401
402 def setUp(self):
403 super(UbuntuWeatherAppTestCaseWithData, self).setUp()
404
405- self.load_vars()
406+ self.load_database_vars()
407 self.create_blank_db()
408
409+ self.load_settings_vars()
410+ self.create_settings_with_location_added()
411+
412 logger.debug("Adding fake data to new database")
413 self.add_locations_to_database()
414

Subscribers

People subscribed via source and target branches

to all changes: