OpenLauncherTeam / openlauncher

Customizable and Open Source Launcher for Android
Apache License 2.0
1.41k stars 413 forks source link

Weather #540

Closed hobleyd closed 3 years ago

hobleyd commented 4 years ago

All,

Here is a big PR, sorry about that. This uses the blank space in the SearchBar to provide a weather lookup if required; I have implemented 2 weather services so far; OpenWeather which is global and the Australian BOM site which is strictly for AU. It will dynamically add icons in to fill the available space.

The AU site does not require an API Key, while the OpenWeather one does. I didn't want to register on your behalf, so the functionality is there to either embed it within the application, or ask the user to specify in the preferences.

I think I have tested most cases; but it is pretty complicated, so I may have missed something. Single clicking on the icons will switch between daily and hourly (AU) or 3-hourly (OpenWeather). Double clicking will open the related Weather app for a full forecast rather than the limited one this provides. Long clicking allows you to select a stored location, location services, or to find a new location from that service. I haven't implemented the ability to remove a stored location as I need to think about the UI for this.

Not sure if this is a problem for you or not, but I used forecastie from F-Droid as the open source preferred OpenWeather app, but this is not available in the Google Store. Not an issue if you have F-Droid installed, but fyi.

Lastly, the Location Services uses the new Google Play APIs - if this is a problem I can revert to the AOSP version, let me know.

Happy to discuss thoughts and concerns as always.

Cheers, David

gsantner commented 4 years ago

@dkanada I'm unsure if it's worth to add location/gps to OpenLauncher - from both battery life and privacy aspects. Whats your thoughts on this?

hobleyd commented 4 years ago

All,

If you request Location Services, it only uses coarse network location so minimal impact to battery life; I don't actually use the fine network location (GPS) as that level of detail is not required for weather forecasts. Additionally, it only queries location once per hour, or if you move more than 25klms, so should not have a big impact there either.

However, Location services are not required at all either unless you want them. You can specify a town or postcode manually and it will work without using location services at all. All at user discretion.

I travel a lot for work, so very helpful for me.

gsantner commented 4 years ago

OK I see, the rough location should be fine. Travel: Understandable use case :D.

As of code: Does it work when you turn location on/off in settings? Or remove permission in e.g. settings? Make sure this whole does no crashes on Launcher when.

To achieve that:

dkanada commented 4 years ago

I'm not too keen on a weather service either, but if the code is clean I think it's fine to merge. I'd much rather improve widget support and just offload calendars, clocks, alarms, and weather to other apps eventually. That would require one or two changes first though.

hobleyd commented 4 years ago

Thanks for your comments:

In terms of turning location services off; Location Services are only requested if you have selected a Weather Service, and not specified a specific location to check (manually). If Location Services are required, it checks the permissions every time it is requested on the assumption that people are likely to say yes if they want to get the weather for a random location. If the permission gets revoked, the next time it tries to display the weather (up to an hour generally) then it will re-request the permission.

I could make this more explicit if you like by putting a specific setting in for Allowing Weather to request location services rather than inferring it from their preferences. Let me know if you would prefer this.

I have renamed the weather icons; they were downloaded from www.flaticon.com which has a free attribution licence which I have put into the README.md file. Is that sufficient do you think? Or would you prefer I add something in to the app somewhere? It looks reasonable based on the licence. I'll also look for CC icons.

In terms of the code breakdown, I am happy to take suggestions here. In essence, all the backend code is localised to the weather subfolder. On the basis of your comment, I have moved the UI code from the SearchBar into WeatherService albeit with tight coupling between the two classes given the UI code needs to be tied in with the SearchBar concepts. Happy to make further changes, if you have any specific requirements.

gsantner commented 4 years ago

I made another review @hobleyd Sorry if annoying, but lets get it into good shape :-)

hobleyd commented 4 years ago

Not a problem at all @gsantner I want to do this properly.

I think I have fixed up all the things you mentioned previously - I can't see any new requests. Have I missed anything?

hobleyd commented 4 years ago

@gsantner , I have made all those changes (with one query). I am flying back to Australia tomorrow morning so will be out of action for a day (just in case you are wondering why I don't respond to updates).

hobleyd commented 4 years ago

@gsantner , my humblest apologies. I should have regression tested this more fully obviously. I believe I have everything working as you would expect now.

I have moved out the Weather City from the preferences as I can't easily put a drop down on a the v7 support preferences; replaced it with an explicit UseLocationServices preference; if UseLocationServices is not set and the Weather Service has been requested, it will prompt for a city which is searched via the relevant Weather Service.

gsantner commented 4 years ago

No problem at all!

After all that's why I'm doing this reviews and help to improve it!

hobleyd commented 4 years ago

@gsantner, One question - have you signed up with OpenWeatherMap for a default API key? Or would you expect people to do that themselves? If the latter, I'll need to do some more work with error handling for that scenario. I'll get to that in the next day or two.

hobleyd commented 4 years ago

Sorry for the delay - I have been a bit distracted by work and a couple of other projects; I actually have Android Studio open to this today and will be looking at it this week.

gsantner commented 3 years ago

What is the status on this?

hobleyd commented 3 years ago

I'm trying to track down an irritating layout bug which I am having difficulty reproducing.

gsantner commented 3 years ago

Closing due no response and inactivity.