[time] Default to system timezone in `toZDT` if none is explicitely set

[rkoshak]

Fixes #306.

During DST changes, any ISO8601 String or Item or DateTimeType or Java ZonedDateTime that is parsed by time.toZDT() lack the ZoneId. However, it is this ZoneId which tells Joda when DST changes occur so when toToday() is called, the time is not adjusted to account for DST.

This defaults to the system timezone if no timezone is explicitely set.

[rkoshak]

We have fixed the problem but only for Strings. 😭 I should have thought of this.

It does not work when the DateTime Item or the raw DateTimeType is passed to toZDT().

See the following code with the results:

var now = time.toZDT();
var beforeDSTstr = time.toZDT(items.TestDateTime.state);
var beforeDSTdtt = time.toZDT(items.TestDateTime.rawState);
var beforeDSTdti = time.toZDT(items.TestDateTime);
console.info('Now:              ' + now);
console.info('Before DST Str:   ' + beforeDSTstr);
console.info('To Today Str:     ' + beforeDSTstr.toToday());
console.info('Before DST DTT:   ' + beforeDSTdtt);
console.info('To Today DTT:     ' + beforeDSTdtt.toToday());
console.info('Before DST DTI:   ' + beforeDSTdti);
console.info('To Today DTI:     ' + beforeDSTdti.toToday());

Results:

2023-11-14 14:59:42.409 [INFO ] [nhab.automation.script.ui.scratchpad] - Now:              2023-11-14T14:59:42.405-07:00[SYSTEM]
2023-11-14 14:59:42.410 [INFO ] [nhab.automation.script.ui.scratchpad] - Before DST Str:   2023-11-03T14:02:04.274-06:00[SYSTEM]
2023-11-14 14:59:42.411 [INFO ] [nhab.automation.script.ui.scratchpad] - To Today Str:     2023-11-14T14:02:04.274-07:00[SYSTEM]
2023-11-14 14:59:42.412 [INFO ] [nhab.automation.script.ui.scratchpad] - Before DST DTT:   2023-11-03T14:02:04.274-06:00
2023-11-14 14:59:42.413 [INFO ] [nhab.automation.script.ui.scratchpad] - To Today DTT:     2023-11-14T14:02:04.274-06:00
2023-11-14 14:59:42.413 [INFO ] [nhab.automation.script.ui.scratchpad] - Before DST DTI:   2023-11-03T14:02:04.274-06:00
2023-11-14 14:59:42.414 [INFO ] [nhab.automation.script.ui.scratchpad] - To Today DTI:     2023-11-14T14:02:04.274-06:00

I was so focused on the toString that I forgot about Items and states too.

toToday() should work on ZDS produced from these too, right? I'm not sure the best way to handle this. Should I add the fix in both of those cases in untils.javaZDTToJsZDT? If I do that do we even need both REGEX anymore? Do we need to sit back and think about this some more?

[florian-h05]

Should I add the fix in both of those cases in untils.javaZDTToJsZDT? If I do that do we even need both REGEX anymore? Do we need to sit back and think about this some more?

Yes, we need both RegEx(es) anymore, because IMO it should be possible to parse a ZDT from an (ISO) String containing a timeline or not containing a tz. Theoretically, the utils function could be replaced by converting the Java ZDT to string and parsing it using the parseISO function, but string parsing has very bad performance. I did some performance benchmarking when developing the utils function,

I am against changing the existing utils function, because IMO the utils should simply convert and not set any defaults. I would add a warning regarding timezones to the utils method’s docs and introduce a private method to time.js to default to the system time zone for all usages of the utils method inside time.js

[florian-h05]

introduce a private method to time.js to default to the system time zone

Or should we add this as a new utils method?

[rkoshak]

Yes, we need both RegEx(es) anymore, because IMO it should be possible to parse a ZDT from an (ISO) String containing a timeline or not containing a tz.

That was working before though, wasn't it? The only problem was that those Strings that were parsed without the zone id wouldn't handle toToday() correctly on the day of a DST change.

I would add a warning regarding timezones to the utils method’s docs and introduce a private method to time.js to default to the system time zone for all usages of the utils method inside time.js

That seems reasonable.

Or should we add this as a new utils method?

I like the idea of keeping like stuff together. I'm not certain why the utils method is in utils in the first place nor where that util method is used. This feels like a similar function though so it makes sense to me to make a new utils function that time.js calls.

I'll fix the regex so the builds become happier.

[florian-h05]

That was working before though, wasn't it? The only problem was that those Strings that were parsed without the zone id wouldn't handle toToday() correctly on the day of a DST change.

Exactly. Those RegExp can be kept as they currently are in this PR.

I'm not certain why the utils method is in utils in the first place nor where that util method is used.

When helping out @jlaur with JS scripting examples for his energy prices binding, we noticed that there might be the need to have such a method. Of course toZDT is able to handle that as well, but given the number of javaToJS functions we already had in utils, I thought it would be useful to provide a simple conversions for ZDT there as well. If I want to convert a Java type to a JS type, I always look first at utils, as this is the dedicated place for such stuff.

I guess the utils methods are mostly used by the library internally, where we often need type conversions. Compared to toZDT, the utils method is extremely simple and therefore less error prune, which is a big advantage for the internal usage.

[jlaur]

When helping out @jlaur with JS scripting examples for his energy prices binding, we noticed that there might be the need to have such a method. Of course toZDT is able to handle that as well, but given the number of javaToJS functions we already had in utils, I thought it would be useful to provide a simple conversions for ZDT there as well. If I want to convert a Java type to a JS type, I always look first at utils, as this is the dedicated place for such stuff.

For reference:

• https://github.com/openhab/openhab-addons/pull/14376#discussion_r1189121993
• https://github.com/openhab/openhab-addons/pull/14376#discussion_r1197040988

[rkoshak]

That makes sense. Because this is very much like the existing utils method I think it should probably go there and then have the time method call the new function from utils instead of the old one.

It will probably be late today but stay tuned for a new commit. The changes are super easy thankfully.

[florian-h05]

@rkoshak Whats the status here? Should I take over?

[rkoshak]

Unfortunately I've not been able to get back to this. It's still at the top of my todo list but stuff like mitigating a burst pipe and planned family activities have eaten up most of the time I have to work this.

If you have the time I'm happy to let you take over.