-
-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Switch DateTimeType
to Instant
internally for consistent time-zone handling
#3583
Conversation
ab14428
to
81c3dc0
Compare
I hit a wall with this when realizing that I don't know if any @openhab/webui-maintainers can throw some light at what may be a possible way forward? |
4ec7437
to
229ee4e
Compare
229ee4e
to
266b055
Compare
@mherwege - seeing your work in #4169 I'm wondering if you can advise how to accomplish decoupling the time-zone + formatting of Currently it's formatted like this: |
@jlaur Main UI works with what it gets from the REST API.
The formatted state comes from the
So, to understand where Line 953 in cbb458e
state is added through the EnrichedItemDTOMapper here: Line 81 in cbb458e
So it is simply using the toFullString() method on the state of the item.
|
@mherwege - many thanks for your insights. I will have a closer look at this soon. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
0829630
to
d007aa1
Compare
@openhab/core-maintainers - I don't have permission to remove the "work in progress" label since I'm not in the contributors team, so perhaps someone can do that for me? I consider this PR ready for review now after finally fixing the returned @mherwege - perhaps you'd also like to have a look? |
What about bindings setting a DateTimeType state with a particular time zone? |
They can still do that, but the time-zone will be stripped, so it's really not needed anymore.
It should be, but there can be unit tests failing if they expect a certain time-zone: DateTimeType dt = new DateTimeType(ZonedDateTime.now());
ZonedDateTime zdt = dt.getZonedDateTime(); Previously this would return a |
d007aa1
to
6f47d07
Compare
@joerg1985 - perhaps you would also like to have a look? I saw some of your recent related comments and enhancements, e.g. #4384. |
This will call Line 300 in 3bc9ae3
which is probably the reason for system time-zone being used in some places. I don't know how to improve this, since Keeping it "as is", in current version of the code I have provided, will be correct in all cases where system time-zone is the same as the time-zone configured in openHAB. When they are different and a state is provided using the correct time-zone from |
8a820be
to
7df4b06
Compare
Signed-off-by: Jacob Laursen <[email protected]>
Signed-off-by: Jacob Laursen <[email protected]>
…th ZoneId Signed-off-by: Jacob Laursen <[email protected]>
36c0202
to
58353e8
Compare
@J-N-K - thanks for picking this up for review. I have tried to address all your review comments. I have force-pushed after rebasing towards main, but all today's changes are in the last two commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks!
Related to openhab/openhab-core#3583 Signed-off-by: Jacob Laursen <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Related to openhab/openhab-core#3583 Signed-off-by: Jacob Laursen <[email protected]>
Due to openhab/openhab-core#3583 Signed-off-by: Cody Cutrer <[email protected]>
Due to openhab/openhab-core#3583 Signed-off-by: Cody Cutrer <[email protected]>
Due to openhab/openhab-core#3583 Signed-off-by: Cody Cutrer <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/datetime-operations/160794/6 |
…Time` (#408) Related to openhab/openhab-core#3583. Resolves #404. This removes usages of the deprecated `DateTimeType` method [getZonedDateTime()](https://www.openhab.org/javadoc/latest/org/openhab/core/library/types/datetimetype#getZonedDateTime()) and instead applies the time-zone configured in openHAB. Signed-off-by: Jacob Laursen <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/solved-habpanel-sluggish/81756/219 |
Proposal for switching
DateTimeType
to useInstant
internally rather thanZonedDateTime
.This will effectively discard time-zone information from the source of the
DateTimeType
and leave it to consumers to apply time-zone at the moment of presentation, according to the configured time-zone in openHAB regional settings or in the browser.Configured time-zone is now applied for:
Consequential changes because
DateTimeType
is now "zone-less":timezone
forTimestampOffsetProfile
has been removed. Remove time-zone parameter from profiletimestamp-offset
openhab-docs#2403 has been prepared.toZone
,toLocaleZone
andgetZonedDateTime
have been marked as deprecated and all usages have been refactored. I would suggest to remove them in 5.0 (or maybe even before when all bindings are refactored, see SimplifyDateTimeType
handling openhab-addons#17725).getZonedDateTime(ZoneId)
has been introduced for convenience.The following demonstrates the difference between old and new behavior.
REST API calls are for
http://localhost:8080/rest/items/DateTimeTest
and the returnedstate
is shown in the comparison table:Updates are made in Karaf using commands like:
As it can be seen from the table, we now have consistent results.
Example DSL rule:
Previous result with system time-zone CET/DST:
New result with same system time-zone:
Summary of benefits for developers:
TimeZoneProvider
is used only to generate aDateTimeType
with the correct time-zone, this can now be completely removed. This simplifies the code. See SimplifyDateTimeType
handling openhab-addons#17725 for examples.DateTimeType
constructor can now be used (without losing correct time-zone).toZone
andtoLocaleZone
. I will go through usages and remove them, since they no longer make sense. See SimplifyDateTimeType
handling openhab-addons#17725.DateTimeType
usingTimeZoneProvider
.And for users:
DateTimeType
(which could be either depending on binding) and other parts used system time-zone.And finally limitations:
DateTimeType
command to a binding with a specified time-zone. The bindings will have to provide a time-zone if needed (e.g. time-zone from device, openHAB configuration or system time-zone). I have not found any bindings using this possibility, so I don't think this will cause any issues.Resolves #2898