Skip to content

Fix support for weather providers without "Location" property (fixes NOAA weather add-on)#68

Open
ivanfitenko wants to merge 5 commits intovdb86:masterfrom
ivanfitenko:weather_location_absent_fix
Open

Fix support for weather providers without "Location" property (fixes NOAA weather add-on)#68
ivanfitenko wants to merge 5 commits intovdb86:masterfrom
ivanfitenko:weather_location_absent_fix

Conversation

@ivanfitenko
Copy link
Copy Markdown

Until recently, I was using "Multi Weather" add-on with Kodi, and it was perfectly compatible with digitalclock screensaver. However, with Yahoo closing API access for new users, "Multi Weather" add-on became less useful out or the box, and was even marked as broken in Kodi's repo, so I had to switch to NOAA weather add-on. That, unfortunately, broke the screensaver, and the weather info was not shown anymore.

The issue appears to be caused by digitalclock plugin using "Weather.Location" property as an "umbrella validator" for data provided by weather add-on. This property is not provided by NOAA plugin, and it is not exposed by digitalclock plugin anyway, so with this PR I suggest to switch to explicitly validating the data that is going to be displayed, i.e. Weather.Temperature and Weather.Conditions .

Implementation notes:

  • Unfortunately, "weatherinfoshow" cannot be renamed to something like "weathertempshow", because this will be a breaking change that will disable this setting for existing users and will require them to turn in back on explicitly.
  • Error messages deliberately uses technical style so that a non-technical user better understands that the it is a fault of a weather plugin's internals, and not something on weather provider's side or something that a user can configure inside kodi
  • I took this chance to make temperature and weather condition reports be toggled separately. Not only it enables a user to keep getting the temperature from a hypothetical provider that does not support displaying weather conditions, but it also makes it possible to disable annoying false weather condition reports that are shown instead of temperature (here's my real-life use-case: I live about 20 miles away and 300 feet higher that the airport that reports the weather. I look into the window to know that it's sunny, and I look on my Kodi display to know the temperature, but with 50% probability the temperature will be replaced with long "Mostly Cloudy" message because the airport reports it and the textarea is limited).

  * Remove dependency on "Location" property which is used and which is not nescessarily provided by weather add-ons
@vdb86
Copy link
Copy Markdown
Owner

vdb86 commented Jul 12, 2024

Hello!
Thank you for the pull request.
There's no need to change all translations. only the default English needs to have strings added/changed - then the bot pulls that to the translation platform and updates everything else.

@ivanfitenko
Copy link
Copy Markdown
Author

Thank you for clarification. Changes to non-default language files are removed now.

Comment thread resources/settings.xml Outdated
<setting id="cputemp" label="32266" type="bool" visible="eq(-13,true)" default="false"/>
<setting id="gputemp" label="32267" type="bool" visible="eq(-14,true)" default="false"/>
<setting id="hddtemp" label="32268" type="bool" visible="eq(-15,true)" default="false"/>
<setting id="fps" label="32269" type="bool" visible="eq(-15,true)" default="false"/>
Copy link
Copy Markdown
Owner

@vdb86 vdb86 Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be updated to as right now it's missing in settings due to the change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a really nasty error, sorry! Fixed in commit #ad44e24

@vdb86
Copy link
Copy Markdown
Owner

vdb86 commented Jul 21, 2024

Except for the above comment, we've also got a problem with weather icon:
The weather icon is shown in the screensaver only when the temperature is enabled and the user has selected the weather icon pack in the settings.
In order to select the weather icon pack from the settings, the user needs to enable weather conditions.

So the setting visibility is dependent on weather conditions being enabled, and yet displaying is dependent on user enabling the temperature.
We can easily make the change in the settings to either connect it to temperature or to make it completely independent.
But the issue remains with the code itself where the display of the icon is connected to the temperature.

I personally like how currently the screensaver shows 28C - Mostly cloudy
When they are shown separately it's less ideal for me.

Before you do any more work, let me think if I'm ok with the current change or not.
I know we could introduce a new setting which would allow users to have this info separate or together - but I'm trying not to overcomplicate the settings - especially the additional info tab which already has a bunch of things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants