Code review comment for lp:~ahayzen/ubuntu-weather-app/reboot-fix-ap-location-detect

Revision history for this message
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

« Back to merge proposal