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

Thanks for your reply.

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.
A: Good suggestion, will fix in next MR update.

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.
A: That's also an idea from David who reviews weather scope. will fix in
weather scope at first.

I suggest that you provide the latitude and longitude into the UriHandler
so use something such as weather://latitude/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
A: weather://51.50853/0.12574/5 format is not that reasonable. I prefer
to keep current implementation(url parameters) by appending geo
information. E.g.
weather://?display=hourly&city=London&lat=51.50853&lng=-0.12574
weather://?display=default&city=London&lat=51.50853&lng=-0.12574

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.
A: That's my general idea for this as well. if location exists, switch to
the relevant page. if not, searching location info based on lat&lng at
firstly, and switch to the regarding page then.

Thanks. Will update the MR later on.

On Wed, Jan 6, 2016 at 8:40 PM, Andrew Hayzen <email address hidden> wrote:

> Review: Needs Fixing
>
> 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
> --
>
> https://code.launchpad.net/~gary-wzl77/ubuntu-weather-app/fix_1518888/+merge/281719
> You are the owner of lp:~gary-wzl77/ubuntu-weather-app/fix_1518888.
>

--
Br
Gary.Wzl