Upgrade XChart to 3.8.4

[J-N-K]

Closes #3279

Not yet ready for merge. See the discussion in attached issue.

Edit: Seems to be ok now.

Signed-off-by: Jan N. Klug github@klug.nrw

[Flole998]

@J-N-K Good news, there's a new xchart release now that should contain the fix

[openhab-bot]

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

https://community.openhab.org/t/openhab-4-0-milestone-discussion/145133/514

[lolodomo]

This upgrade introduced a bug. The X axis legend is missing when chart time frame is one day

It apparently works well for some of the other time frames.

Do we revert this upgrade ?

[J-N-K]

@Flole998 Any idea?

[Flole998]

After openHAB is running for a while charts aren't loading at all for me. And after trying to load them repeatedly the entire openHAB webserver stopped responding. I suggest reverting and considering alternatives to xchart like https://github.com/HanSolo/charts

[Flole998]

Apparently the charts not loading is more a RRD4J issue in my case, the data isn't even persisted for some reason. Once I resolved that I can have a look at what's going on with the charts.

[Flole998]

This upgrade introduced a bug. The X axis legend is missing when chart time frame is one day

So coming back to this after I solved/worked around the other issue for now. I would suggest to double-check what parameters are passed to XChart. It seems like it didn't bother to add any ticks to the chart. Unless we can work around this in openHAB (there are dozens of options you can pass to XChart) an updated XChart version most likely won't be available for the new release. I will try to reproduce this outside of openHAB, then I might be able to provide hints on what could be tried. I wonder if slightly going above 1d (like 1d1h or 23h, I know this doesn't exist but let's assume it did) would already fix it.

[J-N-K]

I was wondering if the could pass something like 24h on case of 1d

[Flole998]

That's actually not passed to xchart at all AFAIK. Xchart gets a list of time-value-pairs and does it's magic then. And it gets label format info aswell.

[Flole998]

I was instantly able to reproduce it on XChart by using one of their examples, making the X-Axis a Date-ArrayList, making it 24 hours and rendering it. For 1 hour it works normally, 24 and the labels are gone.

[Flole998]

I found the issue now, it can be fixed in openHAB (although it is probably not as nice as a fix in XChart): XChart is now checking if all labels are unique, but if we plot 24 hours the first and last label are equal (for example 20:00 yesterday and today). If we would add the day aswell it would work perfectly fine (Fr. 20:00 and Th. 20:00 are not equal). This is what previously caused the issues in #3279 described by @J-N-K, and my fix fixed the exception but didn't correctly revert back to the first thing it tried.

I will try to get this improved over at XChart, in case it won't be done on time you guys can decide if downgrading or changing the pattern for 1d is better ("EE HH:mm" works fine). Making the labels longer obviously causes less ticks on the axis, so the chart is harder to read.

[J-N-K]

Since this release fixes other issues, I would prefer to keep it. We can decide if we compile a forked version of the Fix does not arrive in time.

[Flole998]

Keep an eye on knowm/XChart#774, I tagged the maintainer who can merge aswell as the repo owner who can merge and do a new release. Let's hope they see it and decide it is important enough for a hotfix-release.

[Flole998]

That was fast, it's merged, now only a new release is needed.

[lolodomo]

3 days remaining...

@J-N-K @kaikreuzer what do we decide?
Either revert this PR or build and use our own version including the fix?

[Flole998]

They offer snapshot builds at XChart aswell and a ready-to-use jar can be downloaded. I am not sure if it's possible to force use of a specific snapshot build though without downloading it and specifying the jar file manually. So we don't even need to build it, just somehow use it.

[kaikreuzer]

Snapshots are not really nice to integrate.
What issues would we have if we simply revert the change?

[lolodomo]

I am not aware of any issue declared by OH users with the old version we were using in OH 3.4, at least not on Github.

[Flole998]

I'm also not aware of any issues, it's just that the old version is really really old as various issues prevented upgrading until this release was at least mostly working.

To sum it up, options available are:

• Add "day" to the labels, that would mean longer Texts, less Labels and would make the charts harder to read
• Upgrade to the snapshot/include the specific jar somehow (@J-N-K mentioned that as an option)
• Revert this and try again later (I don't think this has any huge negative consequences). A simple revert isn't enough though as the version info in some readme file was incorrect and this was fixed aswell

While I'd love to see the snapshot and possibly some bug fixes (I believe there have been a few very rare cases where things went wrong and caused an exception), I think it can wait just a little longer and could be reverted.

[Flole998]

Just to keep you guys updated: Over at XChart the maintainer just said he will do a release "this week sometime".

[kaikreuzer]

Might just be too late for 4.0.0 then.
@Flole998 As you seem to know what exactly needs to be done for a full revert: Could you prepare an according PR?

[Flole998]

Sorry, I didn't see it until now. What you did is basically correct, just for some reason the version in the NOTICE file was incorrect before. I prepated #3721 to fix that, it doesn't affect the code or any functionality, just the "documentation".