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

Fix QuantityType dimensionless one and time formatting #4169

Merged
merged 1 commit into from May 18, 2024

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Apr 4, 2024

Resolves #4166
Resolves #4163

This PR tries to resolve two issues with QuantityType:

  1. Updating a Number:Dimensionless Item with a value of unit one, when the unit is set in metadata to another unit, will not update correctly. The proposed solution is to remove the special handling of unit one and make the unit part of the string again. It can be removed from visualisation using a state description.
  2. Formatting a QuantityType tries to use the DateTime String formatting methods. When it fails it may output a UTC date. Durations will also not be correctly formatted when days/hours/minutes/seconds are above the month (31)/day (24)/hour (60)/minute (60) limits. In this PR, the formatting strings are preserved, but interpreted specifically for time with a relative scale (durations), rather than an absolute time.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Copy link
Contributor

@andrewfg andrewfg left a comment

Choose a reason for hiding this comment

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

LGTM thankyou.

@andrewfg
Copy link
Contributor

andrewfg commented Apr 5, 2024

@mherwege many thanks. I like the extra bonus for the duration formatting.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks!

@kaikreuzer kaikreuzer merged commit 6b5eed7 into openhab:main May 18, 2024
3 checks passed
@mherwege mherwege deleted the postupdate_one branch May 18, 2024 14:43
@kaikreuzer
Copy link
Member

We have a few failing tests in the add-ons now, see https://ci.openhab.org/job/openHAB-Addons/lastCompletedBuild/testReport/.

Might these be related to this change, @mherwege?

@mherwege
Copy link
Contributor Author

It probably is. I am investigating. It looks like the mqtt tests assume a formatting of a QuantityType with format pattern %s just drops the unit. It may have done that in the past because in core, first a conversion to BigDecimal (which dropped the unit) was attempted before passing the value verbatim (with unit). I don't think the unit should be dropped like that when formatting a Number with Dimension.

@mherwege
Copy link
Contributor Author

mherwege commented May 19, 2024

@kaikreuzer I have made a change to the MQTT binding that resolves the issue caused by the change in this PR.

The fundamental problem is that the format method on a type is used both for representation purposes in UI's and internal processing when converting to a String. Some logic depends on the formatting to be done in a certain way, which blocks improvements in the UI, and the other way around. I hope this change resolves the issue. At leasts the failing tests work again, so it should keep backward compatibility.

@kaikreuzer
Copy link
Member

@mherwege Many thanks for taking care of it so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants