Merge lp://qastaging/~gary-wzl77/ubuntu-weather-app/fix_1518888 into lp://qastaging/ubuntu-weather-app

PASSED: Continuous integration, rev:194
https://core-apps-jenkins.ubuntu.com/job/weather-app-ci/32/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/304/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/weather-app-ci/32/rebuild

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

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.
>
>
> Furtherm...

FAILED: Continuous integration, rev:195
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/73/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/339/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/73/rebuild

Hi Andrew
1. Does the missing API key cause test run failed?
2. Is there any access limits for following API, at times I didn't get respond from it for a long time
http://api.geonames.org/findNearbyPlaceNameJSON?style=full&username=uweatherdev&lat=51.5047&lng=-0.1283&maxRows=25&featureClass=P

Thanks.

1) Yup, you will require the API keys to OWM/TWC to run the tests, please message me privately if you require them
2) geonames does have a limit and there is a bug 1326918 to migrate to the ubuntu one.

FAILED: Continuous integration, rev:220
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1085/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/642/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/run-ap-tests-ci/1085/rebuild

This looks to be progressing well, is there a silo or debs for the scope so that we can test this? Or are you manually running a the URI-handler from a command? If so do you mind sharing to assist in the testing of the branch?

Hi Andrew
   Sorry for the late response,
   I've been cleaning up the bugs on my hand, The following link you can find a armhf-based test scope, which allows you to open weather app from the scope and check the current temperature for three different locations.
   London(belvedere)
   Berlin(mitte)
   Roma(trevi)

   https://drive.google.com/open?id=0B2H9ECPSSfqIc2xybjlxc0F4SGc
   Please playing around it and verify it on the device.

   Thanks.
   Sorry again for the delay.

P.S due to the API count limitation, sometimes main view didn't navigate to specific location page as we didn't get current temperature information.

PASSED: Continuous integration, rev:221
https://core-apps-jenkins.ubuntu.com/job/weather-app-ci/73/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/971/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/weather-app-ci/73/rebuild

I've installed the scope and this branch on my device.

I've noticed a few functional issues to discuss first:
1) When I open via the scope, it causes all of the views (swiping across) to change to have the hourly view already shown, is this a design decision or an issue? (I wasn't expecting *all* the locations to change to hourly view, maybe only the location I had clicked in the scope)
2) I know you said there were API limitations, but I have not yet been able to select one of the locations in the scopes and then receive the respective location in the app. It always jumps to my 2nd non-location detected location.

Could you describe the expected scenarios here? For example
- if you click on a location and you already have the location, I assume it would jump to it?
- if you click on a location and you don't already have the location, I assume it will add a section - but will this be temporary? or remain in the saved list?

Also are you expecting this to work with both OpenWeatherMap and TheWeatherChannel.

If you want to discuss any of these issues with me, feel free to ping me (ahayzen) on IRC. I'm around in London daytime.

Sorry for the late reply(Just got back from vacation).

Basically, regarding the issue you have that
" It always jumps to my 2nd non-location detected location."
I guess you got the problem by checking the location from scope that app has recorded in db.
Indeed, i can reproduce it and fixed it in the latest commit.

Could you describe the expected scenarios here? For example
- if you click on a location and you already have the location, I assume it would jump to it?
A: Yes
- if you click on a location and you don't already have the location, I assume it will add a section - but will this be temporary? or remain in the saved list?
A:It will add a new location and save it in the location list and navigate to hourlyView to show the location's weather info.

I record a video to show how it works under the scenarios you mentioned above.
https://drive.google.com/open?id=0B2H9ECPSSfqIak42YmU4MGducGs

Pre-condition:
    App only records my current location(Chengdu)

1. Click 'London' in scope to add a new location(Belvedere) in weather app(scenario 2)
2. Click 'Berlin' in scope to add a new location(Mitte) in weather app(scenario 2)
3. Click 'London' again from scope to jump to respective location(Belvedere) in weather app (scenario 1)
4. Click 'Roma' from scope to add a new location(Trevi) in weather app (scenario 2)

P.S:
1.if weather information is not able to be fetched via lat&lng due to api limits, the expected result is "Just open the app and stays at hourly view"
2.During my testing, new feature works fine with two providers(open weather map/the weather channel). In general, as long as backend APIs support to retrieve weather info via latitude and longitude, there would be no problem to add new provider in the future.

As discussed, it seems my issues occur when a manual location (London) has been added via the search functionality in the app.

Otherwise, when I don't have and manual search locations added, this functions as expected :-)

OKay, after taking a closer look, basically it's a bug of the scope, not weather app. I hardcoded the location name("London") for each item in scope. That's why it always jumps to the detected location(London) during your testing.

After the following change was made in scope, it should be working well.
- std::string openUri = "weather://?display=hourly&city=London&lat=" + lat + "&lng=" + lng;
+std::string openUri = "weather://?display=hourly&city=" + loc + "&lat=" + lat + "&lng=" + lng;

I attached the latest scope. Could you please have a check again?
https://drive.google.com/open?id=0B2H9ECPSSfqIZkY4UzhnWGJydzA

Sorry for the noise.
Thanks. :)

OK this is looking better, the only thing i've noticed is that if I have London as a location, when I click London it still jumps to London rather than Belvedere. But I'm thinking that is expected as it is coming from the &city=" + loc + " part of the uri.

So functionally I think this is pretty much there, I'll work on doing a code review next :-)

So testing this I’ve found that the restoration of which index you had previously selected is broken (but it also appears to be in trunk as well?)

Also in the code you have stated "NOTE: potential minor issue: cities with the same name in different countries", I think the best thing todo is report a bug with low priority stating the scenario in which this could happen.

Therefore I'm going to approve this branch, and we can fix the other issues separately.