Code review comment for lp:~gary-wzl77/ubuntu-weather-app/fix_1518888

Andrew Hayzen (ahayzen) wrote :

Thanks for starting this :-)

I've found a few issues while looking at the code:
1) You are using Arguments not Connections { target: UriHandler; onOpened: { ... } } which means that the UrlHandler only works when the app is first started, not if the application was already running. Please refer to the docs [0] or music app [1] for how this works.
2) The idea is that this will be run from the scope, however you can search for any location in the scope and you can have any set of locations loaded in the weather app. So I could be on London in the scope, then click to open the weather-app and the weather-app could be at Berlin, this would not be very useful.

I suggest that you provide the latitude and longitude into the UriHandler so use something such as weather://latitiude/longitude/days for example for London hourly this could be weather://51.50853/0.12574/1 or for a 5 day forecast you could then use weather://51.50853/0.12574/5

The weather could the detect if the location exists and switch to the relevant page, or if it doesn't show a temporary view that allows them to view the info, but also save that as a location in their list.

It's probably best that a mail thread is started between the weather app devs, if we want to discuss the protocol design further.

Furthermore could you add yourself to the AUTHORS file and add an entry into the debian/changelog (use the tool $ dch ).

0 - https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.UriHandler/
1 - http://bazaar.launchpad.net/~music-app-dev/music-app/trunk/view/head:/app/components/Helpers/UriHandlerHelper.qml#L29

review: Needs Fixing

« Back to merge proposal