Extend support to ISO8601 format for sitemap chart period parameter

[lolodomo]

Signed-off-by: Laurent Garnier lg.hc@free.fr

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/6-month-period-of-charts-not-working/150768/9

[mherwege]

While you are at it, could you also add 3 months?

[lolodomo]

While you are at it, could you also add 3 months?

Done

[J-N-K]

I'm not sure this is the way to go. Wouldn't it make more sense to use https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Duration.html#parse(java.lang.CharSequence)?

[lolodomo]

I'm not sure this is the way to go. Wouldn't it make more sense to use https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/time/Duration.html#parse(java.lang.CharSequence)?

Can you explain a little more your thoughts @J-N-K ?

[J-N-K]

ISO8601 defines how to specify periods of time and Duration.parse can be used to get a Duration from such a string.

• D for days
• M for month
• W for weeks
• Y for years
• H for hours
• M for minutes

If we detect a single letter, add 1 in front. If it contains H additionally add T. Examples:

• H -> T1H
• 4H-> T4H
• W-> 1W
• 2M -> 2M

We then use Duration duration = Duration.parse("P" + fixedDurationString); instead of the switch statement. That way we can use every period that the user wants to use, no matter if we added it before. If we log a warning that H should be T1H in the future (i.e. always if we detect that the defines string and the "fixed string" is not the same, we can also support something like T1H30M for the last 90 minutes and still be backward compatible.

[mherwege]

@J-N-K Are you sure it will parse weeks, months and years? In my understanding it only starts with days (D). Or do you mean to convert these to days before parsing?

[J-N-K]

It seems I misread the JavaDoc. I states ISO-8601, but only supports a subset of that. However, SO know the solution: https://stackoverflow.com/a/46258784.

// split in 2 parts
String input = "P2YT3H10M";
String[] v = input.split("T");
Period p;
Duration d;
if (v.length == 1) { // has only date-based fields
    p = Period.parse(input);
    d = Duration.ZERO;
} else {
    if ("P".equals(v[0])) { // has only time-based fields
        p = Period.ZERO;
    } else {
        p = Period.parse(v[0]);
    }
    d = Duration.parse("PT" + v[1]);
}

[lolodomo]

@J-N-K : is your idea to extend the chart period syntax to what Period and Duration Java classes support? This syntax is not very user friendly. Or do we use them but restrict the user syntax to a number of hours or days or weeks or months or years?

[lolodomo]

Ok, what I finally propose and what is, I believe, @J-N-K suggestion, is to accept a duration in the ISO-8601 notation in addition to our current simplified (and limited) notation.
If the provided is h, h, D, D, W, W, M, M, Y or Y, we convert it to a valid ISO-8601 notation for a duration and then try to parse it either as a java Duration or Java Period.
In other cases, we directly try to parse it either as a java Duration or Java Period.

[lolodomo]

@J-N-K @mherwege : I just pushed the new version of this PR extending the support to ISO8601 duration format. I also included unit tests to check that old notation is still properly supported.

[J-N-K]

Thanks, I'll wait a bit if @mherwege wants to make comments. This is really a nice improvement.

[lolodomo]

@openhab/ios-maintainers and @openhab/android-maintainers : please check any potential impact on the iOS and Android apps.

[maniac103]

As it's backwards compatible, there shouldn't be any direct impact, should there?
However, I think it might be worth bumping the REST API version for this.

[lolodomo]

As it's backwards compatible, there shouldn't be any direct impact, should there?

Probably not but there was one little in Basic UI.

[mueller-ma]

In the Android app you can press on a chart to view it full screen and change the period. This list of periods needs to be extended.

[maniac103]

... but only if supported by core, hence the request to bump server API version.

[lolodomo]

In the Android app you can press on a chart to view it full screen and change the period. This list of periods needs to be extended.

Yes, same in Basic UI. But my option is to provide a subset of predefined periods, the most common. I have slightly extended the predefined options.

[lolodomo]

.. but only if supported by core, hence the request to bump server API version.

I have no idea where this version is set. @J-N-K any idea ?

[lolodomo]

And we are not talking about the REST API but just the chart servlet API.

[maniac103]

I have no idea where this version is set.

Here

And we are not talking about the REST API but just the chart servlet API.

For all intents and purposes of client apps the chart API is part of the REST API. Additionally, it doesn't have separate versioning.

[J-N-K]

I agree, this should probably bump API version. @openhab/core-maintainers any other opinion?

[mueller-ma]

A bit OT, but could be the reason for a version bump added as javadoc where the version is defined?

[lolodomo]

@mherwege : the sitemap generator in Main UI will probably require an enhancement, there is currently a restricted list of values allowed I believe.

[mherwege]

LGTM I will look into adjusting the UI sitemap definition checks.

[kaikreuzer]

@openhab/core-maintainers any other opinion?

No, I'm fine with increasing the API version for this.

could be the reason for a version bump added as javadoc where the version is defined?

Would make sense to easily figure out what a new version brought. @lolodomo, could you add such a comment here?

[lolodomo]

@kaikreuzer @J-N-K : API version increased.

[maniac103]

API version increased.

While you're at it, maybe add the full changelog?

/**
 * Version 1: initial version
 * Version 2: include invisible widgets into sitemap response (#499)
 * Version 3: Addition of anyFormat icon parameter (#978)
 * Version 4: OH3, refactored extensions to addons (#1560)
 * Version 5: transparent charts (#2502)
 * Version 6: extended chart period parameter format (#3863)
 */

[lolodomo]

I added the provided full history of the versions.

[kaikreuzer]

Many thanks, @lolodomo!

[lolodomo]

While I was searching where is defined the default configuration for RRD4j persistence service, I discovered the class RRD4jChartServlet with again a restricted list of periods:
https://github.com/openhab/openhab-addons/blob/main/bundles/org.openhab.persistence.rrd4j/src/main/java/org/openhab/persistence/rrd4j/internal/charts/RRD4jChartServlet.java#L96

I was not expecting such a class. I thought the class in core framework was enough. I have now a doubt and I should check again that the new duration format is properly used.
I will also search all classes in add-ons implementing ChartProvider. Edit: only in RRD4J persistence service.