Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 17, 2025

This is an extension to #4351 so that

ItemStateUpdatedEvent to have

  • getLastStateUpdate()

ItemStateChangedEvent to have

  • getLastStateUpdate()
  • getLastStateChange()

The corresponding lastStateUpdate and lastStateChange contexts are also available in RulesDSL.

Resolve #4601

@andrewfg
Copy link
Contributor

@jimtng just a generic thought: I see that you are using ZonedDateTime here, where I wonder if you should not rather be using Instant ??

@jimtng jimtng marked this pull request as draft February 17, 2025 10:10
@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2025

@jimtng just a generic thought: I see that you are using ZonedDateTime here, where I wonder if you should not rather be using Instant ??

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.

@andrewfg
Copy link
Contributor

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.

@jimtng jimtng force-pushed the ItemEvent-lastStateUpdate-lastStateChange branch from 67c4681 to 1346a0b Compare February 17, 2025 10:58
@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2025

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.

@spacemanspiff2007
Copy link
Contributor

How would the instant be represented when sending the event over websockets?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2025

How would the instant be represented when sending the event over websockets?

I'd imagine something like this:

jshell> Instant.now().toString()
$11 ==> "2025-02-17T11:15:04.152240Z"

It's iso8601

@andrewfg
Copy link
Contributor

@jlaur I think you have done some other work in OH concerning Instant vs ZonedDateTime, so perhaps you have an opinion here?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2025

PersistenceExtensions still use ZonedDateTime. My modified build just hit a problem there that I need to adjust.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2025

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);
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mherwege
Copy link
Contributor

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.

I don't have a problem changing that. I still uses Date internally in mapdb and first converts the ZonedDateTime to Instant internally anyway. This will only simplify things.

@jimtng jimtng force-pushed the ItemEvent-lastStateUpdate-lastStateChange branch from e4a69cf to e68bd33 Compare February 18, 2025 02:15
…ItemStateChangedEvent`

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng
Copy link
Contributor Author

jimtng commented Feb 19, 2025

@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.

@jimtng jimtng force-pushed the ItemEvent-lastStateUpdate-lastStateChange branch from e68bd33 to 0950471 Compare February 19, 2025 07:27
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng force-pushed the ItemEvent-lastStateUpdate-lastStateChange branch from 0950471 to 0a1e582 Compare February 19, 2025 07:48
@jlaur
Copy link
Contributor

jlaur commented Feb 19, 2025

Both zdt and Instant represent a moment in time, but a ZDT is far more useful in practical usage, in scripts/rules.

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 TimeZoneProvider in order to create a correct ZonedDateTime for DateTimeType. Even with all this effort, it would still be wrong if the time-zone would then be changed afterwards. Migrating DateTimeType to use Instant internally deferred resolving the time-zone until the moment when it was actually needed. This simplified all places where DateTimeType is constructed and made it consistent as well. It also made it possible to apply a time-zone change that would instantly change presentation of existing DateTimeTypes - and could allow for the browser's time-zone to be used as well, if desired.

Since it's easy to create a ZonedDateTime from an Instant and a time-zone, I think we should prioritize what is logically most "correct" over what is most convenient. Some of that convenience might also be possible to provide in other places. For example, in JavaScript Scripting, you can still get a ZDT from a DateTimeType. The only difference from before 4.3 is that it will now apply the configured time-zone on the fly rather than using any time-zone already stored inside the DateTimeType.

For reference (especially the first mentioned issue might be worth a read):

Another benefit for using ZDT is when I log the information

You could still convert to ZonedDateTime for logging purposes. Using DateTimeType as example, logging is unaffected by the internal switch to Instant:

2025-02-19 10:44:05.105 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'MotionSensor_FamilyRoom_LastMotion' changed from 2025-02-19T10:43:55.092+0100 to 2025-02-19T10:44:05.053+0100

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng marked this pull request as ready for review February 19, 2025 10:37
@jimtng
Copy link
Contributor Author

jimtng commented Feb 19, 2025

OK, then we'll use Instant for this.

You could still convert to ZonedDateTime for logging purposes. Using DateTimeType as example, logging is unaffected by the internal switch to Instant:

In the case of DateTimeType, it was done in its toString() method. This trick doesn't apply here because we're not using a custom class. It's literally the Instant object that we want to log. Any ideas how to make these particular fields to log in ZDT? The only thing I can think of is to create a wrapper class, but that would probably create more issues than it solves.

@jlaur
Copy link
Contributor

jlaur commented Feb 19, 2025

You could still convert to ZonedDateTime for logging purposes. Using DateTimeType as example, logging is unaffected by the internal switch to Instant:

In the case of DateTimeType, it was done in its toString() method. This trick doesn't apply here because we're not using a custom class.

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 Instant format would be more logical.

It's literally the Instant object that we want to log. Any ideas how to make these particular fields to log in ZDT? The only thing I can think of is to create a wrapper class, but that would probably create more issues than it solves.

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 TimeZoneProvider.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 19, 2025

It's literally the Instant object that we want to log. Any ideas how to make these particular fields to log in ZDT? The only thing I can think of is to create a wrapper class, but that would probably create more issues than it solves.

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 TimeZoneProvider.

It's not an issue of getting a TimeZoneProvider.

ItemStateChangedEvent.getLastStateChange() returns an Instant object. It is this object that one may wish to log, so Instant.toString() will be called.

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.

@rkoshak
Copy link

rkoshak commented Feb 19, 2025

Migrating DateTimeType to use Instant internally deferred resolving the time-zone until the moment when it was actually needed. This simplified all places where DateTimeType is constructed and made it consistent as well. It also made it possible to apply a time-zone change that would instantly change presentation of existing DateTimeTypes - and could allow for the browser's time-zone to be used as well, if desired.

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.

@jlaur
Copy link
Contributor

jlaur commented Feb 19, 2025

Migrating DateTimeType to use Instant internally deferred resolving the time-zone until the moment when it was actually needed. This simplified all places where DateTimeType is constructed and made it consistent as well. It also made it possible to apply a time-zone change that would instantly change presentation of existing DateTimeTypes - and could allow for the browser's time-zone to be used as well, if desired.

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.

Do you have a proposal for how to improve Rules DSL in this regard?

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'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 DateTimeType time-zone (since it doesn't exist anymore) and 2) having a deprecation notice when loading a rule that doesn't provide the time-zone explicitly. I'm not sure if this is what you refer to or wanted to bring into the discussion? I'm also not sure how this effect the PR at hand, because I didn't look closely into the details, but just wanted to share my view on Instant vs. ZonedDateTime since some of the same arguments for preferring Instant might apply here as well.

@rkoshak
Copy link

rkoshak commented Feb 19, 2025

Do you have a proposal for how to improve Rules DSL in this regard?

Not make them have to supply the tiemzone every time they want to work with their local time.

I'm not sure what you are referring to here, or mean, but there are no comments from you in #2298 or #3583.

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.

  1. using system time-zone rather than DateTimeType time-zone (since it doesn't exist anymore) and 2) having a deprecation notice when loading a rule that doesn't provide the time-zone explicitly.

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 event so they will be a part of the event Object in JSR223 rules and available there.

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 toInstant to convert from a ZDT or LocalDT to an Instant to be used there (as far as I can tell from the JavaDocs, I cannot call isBefore() et al with an Instant and one of these, they all either need to be Instants or they all need to be ZDTs of LocalDTs.

So to see if lastStateUpdate was before 7:00 AM today local time in Rules DSL (assuming it's an implicit variable like newState it would be one of:

// users have been trained to work with ZDT so this is what most will do
if(ZonedDateTime.ofInstant(lastStateUpdate, ZoneId.of("America/Denver").isBefore(now.withHour(7).withMinute(0).withSecond(0).withNano(0)) {

// Convert ZDT to Instant
if(lastStateUpdate.isBefore(now.withHour(7).withinMinute(0).withSecond(0).withNano(0).toInstant()) {

// LocalTime (LocalTime does not have a toInstant)
if(LocalTime.ofInstant(lastStateUpdate, ZoneId.of("America/Denver")).isBefore(LocalTime.of(7, 0)) {

// LocalDateTime
if(LocalDateTime.ofInstant(lastStateUpdate, ZoneId.of("America/Denver")).isBefore(now.toLocalDateTime().withHour(7).withMinute(0).withSecond(0).withNano(0)) {

// I see no reason why anyone would do it this way but I put it here for completeness
if(lastStateUpdate.isBefore(now.toLocalDateTime().withHour(7).withMinute(0).withSecond(0).withNano(0).toInstant()) {

There are more. But if lastStateUpdate were a ZonedDateTime the code would be:

if(lastUpdate.isBefore(now.withHour(7).withMinute(0).withSecond(0).withNano(0))) {

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.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 19, 2025

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.

Yes, these will be available as implicit variables lastStateUpdate and lastStateChange in rulesDSL. At the moment they'll be of Instant type, so as you said, you'll have to add extra code to convert them to deal with ZDT. IMO, very inconvenient.

Do you have a proposal for how to improve Rules DSL in this regard?

Not make them have to supply the tiemzone every time they want to work with their local time.

This is indeed the main reason why I'm favouring ZonedDateTime over Instant.

Unfortunately, injecting lastStateUpdate / lastStateChange into rulesDSL (and also into the event objects) as a ZonedDateTime, whilst keeping the main Item.getLastStateChange() as Instant would just cause confusion and inconsistencies.

All should be of the same type, either Instant or ZDT.

As I understand it, the main reasons for using Instant are:

  1. to avoid the case when the time zone may change?
  2. to allow each "client" receiving the data (e.g. different browsers / event receivers in various different timezones) to apply their own timezones

I'd say for (1) above, it's an extremely rare event and I'd be happy to ignore this "problem". Besides, if one wishes to avoid this issue, they could still call ZonedDateTime.withZoneSameInstant(myZoneId)
For (2), rather than the client calling Instant.atZone(myZoneId), they can instead call ZonedDateTime.withZoneSameInstant(myZoneId).

@rkoshak
Copy link

rkoshak commented Feb 20, 2025

This might be way bigger and have more impact than we might want to consider, but what if there were a wrapper Class or something that gave you options. If the end user wants a ZDT they call getZonedDateTime. If they want an Instant they can call getInstant. If they want a LocalDateTime or LocalTime or LocalDate they can call a getter for those. I think this was already suggested above, but I wanted to raise it again.

It can store the actual timestamp as an Instant and if any of the timezone needing getters are called then the system default timezone can be assumed. It's still not as good as everything already being the same type to begin with, but it frees the Rules DSL user from the added burden of keeping track of and manually needing to convert between types. They can just ask for what type they want to use.

This is the model that JS uses for State and it seems to work well. If you want the state as a Number, call numericState. Want to use QuantityTypes, call quantityState. The user can ask for what type they want and they don't have to just know and handle conversions manually like Rules DSL already requires so much of the time.

But for consistency this would almost have to be applied everywhere which could be too big of a job. Maybe we can just do this for rules somehow? That's where the user facing impacts will appear.

@jlaur
Copy link
Contributor

jlaur commented Feb 20, 2025

  1. using system time-zone rather than DateTimeType time-zone (since it doesn't exist anymore) and 2) having a deprecation notice when loading a rule that doesn't provide the time-zone explicitly.

This is the part that is a worse experience for Rules DSL users.

Which part, 1 or 2? Unless you are using someone else's server in another time-zone, the system time-zone should usually be the same as the openHAB time-zone. They can be different, hence the deprecation notice.

To continue this particular part of the discussion, I would propose to create an issue for that.

As I understand it, the main reasons for using Instant are:

  1. to avoid the case when the time zone may change?
  2. to allow each "client" receiving the data (e.g. different browsers / event receivers in various different timezones) to apply their own timezones

In general, both Instant and ZonedDateTime each have their uses, that's why they both exist.

For DateTimeType, there is also the advantage of simplifying bindings and making the behavior consistent. Please see the addons PR I already linked to and the comparison table at the top of the core PR I linked to.

I'd say for (1) above, it's an extremely rare event and I'd be happy to ignore this "problem". Besides, if one wishes to avoid this issue, they could still call ZonedDateTime.withZoneSameInstant(myZoneId)
For (2), rather than the client calling Instant.atZone(myZoneId), they can instead call ZonedDateTime.withZoneSameInstant(myZoneId).

That would reintroduce inconsistency, since then all bindings would then have to provide proper ZDT's rather than Instants. If they don't, we'll have a ZDT with wrong time-zone, which would be exposed in some places, but not in others (when withZoneSameInstant is used).

It seems that the main concern and biggest problem in this discussion is Rules DSL, is this correct? The tight coupling between DSL and the type classes seems to be hurting us here, and I would really hope we could find a solution for that rather than let DSL dictate our design. 😢 At least I think we need to be very clear about when we "prefer" one or the other, if it's "just because of DSL" or if there are other reasons involved. That's not to neglect DSL, but if DSL is the only reason for sticking to a flawed design, we really need to consider our options.

@rkoshak
Copy link

rkoshak commented Feb 20, 2025

Which part, 1 or 2? Unless you are using someone else's server in another time-zone, the system time-zone should usually be the same as the openHAB time-zone. They can be different, hence the deprecation notice.

Both part 1 and 2. Indeed they can be different, but how often does that happen? Should the default (i.e. working within one timezone) have to do more work in their rules to support this rare use case? Because that's the case now. We are imposing more work and more complexity on the users of OH who are least capable of adopting to it.

It seems that the main concern and biggest problem in this discussion is Rules DSL, is this correct?

Only because the other languages have a helper library to provide a work around. It's a problem there too, we can just fix the problem on behalf of the end users in the helper library.

The tight coupling between DSL and the type classes seems to be hurting us here

That's how Rules DSL works. There is no helper library. Rules DSL (and Groovy too I think) use the raw Java classes. If you change them for add-ons (for example) they change for Rules DSL users too. And again, the end users are the ones least capable of handling this complexity. We should be making their job easier, not harder. We especially shouldn't be making the default use case harder to support a rare use case.

I can't speak to what Instant does or does not do for add-ons or other parts of OH but I can say that it makes the end user's experience worse.

@jlaur
Copy link
Contributor

jlaur commented Feb 20, 2025

  1. using system time-zone rather than DateTimeType time-zone (since it doesn't exist anymore)
  2. having a deprecation notice when loading a rule that doesn't provide the time-zone explicitly.

This is the part that is a worse experience for Rules DSL users.

Which part, 1 or 2? Unless you are using someone else's server in another time-zone, the system time-zone should usually be the same as the openHAB time-zone. They can be different, hence the deprecation notice.

Both part 1 and 2. Indeed they can be different, but how often does that happen?

In that case we have a misunderstanding. For the specific (and quite normal) case where system and openHAB time-zone is the same, getZonedDateTime() also works almost exactly the same as before DateTimeType was changed to store an Instant internally. The only exception is that previously when bindings would provide a "wrong" time-zone (e.g. UTC), getZonedDateTime() would return this directly. Also, DST information would be lost. Now, getZonedDateTime() consistently returns a ZonedDateTime in the system time-zone.

@rkoshak
Copy link

rkoshak commented Feb 21, 2025

Now, getZonedDateTime() consistently returns a ZonedDateTime in the system time-zone.

But without a wrapper here, lastStateUpdate will be an Instant. The rules developer will need to manually convert to/from an Instant in order to do any comparisons. That's my main point. There's no way to give the timestamp in the user's tiemzone in this case.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 22, 2025

@rkoshak I can either:

a) Create a special InstantWrapper just for RulesDSL, so that they can use it as

lastStateChange.toZonedDateTime

But this won't apply to MyItem.lastStateChange which will still be pure Instant.
And this won't apply to jsr223

or

b) Apply the InstantWrapper everywhere, but only in the scope of this PR, just for Item.getLastStateChange + event.getLastStateChange, and not any other unrelated places where Instant may also be used. If that's desired for other stuff, it can be dealt with in a separate PR.

or

c) Just stay with Instant as is. This would be the cleanest, least "weird" way (Applying an InstantWrapper seems odd to me). But as we know, it's a bit of a hassle to have to convert to a ZDT, especially in rulesdsl.

PS: Conversion from Instant to ZDT is just this in rulesdsl: lastStateChange.atZone(ZoneId.systemDefault), which isn't extremely onerous.

@rkoshak
Copy link

rkoshak commented Feb 22, 2025

My two driving principals are sometimes at cross purposes.

  1. We should try to be consistent. The same sort of operation should work the same everywhere.
  2. We should not impose extra work on the end users, particularly those end users least able to handle it.

a) violates 1 and 2 for MyITem.lastStateChange

c) violates 1 unless everything becomes Instant but then because everything becomes Instant it violates 2.

b) doesn't violate 1 or 2 so it would be my choice, assuming the other PRs happened.

If there isn't some simple way to get the time with the zone by default everywhere time is dealt with by end users then one or both of these plrincipals cannot be met.

PS: Conversion from Instant to ZDT is just this in rulesdsl: lastStateChange.atZone(ZoneId.systemDefault), which isn't extremely onerous.

Except that we are taking the default and most common use case right now (i.e. I get a date time with the zone) and requiring extra work to achieve that and making so the default and least amount of work applies to the least common use case. Both use cases should be supported, but the default should be the most common use case, not the least.

Obviously this is just my opinion and I won't stand in the way of any choice made here (a, b or c).

@jimtng
Copy link
Contributor Author

jimtng commented Feb 22, 2025

b) doesn't violate 1 or 2 so it would be my choice, assuming the other PRs happened.

Can you think of where else Instant is used which also touched end users (jsr223 / rulesdsl)?

DateTimeType doesn't count I think because it already has getZonedDateTime() although that is deprecated, and only getZonedDateTime(ZoneId) is recommended. @jlaur maybe we should not deprecate DateTimeType.getZonedDateTime() (the one which returns the default systemzone)? It can just be seen as an additional convenience method.

That way we'll have InstantWrapper.getZonedDateTime() along with DateTimeType.getZonedDateTime()

Also for consisistency, perhaps we can alias DateTimeType.getZonedDateTime(ZoneId) with DateTimeType.atZone(ZoneId) to make it kind of like Instant.atZone(ZoneId)? This is so from the user's code (rulesdsl), they can call:

  • MyItem.state.atZone(ZoneId) (for DateTimeType) just like MyItem.lastStateChange.atZone(ZoneId) - for when they want to explicitly specify the zoneid
  • MyItem.state.zonedDateTime (for DateTimeType) just like MyItem.lastStateChange.zonedDateTime - for when they want the conversion to the system timezone

@jlaur
Copy link
Contributor

jlaur commented Feb 22, 2025

Can you think of where else Instant is used which also touched end users (jsr223 / rulesdsl)?

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

maybe we should not deprecate DateTimeType.getZonedDateTime() (the one which returns the default systemzone)? It can just be seen as an additional convenience method.

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

Also for consisistency, perhaps we can alias DateTimeType.getZonedDateTime(ZoneId) with DateTimeType.atZone(ZoneId) to make it kind of like Instant.atZone(ZoneId)?

In my opinion we should not try to make DateTimeType "like" Instant, ZonedDateTime, LocalDateTime, Date or anything else. DateTimeType is not an Instant. Internally it stores an Instant, but until recently it internally stored a ZoneDateTime. And before that it internally stored a Calendar. Providing atZone would introduce some kind of "mental" coupling, and then one might expect other similar Instant methods as well. Instead, if you need an Instant, get it and then use that, i.e. DateTimeType.instant.atZone(ZoneId) in your example. I would also add that it's very hard to remove anything once it's introduced, since we have this DSL coupling where we can break user rules.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 22, 2025

  • The reason why it's not recommended is because it uses system time-zone rather than openHAB time-zone. I don't like that this happens implicitly/behind the scenes.

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 ZonedDateTime.now() and compare that against other ZonedDateTime stuff, all of which would be in the systemDefault zone?

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.

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.

Thanks for the explanation. If that's the case, I can see the reason for leaving it marked as deprecated.

In my opinion we should not try to make DateTimeType "like" Instant, ZonedDateTime, LocalDateTime, Date or anything else. DateTimeType is not an Instant. Internally it stores an Instant, but until recently it internally stored a ZoneDateTime. And before that it internally stored a Calendar. Providing atZone would introduce some kind of "mental" coupling, and then one might expect other similar Instant methods as well. Instead, if you need an Instant, get it and then use that, i.e. DateTimeType.instant.atZone(ZoneId) in your example.

In that case, we should just leave the stuff in this PR as Instant.

@rkoshak
Copy link

rkoshak commented Feb 24, 2025

Can you think of where else Instant is used which also touched end users (jsr223 / rulesdsl)?

If we apply my 1 from above, now should become an Instant. Any datetime returned by Persistence or used to create time series should become an Instant. DateTimeType already returns an Instant. The default should be when I get a timestamp or need to create a timestamp or modify a timestamp, it's the same type of Object no matter where I get it from to being with.

For example, if a user needs to compare the lastStateUpdate with now and/or the state of a DateTimeType they shouldn't have to remember that one is an Instant and the other is a ZonedDateTime and they have to convert one or the other so they can be compared.

this deprecation also affected DSL when loading a rule.
I think Groovy also uses the raw Java classes.

AFAIK, in user scripts, they would call something like ZonedDateTime.now() and compare that against other ZonedDateTime stuff, all of which would be in the systemDefault zone?

Correct and ZonedDateTime.now() has been aliased (or how ever it's done or what ever it's called) to now in the rule. And almost everyone starts with now and adds stuff or subtracts stuff or uses withHour() et al to manipulate it as needed. For example to schedule a timer to go off in an hour one would use now.plusHours(1). To see if the lastStateUpdate was more than an hour ago should be lastStateUpdate.isBefore(now.minusHours(1) but if now remains a ZonedDateTime and lastStateUpdate remains an Instant the end user needs to know that and convert one or the other before comparing.

In that case, we should just leave the stuff in this PR as Instant.

Then please consider making everything Instant. Having to supply a timezone everywhere is better than needing to have to remember that it's a ZonedDateTime from here but an Instant from there.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 25, 2025

In that case, we should just leave the stuff in this PR as Instant.

Then please consider making everything Instant. Having to supply a timezone everywhere is better than needing to have to remember that it's a ZonedDateTime from here but an Instant from there.

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.

  • We can see DateTimeType as a separate topic, and in fact it might not even be so relevant here. It has a getZonedDateTime and it has getInstant. So from the user's perspective, it is neither Instant nor ZonedDateTime. It has a special use case because Bindings need to initialise a DateTimeType objects hence why Instant makes sense there. I am not arguing or suggesting here that DateTimeType needs to change, because it is obvious that its change to Instant is 100% a positive change.
  • The main issue that DateTimeType solved by changing its internal storage to Instant may not be so relevant for this PR.
  • The timestamp 'lastStateChange' and 'lastStateUpdate' are only recorded in one place, not by various bindings, so there won't be any inconsistencies.
  • For the rest, most things are already using ZDT, including PersistenceExtensions.
  • RulesDSL now is a ZDT
  • User scripts usually only care about the local zone, and ZDT works fine there
  • Whey they do need to pay attention to time zones, then they can perform the conversions as needed
  • ZDT offers relevant methods such as plushours, plusdays, etc whereas Instant doesn't
  • We can also argue that using ZDT here and everywhere, as they have been, will cause the least breaking change. This is not referring to DateTimeType.

@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.

@andrewfg
Copy link
Contributor

do you have any thoughts on this?

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.

@jlaur
Copy link
Contributor

jlaur commented Feb 25, 2025

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.

I fully agree, and I apologize for my part of contributing to this confusion. My intention was to share some general thoughts about ZonedDateTime vs. Instant after being asked, and I also wanted to defend #3583 when that was questioned.

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.

This reverts commit e4aa5e6.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng force-pushed the ItemEvent-lastStateUpdate-lastStateChange branch from dff0b79 to bd9e18a Compare February 25, 2025 10:15
@jimtng
Copy link
Contributor Author

jimtng commented Feb 25, 2025

OK, I've reverted to ZDT.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
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.

Add LastStateChange and LastStateUpdate Timestamp to events
6 participants