JohnCoates / Aerial

Apple TV Aerial Screensaver for Mac
MIT License
20.76k stars 1.05k forks source link

Weather Overlay test is broken #1272

Closed mbierman closed 1 year ago

mbierman commented 1 year ago

Required information

In order to help us sort your issue, we ask that you provide the following information:

Description of issue / Feature request

  1. Testing the Weather overlay does nothing.
  2. Weather informatoin displayed is wrong. e.g. currently image

    Shown in Aerial

mbierman commented 1 year ago

Ah, suddenly the test did work. Looks like the weather is for the wrong city. It is using Santa Clara, Cuba, not California. santa clara

Settings are for California

image
mbierman commented 1 year ago

Also, the instructions are conflicting. It shows using a state abbreviation but also says not to abbreviate. Note, neither "santa clara, california, USA" nor "santa clara, ca, USA" work.

image

lastly, current location also seems to be broken at the moment.

glouel commented 1 year ago

Hey @mbierman

In general city lookup is powered by OpenWeather and what gets replied can depend from time to time, it seems like they are constantly tweaking their servers. I do get weird results with my city too here in France.

I specifically modified the text yesterday because someone from the US said that doing City, State, USA was the way to go for the USA, but that may not have been correct. It looks like Open Weather's lookup can be weird at time specifically for the US, so I'm not sure what I should even put there.

Regarding current location, be aware that if you open settings via Companion, location will not work unless you allow Companion to use location services in System Settings, on top of doing it for legacyScreenSaver for the regular panel.

Hope that clears things up a bit.

glouel commented 1 year ago

Also, I just did some more testing :

Santa Clara, CA <- says City doesn't exist (you may have to resize the panel a bit if you don't see the message) Santa Clara, USA <- works Santa Clara, CA, USA <- works

I had a 3rd party check those results with same results, so I think the current recommendation (always put , USA at the end, and add state if needed for disambiguation) is correct.

mbierman commented 1 year ago

I specifically modified the text yesterday because someone from the US said that doing City, State, USA was the way to go for the USA, but that may not have been correct. It looks like Open Weather's lookup can be weird at time specifically for the US, so I'm not sure what I should even put there.

Here's the API https://openweathermap.org/api/geocoding-api

image

And note

image

Regarding current location, be aware that if you open settings via Companion, location will not work unless you allow Companion to use location services in System Settings, on top of doing it for legacyScreenSaver for the regular panel.

I was only looking at Aerial itself, not Companion.

USA is not the right country code it must be US and in the US, states must be two letter abbreviations as best I can tell.

https://www.iso.org/iso-3166-country-codes.html

So it is just a documentation issue, fix the help file, and perhaps put some default text as an example? or put it below the field.

glouel commented 1 year ago

Thanks a whole lot for digging in @mbierman I'll fix a bit the documentation and put an example such as :

"Cupertino, CA, US" which should tell everyone in the US how to handle their case.

glouel commented 1 year ago

@mbierman this is the new text :

I think this should solve this issue ? Same example is in placeholder for next build too.

mbierman commented 1 year ago

That looks great! Thank you, thank you!