[astro] Refresh zodiac handling

[clinique]

This PR is a review of the Zodiac handling in the Astro Binding.
It adds an AstroIconProvider, furnishing appropriate icons for Zodiac signs.
It unifies Zodiac calc method for Sun and Moon. Unit tests comparing old method toward new method added.
It introduces usage of Instant toward Calendar and according unit tests.
Common astro constants, methods, extracted from xxxCalc classes into AstroConstants and MathUtils classes.

This PR holds many changes and serves the following goals:

• Start to simplify this addon
• Introduce Instant in favor of Calendar

I've tried to cover changes as possible as doable with tests to ensure no breaking change introduced.

[Copilot]

Pull request overview

This PR refactors zodiac handling in the Astro binding, introducing modern Java time APIs and consolidating calculation methods while adding visual enhancements through icon support.

• Introduces Instant API in favor of Calendar for time handling
• Unifies zodiac calculation logic across Sun and Moon with new ZodiacCalc class
• Adds comprehensive icon provider with SVG zodiac sign icons

Reviewed changes

Copilot reviewed 26 out of 52 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
DateTimeUtilsTest.java	Adds unit tests for new Instant-based utility methods
SunZodiacCalcTest.java	New test comparing old date-based vs new position-based zodiac calculation
MoonZodiacCalcTest.java	Moves moon zodiac test from MoonCalcTest to dedicated test class
zodiac*.svg	Adds 13 SVG icons for zodiac signs and default zodiac icon
channels.xml	Adds category for zodiac sign channel
astro.properties	Adds i18n entries for icon provider
PropertyUtils.java	Adds support for Instant type in state conversion
MathUtils.java	New utility class extracting common trigonometric functions
DateTimeUtils.java	Adds Instant-based methods and consolidates Julian date constants
AstroConstants.java	New class for common astronomical constants
ZodiacSign.java	Adds helper method for radians per zodiac sign
Zodiac.java	Refactors to include start/end Instant fields and validation
Sun.java/Moon.java	Changes zodiac field type from nullable SunZodiac to Zodiac
ZodiacCalc.java	New unified zodiac calculator using ecliptic longitude
SunCalc.java/MoonCalc.java/SeasonCalc.java	Refactors to use MathUtils and unified ZodiacCalc
Job.java/DailyJobSun.java/AstroThingHandler.java	Adds Instant-based job scheduling support
AstroIconProvider.java	New icon provider for dynamic zodiac sign icons
README.md	Documents available zodiac icons
NOTICE	Adds attribution for zodiac icon set

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Nadahar]

It introduces usage of Instant toward Calendar and according unit tests.

I haven't studied the code, but I will just mention that this introduces a lot of possibilities for bugs. Are you sure that you want to do that in combination with other changes? I would think that moving the Instant would be more than enough to tackle in a PR alone.

Both Instant and Calendar are notorious for relying on the default time zone and locale, which is bad in most cases. The defaults might not correspond to what OH is configured for, and the default time zone might not correspond to the configured location at all. I recently did a PR trying to address "the worst" of these issues in the binding, but I'm in no doubt that there are more in here. When moving to Instant you run the risk of introducing new reliance on the defaults (I think that locale is less problematic with the "new types", but time zone still very much is).

I've added warnings for the use of these defaults in the next version of the SAT plugin. It's not released yet, but the snapshot exists, so I would recommend using that for test-builds when trying to replace this, by using SAT plugin 0.18.0-SNAPSHOT instead of 0.17.0. That will probably issue a lot of warnings.

edit: Come to think of it, I'm not sure if 0.18.0-SNAPSHOT is published somewhere or not, I have built it locally, so it's available for me regardless.

[Nadahar]

I checked out this PR locally and ran the "new" SAT plugin, the results actually aren't bad (I don't know if any of the warnings stems from code from this PR):

org/openhab/binding/astro/internal/AstroIconProvider.java:

Rule	Violation	Line
ImplicitDefaultLocale	Avoid method with implicit default Locale: sign.name().toLowerCase()	91
RelianceOnDefaultCharset	Specify a character set instead of relying on the default charset	106

org/openhab/binding/astro/internal/action/AstroActions.java:

Rule	Violation	Line
ImplicitDefaultTimeZone	Avoid method with implicit default time zone: ZonedDateTime.now()	74
ImplicitDefaultTimeZone	Avoid method with implicit default time zone: ZonedDateTime.now()	87
ImplicitDefaultTimeZone	Avoid method with implicit default time zone: ZonedDateTime.now()	101
ImplicitDefaultLocale	Avoid method with implicit default Locale: phaseName.toUpperCase()	122
ImplicitDefaultTimeZone	Avoid method with implicit default time zone: ZonedDateTime.now()	123

org/openhab/binding/astro/internal/handler/AstroThingHandler.java:

Rule	Violation	Line
ImplicitDefaultLocale	Avoid method with implicit default Locale: new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss")	78

[lsiepel]

Looked at allmost all files. Need another round to finish.

[clinique]

It introduces usage of Instant toward Calendar and according unit tests.

I haven't studied the code, but I will just mention that this introduces a lot of possibilities for bugs. Are you sure that you want to do that in combination with other changes? I would think that moving the Instant would be more than enough to tackle in a PR alone.

Both Instant and Calendar are notorious for relying on the default time zone and locale, which is bad in most cases. The defaults might not correspond to what OH is configured for, and the default time zone might not correspond to the configured location at all. I recently did a PR trying to address "the worst" of these issues in the binding, but I'm in no doubt that there are more in here. When moving to Instant you run the risk of introducing new reliance on the defaults (I think that locale is less problematic with the "new types", but time zone still very much is).

I did it with Zodiac on purpose because:
1°/ Zodiac is not related for its calculation to locale nor timezone, it is only related to relative position of sun/moon toward earth.
2°/ I tend to think that this is nowhere business critical for any automation

[clinique]

I checked out this PR locally and ran the "new" SAT plugin, the results actually aren't bad (I don't know if any of the warnings stems from code from this PR):

I adressed those coming from this PR and some other but not all.

[lsiepel]

Thanks, LGTM

[Nadahar]

I adressed those coming from this PR and some other but not all.

Great, much fewer warnings now. I made #19866 to address the rest, but as always, there are complications. Why don't I ever learn not to stick my nose into such things 😉

[clinique]

I adressed those coming from this PR and some other but not all.

Great, much fewer warnings now. I made #19866 to address the rest, but as always, there are complications. Why don't I ever learn not to stick my nose into such things 😉

In the Asto, there are many places where your nose can be sticked !

[Nadahar]

It seems like we missed something here - the introduction of Instant isn't as trivial as it might sound. This method in AstroThingHandler for example, will generate log entries that can lead to a lot of confusion:

    public void schedule(Job job, Instant eventAt) {
        long sleepTime = Instant.now().until(eventAt, ChronoUnit.MILLIS);
        schedule(job, sleepTime);
        logger.debug("Scheduled {} in {}ms (at {})", job, sleepTime, eventAt);
    }

What must be remembered with Instant is that it is, by definition, in UTC. So, every timestamp you output/print will have to be converted to a relevant time zone before being logged - or be very confusing. If all timestamps in logs were in UTC it would be one thing, but that's not the case, the vast majority is in the default time zone. Understanding that some all of a sudden is in a different time zone thus isn't very intuitive.

Further, if there are isSameDay() and similar methods using Instant, they will be wrong for anybody that doesn't happen to use UTC.

[lsiepel]

What must be remembered with Instant is that it is, by definition, in UTC. So, every timestamp you output/print will have to be converted to a relevant time zone before being logged - or be very confusing. If all timestamps in logs were in UTC it would be one thing, but that's not the case, the vast majority is in the default time zone. Understanding that some all of a sudden is in a different time zone thus isn't very intuitive.

Further, if there are isSameDay() and similar methods using Instant, they will be wrong for anybody that doesn't happen to use UTC.

Good catch.
We could use the TimeZoneProvider to print this correctly in the logfile, but I’m wondering whether the log timestamps are based on the openHAB-configured timezone or the system timezone.
Hopefully those are aligned, but if not, this could become quite annoying.

[Nadahar]

We could use the TimeZoneProvider to print this correctly in the logfile, but I’m wondering whether the log timestamps are based on the openHAB-configured timezone or the system timezone.
Hopefully those are aligned, but if not, this could become quite annoying.

I think we'll have to "accept" that logs are using the default timezone. Otherwise, we'll have to inject TimeZoneProvider "everywhere". So, I'll say that for logging, the default time zone should be the standard.

edit: The new SAT checks doesn't "ban" using the default time zone - as long as it's done explicitly. It only warns when it's used implicitly.

[Nadahar]

For the logging brought up here, this should suffice I think:

        if (logger.isDebugEnabled()) {
            logger.debug("Scheduled {} in {}ms (at {})", job, sleepTime, eventAt.atZone(ZoneId.systemDefault()));
        }

But, basically, all use of Instant must be checked and verified that the fact that it is in UTC doesn't cause any problems.

[clinique]

For the logging brought up here, this should suffice I think:

        if (logger.isDebugEnabled()) {
            logger.debug("Scheduled {} in {}ms (at {})", job, sleepTime, eventAt.atZone(ZoneId.systemDefault()));
        }

But, basically, all use of Instant must be checked and verified that the fact that it is in UTC doesn't cause any problems.

Brought your modification in astro_radiation PR