Slider and Setpoint with Number:Temperature broken after upgrading to OpenHAB 3.4.0 #1605

goligo · 2022-12-24T02:17:31Z

Which UI are you reporting an issue for?

Basic UI
HABPanel
HABot
CometVisu

The problem

After updating to OpenHAB 3.4.0 the slider widgets for controlling the target temperature do no longer work. Moving the slider has no effect, the temperature is neither updated on the UI, nor is any event for changing the item state shown in the event log.

The reason is that request triggered by the slider is receiving a "400 Bad Request" response. When checking the request payload, I saw that the slider is sending "19 %unit%" as the update value. This seems to be causing the error, when I change the pattern on the item explicitely to "%.1f °C", the issue does no longer occur.

Expected behavior

When moving the slider, the item state should be updated and the target temperature should be changed.

Steps to reproduce

Create an item of type "Number:Temperature" and set the pattern to "%.1f %unit%"
Create slider in the basic UI for this item
Try to move the slider and check the browser network trace

Your environment

runtimeInfo:
  version: 3.4.0
  buildString: Release Build
locale: de-DE
systemInfo:
  configFolder: /etc/openhab
  userdataFolder: /var/lib/openhab
  logFolder: /var/log/openhab
  javaVersion: 11.0.10
  javaVendor: Azul Systems, Inc.
  javaVendorVersion: Zulu11.45+27-CA
  osName: Linux
  osVersion: 5.10.63-v7+
  osArchitecture: arm
  availableProcessors: 4
  freeMemory: 37094744
  totalMemory: 194772992
  startLevel: 100
bindings:
  - avmfritz
  - exec
  - goecharger
  - http
  - icalendar
  - ipp
  - mail
  - mqtt
  - network
  - ntp
  - shelly
  - telegram
  - tr064
  - vitotronic
clientInfo:
  device:
    ios: false
    android: false
    androidChrome: false
    desktop: true
    iphone: false
    ipod: false
    ipad: false
    edge: false
    ie: false
    firefox: false
    macos: true
    windows: false
    cordova: false
    phonegap: false
    electron: false
    nwjs: false
    webView: false
    webview: false
    standalone: false
    os: macos
    pixelRatio: 1
    prefersColorScheme: dark
  isSecureContext: false
  locationbarVisible: true
  menubarVisible: true
  navigator:
    cookieEnabled: true
    deviceMemory: N/A
    hardwareConcurrency: 4
    language: de
    languages:
      - de
    onLine: true
    platform: MacIntel
  screen:
    width: 2560
    height: 1440
    colorDepth: 24
  support:
    touch: false
    pointerEvents: true
    observer: true
    passiveListener: true
    gestures: false
    intersectionObserver: true
  themeOptions:
    dark: dark
    filled: true
    pageTransitionAnimation: default
    bars: light
    homeNavbar: default
    homeBackground: default
    expandableCardAnimation: default
  userAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36
    (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36
timestamp: 2022-12-24T02:13:18.376Z

Browser console

smarthome.js:1          POST http://openhab:8080/rest/items/HaupthausBadLinks_Zieltemperatur 400 (Bad Request)
l @ smarthome.js:1
I @ smarthome.js:1
(anonymous) @ smarthome.js:1
o.call @ smarthome.js:1
a @ smarthome.js:1

Browser network traffic

Additional information

Slider was working fine with OpenHAB 3.3.0, issue only occured after upgrading to 3.4.0

lolodomo · 2022-12-24T11:30:29Z

There was no change about that directly in Basic UI in OH 3.4 I am aware. Maybe this is related to the stuff with UoM done in the core framework. @J-N-K : do you thing itemUIRegistry.getUnitForWidget(widget) could now return "%unit%" while not before ? I am not sure it was not possible before, but the current issue seems to say it.
By the way, I could agree that Basic UI should handle better this value in case it could occur now.

J-N-K · 2022-12-24T11:37:49Z

I don't see anything in the code that could have changed, and actually I don't see how %unit% could ever end up with the label There is a check that takes the unit symbol if the widget label has %unit% and NumberItem.getUnitSymbol (which is called if the placeholder is found) uses NumberItem.getUnit which returns a Unit, not a String.

lolodomo · 2022-12-24T12:08:18Z

@goligo : can you show what you got inside events.log (filtering on your item) before opening BasicUI and when moving the slider with BasicUI.
How is looking the item value in MainUI ?
Is your item defined in a config file ? Please show how it is defined.

The only explanation I can find is that for an unknown reason, your item state contains '%unit%" when opening the page in BasicUI.

lolodomo · 2022-12-24T12:10:45Z

I don't see anything in the code that could have changed, and actually I don't see how %unit% could ever end up with the label There is a check that takes the unit symbol if the widget label has %unit% and NumberItem.getUnitSymbol (which is called if the placeholder is found) uses NumberItem.getUnit which returns a Unit, not a String.

Ok so we have to understand how "%unit%" appeared in the item state.

If you say it should never happen, there is nothing to change in BasicUI.

goligo · 2022-12-24T12:37:52Z

%unit% does not appear in the label, it is not shown on the UI, the value is formatted as expected as "19 °C". Only when moving the slider the payload sent back to the server does contain %unit% instead of the actual unit.

I don't know whether it already was sent like this before, but the server could handle it, or whether it was sent as °C before.

lolodomo · 2022-12-24T13:04:48Z

What is certain is that it is not BasicUI that adds "%unit%" by itself. BasicUI just extracts the unit and saves it to uses it letter when sending a command.
I have no idea what's going on.

lolodomo · 2022-12-24T13:05:37Z

This item is handled by what binding ?
Are you managing its value/unit inside a rule ?

goligo · 2022-12-24T14:42:02Z

Item is linked to targetTemp channel of the Shelly binding. I don't do any custom unit handling.

goligo · 2022-12-24T22:56:06Z

I can confirm the issue is not specific to the slider widget, but happens with any widget trying to modify the value. Unit %unit% is taken from data-unit="%unit%" attribute in the rendered HTML of the widget.

goligo · 2022-12-24T23:44:24Z

Seems to be the same issue as or related to #765

J-N-K · 2022-12-25T00:10:27Z

In fact it is the SliderRenderer that adds (or better does not properly replace) the %unit%. This happens when the unit returned by NumberItem.getUnit returns null. Before OH3.4 this was the system default unit if %unit% was set. This is obviously wrong because %unit% means "the unit of the state", so it it worked before due to a bug in openhab-core that was fixed in 3.4.

goligo · 2022-12-25T00:13:21Z

As far I understand the issue, @J-N-K recently made a change, to return null for getUnit, when the state description contains the default unit placeholder

https://github.com/J-N-K/openhab-core/blame/7b9bd6f0214725d7a834ec2fe852b53ad8eba530/bundles/org.openhab.core/src/main/java/org/openhab/core/library/items/NumberItem.java#L224-L227

The Slider and Setpoint renderers contain code to not replace the unit placeholder, if the unit symbol is null

openhab-webui/bundles/org.openhab.ui.basic/src/main/java/org/openhab/ui/basic/internal/render/SliderRenderer.java

Lines 72 to 74 in eab99f0

 if (unit != null) { 

 snippet = snippet.replace("%unit%", unit); 

 }

So %unit% stays in the data-unit attribute and is later concatenated to the value, before it is sent back to the server.

Ah, the response from J-N-K came in while I was writing mine. So how it should be fixed? Which unit should be used by the basic UI and where does it get it?

goligo · 2022-12-25T00:16:00Z

@J-N-K So if %unit% means "the unit of the state", shouldn't getUnit not return this unit, instead of null?

J-N-K · 2022-12-25T00:26:40Z

What do you return if the state is UNDEF? In that case null would probably be correct, but result in the same issue you see now.

Another issue (I‘m not 100% sure here): if you do not reload the page, the widget will not change its unit. But with %unit% this can happen with every state update (900 m / 1 km).

The correct solution would be to dynamically change the unit depending on the actual state (this obviously has to be done by the UI, since the item is asked only once) or show no unit at all.

To fix it for now, set the unit in the state description instead of %unit%.

goligo · 2022-12-25T00:32:37Z

Yes, I could fix it locally by setting the unit explicitly in the state description. However your change basically broke every basic UI Setpoint and Slider using a Number type with dimension.

While I understand your example for units of distance, mass or volume, it does not apply to temperatures. Dependent on where you live, you either want °C or °F.

If the state is UNDEF, and the pattern doesn't contain an explicit unit, it should return the default unit for the type.

J-N-K · 2022-12-25T00:37:02Z

My change did not break anything. It was BasicUI relying on erratic behavior of core.

Zhat aside: your proposal would break a lot of things inside openHAB.

goligo · 2022-12-25T00:42:58Z

This is a little too simple, isn't it? I fully agree that there is an issue is in the basic UI renderer, but that doesn't change the fact that it did work before, and due to your change UIs are now broken.

Despite of that, in OpenHAB 3.3.0 getUnit did return the system default unit, and this was the expected behaviour in many or most cases. I don't see how this would break a lot of things? Let aside the UNDEF case - why can't getUnit not return the same unit it did use for formatting the value?

goligo · 2022-12-25T00:46:31Z

How should the basic UI find out which unit to use, if it does not get it from the item? Can it just omit the unit completely and you will then assign the default unit on the server side?

J-N-K · 2022-12-25T00:53:48Z

From the state, as the user requested by setting %unit%.

Returning the system unit is wrong. I don’t see why we should revert a fix to re-introduce a bug. Also there is a discussion on the forum about UoM handling where everybody agrees that presentation (from state description) and item unit should be separated and I believe that this will be the case in OH4.

goligo · 2022-12-25T01:11:20Z

I don't have enough insight to argue about architectural decisions in OpenHAB. I just see that many UIs are broken now, the issue is not easy to understand and a lot of work to fix for people.

What do you mean with "from the state"? When the current state is "19 °C", then you want the UI to parse the unit from the string themselves? Although you exactly know the unit, as you used it to format the string?

Would it be an option to provide an additional getter for the default unit, so in case the the state is UNDEF, the UI still has a unit it can use?

goligo · 2022-12-25T02:18:23Z

BTW, there might be an additional issue - if I understand the code correctly, if I don't have a state description and provide no pattern, it should return the default unit from the UnitProvider. However, if my item doesn't have a state description, I still have the same issue.

J-N-K · 2022-12-25T10:03:00Z

Indeed, and that proves that unit handling is required in the UI anyway.

lolodomo · 2022-12-26T12:02:35Z

In fact it is the SliderRenderer that adds (or better does not properly replace) the %unit%. This happens when the unit returned by NumberItem.getUnit returns null.

Ok, finally I understand, the "%unit%" in the snippet code is not replaced because the method now returns null rather than a unit.

Before OH3.4 this was the system default unit if %unit% was set. This is obviously wrong because %unit% means "the unit of the state", so it it worked before due to a bug in openhab-core that was fixed in 3.4.

Maybe the impact to our official OH components of such change in core framework should have been at least evaluated ?!

J-N-K · 2022-12-26T12:07:48Z

I guess it's impossible to know that other code depends on bugs to work.

Probably that would have been found if there was proper test code. Unfortunately besides openhab-core we have very little tests, neither unit tests nor integration tests.

lolodomo · 2022-12-26T12:09:58Z

Another issue (I‘m not 100% sure here): if you do not reload the page, the widget will not change its unit. But with %unit% this can happen with every state update (900 m / 1 km).

The front code is updating the widget unit at each new received state update (when the page is opened).

The correct solution would be to dynamically change the unit depending on the actual state (this obviously has to be done by the UI, since the item is asked only once) or show no unit at all.

If I correctly understand what you say, this is already done in my opinion.

I just still do not understand why null was returned when the page is opened while it was said in the provided example that the item state was "19 °C ?
I can imagine null to be returned if item state is NULL or UNDEF, but not if it has a real value ?

lolodomo · 2022-12-26T12:14:35Z

I guess it's impossible to know that other code depends on bugs to work.

Just a grep of the updated method in UI repo should have be sufficient to identify a probable impact.

J-N-K · 2022-12-26T12:17:02Z

.getUnit returns the unit of the item, not the state. That is either set by the state description or the system default. There is no unit for the item if the user configures %unit% because that means "no default unit, take what the state says".

This situation would also already be that case in 3.3 for dimensions that did not have a default unit (e.g. flow rate). I must admit that not many people may control the flow of something from openHAB with a slider, but if someone had tried, he would have run into the same issue.

The fix was in a private method which is used in public methods.

lolodomo · 2022-12-26T12:18:15Z

I am ok to propose a fix in Basic UI but I would really like to understand why the method is returning null while the item state was "19 °C" ?

A simple fix would be to replace "%unit" by an empty string in the snippet when the method returns null.
Then the UI will send "19.5" instead of "19.5 %unit%".

J-N-K · 2022-12-26T12:23:49Z

To handle %unit% properly BasicUI should check if a unit is returned from .getUnit. If that is the case, it should use that. If no unit is returned, then it should check if the item has a unit. If that is the case (%unit% is used), this unit should be used for the display and also for sending values. If there is no unit returned, then it is a plain Number item and a slider without units should be rendered.

lolodomo · 2022-12-26T12:24:53Z

.getUnit returns the unit of the item, not the state. That is either set by the state description or the system default. There is no unit for the item if the user configures %unit% because that means "no default unit, take what the state says".

Understood.
No chance that this method can get the state unit when "%unit%" is set in the item pattern ?

I will check again in details but for me the unit is correctly updated when receiving a state update while the page is opened. The problem would be only when opening the page and the first initialization of the unit. Rather than setting unit to "", I will try to extract the unit from the item state.

lolodomo · 2022-12-26T12:26:19Z

Now that all the code has moved to Java 17, I have no idea if I can continue with a Java 11 dev env. If not, what are the steps to update our dev env ?

J-N-K · 2022-12-26T12:28:40Z

No. If you look at the forum discussion about OH4 UoM handling (https://community.openhab.org/t/uom-default-units-and-consequences/142360) there is broad majority supporting that the state description should not be taken into account when determining the unit of the item (but add a new metadata to configure that). This is also what is implemented in openhab/openhab-core#3248.

Regarding the dev environment: If you create the branch from the last 3.4.0 commit (i.e. immediately before the release) you can continue working with JDK 11. If you compile a jar then, it's usable in OH 3.4. What you need to update to use JDK 17 depends on your setup. I had to install JDK17 and change the project SDK in IntelliJ.

lolodomo · 2022-12-26T12:31:10Z

No. If you look at the forum discussion about OH4 UoM handling (https://community.openhab.org/t/uom-default-units-and-consequences/142360) there is broad majority supporting that the state description should not be taken into account when determining the unit of the item (but add a new metadata to configure that). This is also what is implemented in openhab/openhab-core#3248.

First, I want to push quickly a fix for all OH3.4 users (in the patch 1). We can see later what to change for OH4.

lolodomo · 2022-12-26T12:32:27Z

If this is better, I will push my fix directly in the 3.4.x branch.

lolodomo · 2022-12-26T12:37:37Z

Searching where is used getUnitForWidget in Basic UI, I see that selection and switch renderers could also require a change when linked to a Number item with options.

J-N-K · 2022-12-26T12:40:00Z

My guess would be that .getUnit never returns null (except for a plain Number) in OH4, but it'll probably not hurt to have code that checks for null.

lolodomo · 2022-12-26T13:01:11Z

If null, I will try to find the unit in item state.

First trying to switch to branch 3.4.x and build, hopping I can then run and test in Eclipse...

lolodomo · 2022-12-26T15:10:01Z

The fix should not be too much difficult to implement but I am first fighting since several hours to have Eclipse consider my code changes !

lolodomo · 2022-12-26T16:39:59Z

Looking again what is done for slider and setpoint widgets in the frontend (JavaScript):

for setpoint widget, unit that will be used later for commands is updated if a state update is received while the page is already opened; the unit is then taken from the item state and not from the label => risk to switch to a different unit ?
for slider widget, unit that will be used later for commands is not updated if a state update is received while the page is already opened.

The way it is done for slider widget looks better but there will be still a particular case to consider: %unit% was in the state pattern, the item state was NULL or UNDEF when the page was opened, the item state is then updated (with a unit) while the page is opened. The frontend code has then to save the received unit but it must take the one in label and not in item state.

Edit: in fact, it looks like the updated state is first converted by the framework to the unit mentioned in the state pattern or the default unit if no unit defined in state pattern. Only exception is if state pattern contains "%unit%". In that last case, there is no conversion as it is expected. So the way it is handled for the setpoint widget looks finally better to me.

lolodomo · 2022-12-26T18:24:07Z

While I am not succeeding to test in Eclipse, I tested by replacing BasicUI bundle directly in my production environment.

For setpoint widget, I succeeded to get the item state with the unit and so the problem is solved.

For slider widget, the problem is that the core framework includes a specific code in ItemUIRegistryImpl that returns a DecimalType rather than a QuantityType in method convertState called by method getState(Widgetw):
https://github.com/openhab/openhab-core/blob/e556fdf81bbf557ef8de2edc0c1141d7d60b18db/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java#L638

So if the state pattern contains "%unit%", with my current new code, no unit is then considered, the slider will generate a command without unit and then the unit is added by the core framework:

19:13:25.476 [INFO ] [openhab.event.ItemCommandEvent       ] - Item 'DemoTemperature' received command 68
19:13:25.485 [INFO ] [openhab.event.ItemStateChangedEvent  ] - Item 'DemoTemperature' changed from 10 °C to 68 °C

This could be a problem if the default unit does not match the real item state.
So I will need to override the method getState(Widgetw) form core framework !

J-N-K · 2022-12-26T18:30:06Z

IIRC the .getUnit did never return the unit of the state. It did return the system default pre-3.4.0, so there is no change in behavior regarding the unit handling.

The change in unit has always been a problem when %unit% was used, maybe it was even worse, because it would not have ben detected for sliders at all.

pattern In OH3.4, getUnitForWidget(Widget w) now returns null when the state pattern contains "%unit%". In that case, the unit must be searched in the item state. Fix openhab#1605 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2022-12-26T21:40:54Z

@goligo : you have a jar for testing the fix in the PR #1611 . I tested it myself directly over the official 3.4 release version.

goligo · 2022-12-26T23:46:27Z

jar testet on my installation - does work for items without state description/pattern as well as items with pattern containing %unit%.

goligo · 2022-12-27T06:32:34Z

@J-N-K sorry to bother you again, but can you quickly explain, why it is useful that each UI layer does implement this fallback logic to the state by itself, with the possibility to do it slightly different or introduce new bugs, instead of implementing it once properly on the item?

lolodomo · 2023-01-07T08:42:04Z

Closed by #1612

goligo added the bug Something isn't working label Dec 24, 2022

lolodomo added the basic ui Basic UI label Dec 24, 2022

goligo changed the title ~~Slider no longer working with Number:Temperature after upgrading to OpenHAB 3.4.0~~ control-changeSlider no longer working with Number:Temperature after upgrading to OpenHAB 3.4.0 Dec 24, 2022

goligo changed the title ~~control-changeSlider no longer working with Number:Temperature after upgrading to OpenHAB 3.4.0~~ control-change event with type Number:Temperature causing 400 error after upgrading to OpenHAB 3.4.0 Dec 24, 2022

goligo changed the title ~~control-change event with type Number:Temperature causing 400 error after upgrading to OpenHAB 3.4.0~~ Slider and Setpoint with Number:Temperature broken after upgrading to OpenHAB 3.4.0 Dec 25, 2022

lolodomo mentioned this issue Dec 26, 2022

[basicui] Fix setpoint/slider widgets when %unit% is used in state #1611

Closed

lolodomo closed this as completed Jan 7, 2023

Slider and Setpoint with Number:Temperature broken after upgrading to OpenHAB 3.4.0 #1605

Slider and Setpoint with Number:Temperature broken after upgrading to OpenHAB 3.4.0 #1605

Comments

goligo commented Dec 24, 2022

Which UI are you reporting an issue for?

The problem

Expected behavior

Steps to reproduce

Your environment

Browser console

Browser network traffic

Additional information

lolodomo commented Dec 24, 2022

J-N-K commented Dec 24, 2022

lolodomo commented Dec 24, 2022 • edited Loading

lolodomo commented Dec 24, 2022

goligo commented Dec 24, 2022 • edited Loading

lolodomo commented Dec 24, 2022

lolodomo commented Dec 24, 2022

goligo commented Dec 24, 2022

goligo commented Dec 24, 2022

goligo commented Dec 24, 2022

J-N-K commented Dec 25, 2022 • edited Loading

goligo commented Dec 25, 2022

goligo commented Dec 25, 2022

J-N-K commented Dec 25, 2022 • edited Loading

goligo commented Dec 25, 2022

J-N-K commented Dec 25, 2022 • edited Loading

goligo commented Dec 25, 2022

goligo commented Dec 25, 2022 • edited Loading

J-N-K commented Dec 25, 2022

goligo commented Dec 25, 2022

goligo commented Dec 25, 2022

J-N-K commented Dec 25, 2022

lolodomo commented Dec 26, 2022

J-N-K commented Dec 26, 2022

lolodomo commented Dec 26, 2022

lolodomo commented Dec 26, 2022

J-N-K commented Dec 26, 2022 • edited Loading

lolodomo commented Dec 26, 2022

J-N-K commented Dec 26, 2022

lolodomo commented Dec 26, 2022

lolodomo commented Dec 26, 2022

J-N-K commented Dec 26, 2022 • edited Loading

lolodomo commented Dec 26, 2022

lolodomo commented Dec 26, 2022

lolodomo commented Dec 26, 2022 • edited Loading

J-N-K commented Dec 26, 2022

lolodomo commented Dec 26, 2022

lolodomo commented Dec 26, 2022

lolodomo commented Dec 26, 2022 • edited Loading

lolodomo commented Dec 26, 2022

J-N-K commented Dec 26, 2022

lolodomo commented Dec 26, 2022

goligo commented Dec 26, 2022

goligo commented Dec 27, 2022 • edited Loading

lolodomo commented Jan 7, 2023

lolodomo commented Dec 24, 2022 •

edited

Loading

goligo commented Dec 24, 2022 •

edited

Loading

J-N-K commented Dec 25, 2022 •

edited

Loading

J-N-K commented Dec 25, 2022 •

edited

Loading

J-N-K commented Dec 25, 2022 •

edited

Loading

goligo commented Dec 25, 2022 •

edited

Loading

J-N-K commented Dec 26, 2022 •

edited

Loading

J-N-K commented Dec 26, 2022 •

edited

Loading

lolodomo commented Dec 26, 2022 •

edited

Loading

lolodomo commented Dec 26, 2022 •

edited

Loading

goligo commented Dec 27, 2022 •

edited

Loading