-
-
Notifications
You must be signed in to change notification settings - Fork 450
Add lastStateUpdate
, lastStateChange
to ItemStateUpdatedEvent
/ItemStateChangedEvent
#4606
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
Add lastStateUpdate
, lastStateChange
to ItemStateUpdatedEvent
/ItemStateChangedEvent
#4606
Conversation
42aa018
to
67c4681
Compare
@jimtng just a generic thought: I see that you are using |
I thought about it, but have no preference either way. I chose ZonedDateTime because Item.getLastStateUpdate etc use ZonedDateTime too. WDYT, should we change everything to Instant? ZDT has a lot more useful methods to operate with, so that's a plus for it. |
I think the principle is that OH Core data is independent of the timezone (i.e. using Instant) and that OH UI converts to the local timezone at the point of display. |
67c4681
to
1346a0b
Compare
OK, and considering that DateTimeType also switched to Instant, it would indeed be uniform to make this also Instant. Since the Item.getLastStateUpdate etc was just recently merged, I hope it's ok to change it. |
How would the instant be represented when sending the event over websockets? |
I'd imagine something like this:
It's iso8601 |
@jlaur I think you have done some other work in OH concerning Instant vs ZonedDateTime, so perhaps you have an opinion here? |
PersistenceExtensions still use ZonedDateTime. My modified build just hit a problem there that I need to adjust. |
The change to Instant will affect the recent changes in mapdb that utilise getLastStateUpdate/Change - @mherwege just FYI - atm this is subject to maintainer's review. |
@@ -476,15 +477,21 @@ private static void internalPersist(Item item, TimeSeries timeSeries, @Nullable | |||
if (!forward && !historicItem.getState().equals(state)) { | |||
// Last persisted state value different from current state value, so it must have updated | |||
// since last persist. We do not know when from persistence, so get it from the item. | |||
return item.getLastStateUpdate(); | |||
return Optional.ofNullable(item.getLastStateUpdate()) | |||
.map(instant -> instant.atZone(ZoneId.systemDefault())).orElse(null); |
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.
The class already has a reference to TimeZoneProvider
, so I think the converion should be done using the configured timezone.
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.
done
I don't have a problem changing that. I still uses |
e4a69cf
to
e68bd33
Compare
@andrewfg I'm a bit on the fence about Instant vs ZonedDateTime. I'm leaning towards ZonedDateTime for lastStateUpdate/lastStateChange. Both zdt and Instant represent a moment in time, but a ZDT is far more useful in practical usage, in scripts/rules. Both can be used to check "how long ago did it happen", i.e. delta time, but only ZDT gives you the extra information about whether it occurred "today", before noon, etc. which can be useful for writing automation rules. Another benefit for using ZDT is when I log the information, it is immediately displayed in the timezone that I'm familiar with (i.e. my local timezone) instead of Zulu, which requires further mental math to be understood. Of course you can deduce the same information from an Instant but that would require a conversion to ZDT first. And this is a bit inconvenient. So perhaps the question is: where are these going to be used? If they'll be used primarily in rules, ZDT would be more convenient. If they'll be used primarily in UI, perhaps Instant offers better flexibility in the same manner that DateTimeType, i.e. an item's state, is handled. |
0950471
to
0a1e582
Compare
No doubt you can do more, when you also have the time-zone. The question is, which time-zone? If you store the time-zone at the moment of the event, it could change afterwards. Many bindings went through a lot effort to obtain the time-zone from Since it's easy to create a For reference (especially the first mentioned issue might be worth a read):
You could still convert to
|
OK, then we'll use Instant for this.
In the case of DateTimeType, it was done in its |
You are right it's a trick, it's even a cheap trick since it uses the system time-zone and not the configured time-zone in OH. It's more or less left for backwards compatibility as an
Can you provide me with a reference to where this happens in the code? I could have a look, but you are probably right it's not so easy to fix, especially if it's not possible to obtain an injected |
It's not an issue of getting a TimeZoneProvider.
Contrast this to DateTimeType object. When one wishes to log this DateTimeType object, DateTimeType.toString() is called. Hence why I said, we can create a wrapper for Instant and ItemStateChangedEvent.getLastStateChange() can return an object of that wrapper, but I don't like that solution. It's not a huge deal, and maybe I'd just leave it as it is now. |
But at least for Rules DSL users it made getting and interacting with DateTimeType worse, and dealing with date times already sucks in Ruels DSL. I originally asked on that original issue/PR if this would that would be user facing and was told not only to have it show up as a breaking change. I would have spoken up more if I knew users would have to manually add back the tiemzone in all their rules. While I dislike Rules DSL already, I don't want to be hostile to its users. But since these are for the events, how would these be exposed to the Rules DSL user? Presumably, injected as an implicit variable? Maybe there is a chance to do something there. I'm not as worried about the JSR223 languages because they have a helper library to put the timezone back on since pretty much all the time a user in a rule is going to want LocalTime or LocalDateTime and for that you need a timezone. |
Do you have a proposal for how to improve Rules DSL in this regard?
I'm not sure what you are referring to here, or mean, but there are no comments from you in #2898 or #3583. The issue was created in April 2022 and I only managed to get the PR ready around November 2024, so I was not rushing or deliberately ignoring any inputs received in those years. 🙂 As mentioned here I did not anticipate beforehand that Rules DSL would be affected. However, the effect is (currently) minimized to 1) using system time-zone rather than |
Not make them have to supply the tiemzone every time they want to work with their local time.
Then I must be remembering a different issue that UI'm confusing with those. There was some issue or PR that dealt with replacing ZDT with Instant that I commented on. I must be confusing them.
This is the part that is a worse experience for Rules DSL users. Almost all the time a user wants to work in local time, not zulu. This means that almost all the time they have to manually add the timezone in order to get to local time. We've gone from needing to supply the timezone being the exception to needing to supply the timezone being the rule. It's relevant to this issue if these date times are going to be exposed in rules too. Clearly they will be a part of the It's unclear whether/how they will be made available in Rules DSL. I can't imagine their not being made available there. If they are exposed as Instants there too, that means there are even more places where the users have to supply their timezone by default instead of the default being to have the default set for them. Or having to call So to see if
There are more. But if
It still sucks but at least it's no worse and there isn't the extra set of conversions required to and/or from Instant. |
It is also used in time series, which I suppose can be accessed from DSL as well. I'm so rusty in DSL by now that I gave up creating a DSL version of this example: https://www.openhab.org/addons/bindings/energidataservice/#javascript
My only concerns are:
I would be totally fine with "un"-deprecating it from DSL rules, but I would be sad to see new usages starting to appear in Java code because of missing warnings, so it's a dilemma. The plan to remove it has been postponed indefinitely because of DSL. Originally I had 5.0 in mind. Two others with no value and most likely very few usages are already removed: #4522
In my opinion we should not try to make |
I have only just started to learn about this openHAB time-zone + TimeZoneProvider. In what is it used? AFAIK, in user scripts, they would call something like If they are aware of TimeZoneProvider, surely they'd be aware of the difference against systemDefault timezone and would stop to think about which timezone would be returned from calling a method such as something.getZonedDateTime(). At which point, they'd refer to the documentation which would state that it returns the systemDefault, if not already intuitively assumed.
Thanks for the explanation. If that's the case, I can see the reason for leaving it marked as deprecated.
In that case, we should just leave the stuff in this PR as Instant. |
If we apply my 1 from above, For example, if a user needs to compare the
Correct and
Then please consider making everything |
That would be a big breaking change especially when it comes to persistence. I personally would prefer working with ZonedDateTime, and I think, especially for me, there's a bit of confusion and mix up between what to use for this PR vs DateTimeType, and somehow in my mind the two got mushed together. So in an attempt to gain better clarity, I'm writing my thoughts, which have been going round and around on this.
@andrewfg do you have any thoughts on this? After all this, I'm still not clear as to what using Instant here, instead of ZDT, would solve or what problems it would avoid. What is clear to me is that using Instant here will be a pain. |
I don’t have a strong opinion either way. The reason why I brought it up in the first place is that I knew (as witnessed by the above thread) that there are indeed two opposing sides who both have very strong opinions and who seem mutually unwilling to reach a common conclusion. Personally my only argument is that whichever side we choose should establish and adhere to a single common approach across the whole of OH. The only thing worse than all A or all B would be a little bit of both. I am not a maintainer so I can’t make the final call. But I do fully support whichever maintainer who has been authorised by the team to make such a call and move on. PS and after making the call we should clearly document the decision all A or all B in the developer documentation so as to avoid future such hand wringing. |
I fully agree, and I apologize for my part of contributing to this confusion. My intention was to share some general thoughts about So just to be clear, I don't have any strong opinions about one or the other in context of this PR. I agree with your analysis, and you should stay consistent with the domain, which seems indeed to be ZDT. |
dff0b79
to
bd9e18a
Compare
OK, I've reverted to ZDT. |
It's currently settled to use ZonedDateTime. @openhab/core-maintainers I think this is ready to be considered now. I've updated the original post to illustrate the usefulness of this PR. |
…ItemStateChangedEvent` Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
This reverts commit e4aa5e6. Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
577e45d
to
c3b7673
Compare
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Jimmy Tanagra <[email protected]>
Rebased and adjusted to the new changes in main |
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.
Thanks @jimtng!
Could you please create a PR for the Rule DSL docs to add the new implicit variables there?
…edEvent#last_state_change` (#401) See openhab/openhab-core#4606 This affects the event's #inspect method, so marking this as ehancement, not just documentation changes Signed-off-by: Jimmy Tanagra <[email protected]>
This is an extension to #4351 so that
ItemStateUpdatedEvent
to havegetLastStateUpdate()
ItemStateChangedEvent
to havegetLastStateUpdate()
getLastStateChange()
The corresponding
lastStateUpdate
andlastStateChange
contexts are also available in RulesDSL.It is a useful addition to the Item.getLastUpdate/getLastChange and this information cannot be obtained correctly from the Item itself.
For example, this rule:
Resolve #4601