Save stateDescriptionPattern from channel type for UoM Items & Save UoM metadata when adding from model

[mherwege]

When creating a new Number with Dimension item from a channel link, the stateDescriptionPattern does not propose the state description pattern from the channel type as the default.

This PR introduces this.

It only saves this state description pattern from the channel type to metadata of the item if it is not the default state description pattern for a Number type.

Subsequent edits of the item will present a disabled state description pattern input.
If no state description field is set, a blank field will be presented.
Reasons are:

• A state description can contain more than a pattern.
  Saving only the pattern would override the rest of the stateDescription.
• An Item can be linked to multiple channels with (possibly conflicting) stateDescription.
  The only solution would be for the user the set it on the item and override the channel stateDescriptions.

See openhab/openhab-addons#16531 (comment).

This PR also now saves the unit (and state description pattern) when creating equipment or point from a thing.
See discussion https://community.openhab.org/t/units-are-not-propagated-when-creating-items/154748.
So far, changes were only saved when editing or creating items directly or from thing channels, not when creating from things in the model.

[relativeci]

Job #1897: Bundle Size — 10.56MiB (+0.01%).

ef546be(current) vs 1232bf5 main#1894(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics   2 changes 1 regression

	Current Job #1897	Baseline Job #1894
Initial JS	1.86MiB(~+0.01%)	1.86MiB
Initial CSS	607.87KiB	607.87KiB
Cache Invalidation	18.3%	17.8%
Chunks	223	223
Assets	246	246
Modules	2861	2861
Duplicate Modules	141	141
Duplicate Code	1.73%	1.73%
Packages	95	95
Duplicate Packages	2	2

Bundle size by type   1 change 1 regression

	Current Job #1897	Baseline Job #1894
JS	8.75MiB (+0.01%)	8.75MiB
CSS	890.07KiB	890.07KiB
Fonts	526.1KiB	526.1KiB
Media	295.6KiB	295.6KiB
IMG	140.74KiB	140.74KiB
HTML	1.24KiB	1.24KiB
Other	871B	871B

---

View job #1897 report View mherwege:stateDescription branch activity View project dashboard

[mherwege]

From linked PR: openhab/openhab-addons#16531

I understand your point. But please tell me why you think the state pattern element in thing xml exists if not for the purposes of (helping) creating items? It seems to me that 455 binding developers may have been wasting their time writing state pattern elements if the whole concept has no purpose.

I think this whole concept is unfortunately messy, but difficult to change without breaking things.
Current behaviour, if you have one item connected to 2 channels with different channel type and state description pattern, no state description pattern set on the item (in metadata, a config file) or on a sitemap (for the sitemap based UI's), one of the channel type state descriptions will be used for formatting. Which one is entirely random.
If the item is only linked to one channel, it should use the state description pattern of the channel type if nothing else is set, even if not visible in the UI. That definitely is the most common case. Typically, very few items get linked to multiple channels.

This PR proposes to show the channel type state description pattern when originally creating an item linked to a channel of a thing. That gives users the possibility to set something else if that makes sense. It avoids changing item display state update behaviour.
An alternative could be to always set the channel type state description through the UI in metadata (so create a specific state description pattern). In that case, not just the state description pattern should be set, but also the other parameters from the state description should be copied over.
Of course, this alternative solution would not apply if you first create the item, and then link it to (one or more) channels. In that case, it would still be the behaviour as today.

[rkoshak]

I'm sure you've already thought about this but one little wrinkle to keep in the back of your mind as you work this is sometimes the type of the destination Item may be dramatically different from the type the Channel (and therefore the state description pattern pushed by binding) may not even make sense.

For example, if you have a timestamp profile on the link between a Number:Dimensionless Channel and a DateTime Item, %.0f %% as a State Description pattern is worse than nonsense, it generates an error. I think/hope that pattern pushed by the binding is ignored in this case but want to make sure we don't reintroduce errors in this particular use case.

I don't think there's a problem here, just wanted to mention it.

[mherwege]

@rkoshak Thank you for the insight. I didn’t consider that so far. But with the change as implemented so far, it shouldn’t change the current behavior. But I honestly don’t know how the system currently copes with it. Does it ignore channel type state descriptions when a profile changing to a different type is used?
The risk is higher when the code would take the state description from the channel type and force it on the item, store it in the item metadata.
As long as it doesn’t do that by default, behavior does not change.

[rkoshak]

Does it ignore channel type state descriptions when a profile changing to a different type is used?

Awhile back it was actually causing a bunch of problems because some bindings would push state description "options" which were silently applied to the Item but because the Item type changed due to a profile it messed up everything in the state description. This was a couple years ago now I think.

I was under the impression that when that problem was sorted it also addressed the pattern. But I don't know how to test it. My searching has so far failed to show the original issue or PR.

I don't think it forces the metadata to be created though. I think it's only kept in memory somehow.

[mherwege]

My searching has so far failed to show the original issue or PR.

Probably this: openhab/openhab-core#2283
But it doesn't look like that problem has been solved.

And here is another issue with this when using profiles: openhab/openhab-core#4050

[rkoshak]

Those both look like the same overall issue but neither is the one I'm thinking about. But I could be mistaken about my memory that it got fixed if these are open. And it could have been a forum thread I'm remembering. There's so much to keep track of.

[andrewfg]

@mherwege I was wondering how to make progress on this topic. And how to resolve the prior as yet unresolved issues.

I suggest that the code must comply with the "functional specification" for state descriptions as written in the state description developer document

This functional specification is either a) fully correct, b) fully wrong, or c) missing something. If the functional specification is fully correct, then this PR MUST follow that verbatim. Otherwise if the functional specification is wrong or missing something then we should first edit and agree upon the functional specification BEFORE we try to review the code in this PR. => WDYT?

[mherwege]

@andrewfg This PR does not have the ambition to resolve all issues with state descriptions when applying profiles. It merely tries to capture the state description pattern from the channel type and apply it when creating a new item with dimension number and a type and make sure it is applied to the item. It avoids mismatches in the UI and unpredictability because of mismatches in channel type state description patterns. The UI naively supposed so far a %0.f %unit% pattern when nothing was set in the UI. From my perspective, this PR still complies with the functional specification. All other parameters in the state description should not be touched by this PR. Where do you think I go wrong with this reasoning?
Second element in this PR is to make saving units and state description patterns work properly when creating items from the model.

[andrewfg]

^
@mherwege I am not trying to drive a polemic here. But I do want to find a path forward. I don't know the webui language so for me it is easier to discuss the doc. If the doc and your PR code are mutually consistent then I think we should merge your PR immediately. But if there are inconsistencies then we should identify those, and map a path to resolve them. Such resolution could be either by changing the doc, and/or by changing the ui code (in this PR and/or in other PRs). But in all cases I would suggest that we agree on the wording of the doc BEFORE we try to interpret and merge your code..

[mherwege]

If the doc and your PR code are mutually consistent then I think we should merge your PR immediately. But if there are inconsistencies then we should identify those, and map a path to resolve them.

@andrewfg I still think the doc and this PR are mutually consistent. I spend some extra time testing.

This PR will only set the state description pattern (not the full state description) for a Number Dimension item when creating an item from a thing channel. It copies the state description pattern from the channel type and saves that, or saves a new value set by the user.
If the user links the same Number Dimension item to another channel (or links an existing item to a channel) it will not be set or overwritten.
State description patterns for any other item type will not be touched by this PR. They are not available in these forms and would need to be explicitely set through metadata.
State descriptions are used to calculate the display state of items to be used in the UI. The state description used is a merge of the item state description and the state descriptions on the channels the item is linked to. The item state description takes precedence when the same state description field exists on the item and the channel. I tested with the following example:

Channel type for a channel on a binding (I used nikohomecontrol):

	<channel-type id="setpoint">
		<item-type>Number:Temperature</item-type>
		<label>@text/channelSetpointLabel</label>
		<description>@text/channelSetpointDescription</description>
		<category>Temperature</category>
		<tags>
			<tag>Setpoint</tag>
			<tag>Temperature</tag>
		</tags>
		<state min="0" max="100" step="0.5" pattern="%.1f %unit%"/>
	</channel-type>

I create an item linked to a channel on the binding and explicitly set a state description pattern of %.5f %unit%.
I then called the REST API (/items/{itemname} endpoint, with the following result:

{
  "link": "http://localhost:8080/rest/items/Thermostat_Setpoint",
  "state": "NULL",
  "stateDescription": {
    "minimum": 0,
    "maximum": 100,
    "step": 0.5,
    "pattern": "%.5f %unit%",
    "readOnly": false,
    "options": []
  },
  "unitSymbol": "°C",
  "metadata": {
    "unit": {
      "value": "°C"
    },
    "semantics": {
      "value": "Point_Setpoint",
      "config": {
        "relatesTo": "Property_Temperature"
      }
    },
    "stateDescription": {
      "value": " ",
      "config": {
        "pattern": "%.5f %unit%"
      }
    }
  },
  "editable": true,
  "type": "Number:Temperature",
  "name": "Thermostat_Setpoint",
  "label": "Setpoint",
  "category": "Temperature",
  "tags": [
    "Temperature",
    "Setpoint"
  ],
  "groupNames": []
}

You clearly see the effect of merging the state description.

So I see no inconsistency between documentation and what I try to do here. The only thing you could argue about is that no warning is given on the effect of merging state descriptions (which is already the case without this PR) on the resulting state description in the documentation.

If you feel differently, please let me know. I will be happy to change this PR. But at this time, I don't see how it would invalidate the documentation and current contract in the slightest way.

[andrewfg]

@mherwege many thanks for the explanation above.

Based on that, I think the following..

1. The doc does not need to be changed.
2. This PR => LGTM

Further questions..

3. Do you have any idea if the 'min', 'max' and 'step' parameters are also properly taken over? And if so, where? Maybe there is a need for another PR to cover that?
4. As your PR here was partly triggered by my work on a binding with humidity channels having the unit "%rH" I wonder if you have any thoughts concerning my UoM add units for humidity openhab-core#4157 ??

[andrewfg]

LGTM

[mherwege]

3. Do you have any idea if the 'min', 'max' and 'step' parameters are also properly taken over? And if so, where? Maybe there is a need for another PR to cover that?

My understanding is it is part of the state description and will get merged with the rules I explained. There is no attempt in this PR to do anything with the state description beyond the state description pattern for Number Dimension items.
In the example above, you see the REST response returning min, max and step from the channel. They are not set on item metadata.
There are issues with it though, but they are outside of the scope of this PR. There is no unit set on min, max and step. So the behavior changes when changing the unit (same step in other unit). Solving the issue is out of scope for this PR. And it may also be tricky to solve (min, max and step are probably device dependent and may or may not always be in a specific unit).

[florian-h05]

After reading though the discussion in this PR and the docs, I am also fine with the current state of this PR and will review this PR once I’m back in action.

I have a few comments on the above:

The only thing you could argue about is that no warning is given on the effect of merging state descriptions (which is already the case without this PR) on the resulting state description in the documentation.

Can you please improve the docs to mention that?

Do you have any idea if the 'min', 'max' and 'step' parameters are also properly taken over? And if so, where? Maybe there is a need for another PR to cover that?

Seems like the UI does not always take these into account, for example does oh-slider not get its min and max values from the state description of the Item: https://github.com/openhab/openhab-webui/blob/4a0054a1f020cbec8a1590d550e60542826d8504/bundles/org.openhab.ui/web/src/components/widgets/system/oh-slider.vue
at least when manually adding a slider widget.
If it is automatically chosen by the UI as the widget for an Item, this might be different (would need to be tested).
The problem is that getting the state description metadata of an item would be an extra request to REST, these requests could easily sum up.

[lolodomo]

I suggest that the code must comply with the "functional specification" for state descriptions as written in the state description developer document

A small fix in this documentation would be necessary.

The pattern attribute can be used for Number and String items.

This is also applicable to DateTime items.

[lolodomo]

It only saves this state description pattern from the channel type to metadata of the item if it is not the default state description pattern for a Number type.

@mherwege : can you check you are aligned with my PR openhab/openhab-core#4175 for the default pattern.

As a reminder, for a number with dimension item, ChannelStateDescriptionProvider will set "%.0f" as default pattern if none is provided by the channel.
With my PR, ChannelStateDescriptionProvider will no more set any default pattern but DefaultStateDescriptionFragmentProvider will set "%.0f %unit%" as default pattern.

The current order when merging state descriptions from the different providers, from the highest priority to the lowest priority:

1. MetadataStateDescriptionFragmentProvider
2. GenericItemProvider (the .items file)
3. ChannelStateDescriptionProvider

With my change, it will become:

1. MetadataStateDescriptionFragmentProvider
2. GenericItemProvider (the .items file)
3. ChannelStateDescriptionProvider
4. DefaultStateDescriptionFragmentProvider

[mherwege]

@mherwege : can you check you are aligned with my PR openhab/openhab-core#4175 for the default pattern.

I am aligned with that and I believe both complement each other well.

Note that this PR at its current state only does impact the state description pattern for items with dimension. With the changes in openhab/openhab-core#4175 a next step would be to expose the state description pattern for all items and set it in he same way as for items with dimension.

This would solve the issue of potential conflicting channel state description patterns when linking items to two channels with different state description patterns (at least in the UI when creating items from a channel). It effectively promotes one of the state description patterns to the highest priority level by setting it explicitly in metadata.

[lolodomo]

I am aligned with that and I believe both complement each other well.

Normally with my PR, you don't need to set a default state pattern if not provided by channel.

[mherwege]

Normally with my PR, you don't need to set a default state pattern if not provided by channel.

This PR only sets it when creating an item from a channel or thing (model), so not for new unlinked items.

[florian-h05]

LGTM, thanks!

Just one thing: IMO we should disable the state description field for existing Items, because changing the state description is a potentially dangerous operation. If you change the pattern, the rest of the metadata is overwritten, so you may loose your defined pattern etc. by mistake.
I will soon push a commit to disable this field for existing Items and show a message:

Pattern can only be changed from the state description metadata page after Item creation!

[florian-h05]

@mherwege Please let me know if you agree!

[mherwege]

@mherwege Please let me know if you agree!

Wouldn’t it be better to merge with the rest of the state description if one already exists on save to avoid this?
E.g. if you change the unit, you may also want to change the change description pattern. Not showing the second field makes it very confusing again.

[florian-h05]

Wouldn’t it be better to merge with the rest of the state description if one already exists on save to avoid this?

This would allow to change the state description pattern easily when changing the unit, but in case you have a min/max you would have to go the state description metadata page anyway to change them. And „hiding“ min/max when allowing to change the state description pattern could cause confusion.

Not showing the second field makes it very confusing again.

It is not hidden, only disabled with a hint to go to the state description metadata page.

IMO it is safer to disable the state description pattern field in the item-form, because you then force the user to see and think about the whole state description when he wants to change the pattern afterwards.

[mherwege]

It is not hidden, only disabled with a hint to go to the state description metadata page.

IMO it is safer to disable the state description pattern field in the item-form, because you then force the user to see and think about the whole state description when he wants to change the pattern afterwards.

OK to disable.

while I started this for the state description pattern in the context of UOM, I still think there is further room for improvement. It is not only the state description pattern that can contain conflicting information when linked to multiple channels, but the whole of the state description. For the pattern, it is solved here in the UI by copying he pattern from the channel when creating the item, thereby giving a higher priority to the first one (with the ability to change). Something similar would probably be good to do for the rest of the state description. But I considered that beyond he scope of his PR.

[florian-h05]

I still think there is further room for improvement.

Agreed and your proposal for another PR sounds nice 👍

[andrewfg]

@mherwege many thanks for the contribution!