bogstandard / rl-weather

Weather plugin for RuneLite
BSD 2-Clause "Simplified" License
9 stars 3 forks source link

Add support for location-based weather #5

Closed thyme4soup closed 2 years ago

thyme4soup commented 2 years ago

Location-Based Weather

Thank you for such a great plugin! I enjoyed it a lot and thought using a weather api would be a good fit. I figured the best way would be to have a user-supplied key so I implemented it using the OpenWeatherMap API. This supplies the user-provided location directly to the API, so location could be something like city, city and country, or postal code (US).

I figured the way to provide error/success feedback was through the console, formatting could be improved for errors but wanted to provide something that could let users debug their settings if needed. Let me know what you think!

New Settings

Location enable/disable (slider) Location (String) API Key (String)

Behavior

Location is not enabled

Default to other settings

Location is enabled but API inputs are invalid

Send message to console and disable weather

Location is enabled and API inputs are valid

Send a success message to chat and enable weather (rain, snow, or thunder) if regular settings are enabled

Example of settings

settings

Example of console outputs

console

bogstandard commented 2 years ago

Thanks for the thanks, and for doing all the work on this feature! I'll be reviewing this over the coming week but its already looking like something I'm happy to pull in. Great work.

I'm going to be fixing (or removing) the sound effects that have been broken since launch in the next update pushed to the runelite-hub, so I think this feature would be good to bundle in along-side that when its done.

Sorry if I'm a bit slow reviewing & accepting this pull request btw, life's busy and it's Christmas too 😉 , but all-in-all, looks good.

My only thoughts is whether we could instead show the success notes/errors within the config pane rather than the chat, but that's no big deal. I'm not sure on the advice Runelite gives to devs on reporting feedback to users.

bogstandard commented 2 years ago

Hi Mate, I've made some changes to tidy things up a bit and prevent the OpenWeatherAPI being hammered too hard. Since people are putting their possibly-billed keys into this I think its better to be safe and slow with the requests, just in-case!

Summary of changes:

  1. Changed /forecast to /weather, so we use the current weather not the long forecast.
  2. Changed MAX_STALENESS to approximately three minutes. Every ~15 seconds felt like too many API requests. (fyi GameTicks are ~0.6 seconds, not 1 second)
  3. Made the Config Panel text fit on the screen a bit better (maybe one day RuneLite will allow us newlines 🤭).
  4. General refactor, while the API gives us lots of values, we only really need the weather[0].id value!

I'll be testing the functionality thoughtout the week while I try and fix the sound effects issues.

Once that's done I'll dunk it into the public release!

thyme4soup commented 2 years ago

Awesome, thanks for the updates!

Gametick makes sense, I had assumed it was 1 sec 😖. Also big ups on the api changes, I hadn't realized I was pulling the full forecast!

bogstandard commented 2 years ago

This hasn't been forgotten about, aim to merge this in and release in the New Year! My attempts to fix the sound effects have failed, but I won't let that delay this separate feature further than the Holidays.

bogstandard commented 2 years ago

Merged into master & PR made to update the plugin for public users on the Plugin Hub. Closing this pull request now. Happy new year!

Jin-Jiyunsun commented 2 years ago

Any eta on this? Super excited to try it!

bogstandard commented 2 years ago

Hi Jin, I'm hoping by next week. The delays are from having trouble getting it merged into the Plugin Hub because of the new third-party dependencies we're introducing & I've been too busy to properly look at it 😟

Working on it tonight, so hopefully I have some better luck!

Thanks for your interest in this, it's great to know people are enjoying the plugin!

thyme4soup commented 2 years ago

Ah I see the plugin hub PR, probably would have been better to avoid the API dependencies my bad 😅

bogstandard commented 2 years ago

Haha, not a problem mate :) took me by surprise too, we both should have read the Hub readme better! Lesson learnt eh! 😄

We'll see how the maintainers respond & refactor if needed

Jin-Jiyunsun commented 2 years ago

Did this just die :(? nothing seems to have come from it

thyme4soup commented 2 years ago

We're waiting on approval here https://github.com/runelite/plugin-hub/pull/2257 so it's whenever that goes through

bogstandard commented 2 years ago

Hi both, as per the Maintainer's remarks I'll be adjusting this to be using okhttp + gson instead of retrofit to save them time & to avoid them adding another third-party dependency to their stack.

I'm hoping to find time for this in the week, but if anyones happy to beat me to the punch; go for it.

thyme4soup commented 2 years ago

Makes sense, I'll also see if I can take a crack this week

bogstandard commented 2 years ago

Hi @thyme4soup I've beaten you to it 🤭

This is the commit with the important changes, there's a few later commits that are superficial things. All of these are pushed & sat on the master branch of this repo.

If you've time, do you mind casting an eye over it &/or pulling down & running a test your end to check its all okay? I think I've done it right. I've been using https://weather-radar-live.com/rain-radar/ to check different locations (The 3 minute time out is a bit annoying but a necessity!).

If you've not got the time I'll push this up to the Hub anyway in the tail end of the week after a few more tests!

thyme4soup commented 2 years ago

Cheers! I wasn't looking forward to it but looks like the change wasn't too bad! I don't have my workspace set up at the moment to test, but it looks good to me. I think reusing the Model thru gson is good news as all the logic is preserved. Also the error handling looks the same as it was so if we tested an invalid location and API key I'd be confident in the change

I think only note looks like we can remove URL or change it to HOST unless I'm missing it somewhere else

bogstandard commented 2 years ago

Hi All, just an update; those new dependencies have been removed & it's passing build checks on the Hub's repo, so hopefully we'll be smooth sailing from here. It's back over to the maintainers of the Hub now.

Also during this fix I put in a config change event watcher meaning; users don't have to wait the full ~3 minutes to see things update.

Apologies for the few days delay on this one!

Jin-Jiyunsun commented 2 years ago

Any update on this, sad this hasn't been merged by the main devs yet :(

bogstandard commented 2 years ago

Hi Jin, I've nudged the maintainers, hopefully soon!

Jin-Jiyunsun commented 2 years ago

Guess this is crabbed?

bogstandard commented 2 years ago

Good news all (@Jin-Jiyunsun and @thyme4soup) this has been merged by the Plugin Hub maintainers!!

Many thanks to all involved & for your support and patience with this, crazy that it's been rolling since Christmas but life is very busy!

Think I'll take us to Varrock tavern for a well earned pint!

thyme4soup commented 2 years ago

🚀🚀🚀🚀