openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
914 stars 422 forks source link

Rollershutter slider has incorrect value #4358

Open mueller-ma opened 1 month ago

mueller-ma commented 1 month ago

Actual behaviour

I have a group Item with the following config: grafik

And a Sitemap widget configured as Switch item=MyGroup

When the average is let's say 50, I expect the slider in the bottom sheet to be at 50, but it's either at 0 (when the actual value is 0) or 100 otherwise.

Here's what gets passed to Widget.updateFromEvent(): rollershutter The state for the widget is ON, which translates to the numeric value 100. But the item state is 3.00000, which is correct.

Is there any metadata I need to set? Or should the client make a special handling for Rollershutter group switches?

Expected behaviour

The slider has the correct value.

maniac103 commented 1 month ago

I'd say that the actual issue is not within the client, but the widget state being ON. Why that is I'm not sure though...

mueller-ma commented 1 month ago

Because it's a switch, I guess.

maniac103 commented 1 month ago

Good point. Any reason for not making this a Rollershutter widget?

mueller-ma commented 1 month ago

There's no explicit Rollershutter widget: https://www.openhab.org/docs/ui/sitemaps.html#element-types

The client handles Switch with Rollershutter items different: https://github.com/openhab/openhab-android/blob/main/mobile/src/main/java/org/openhab/habdroid/ui/WidgetAdapter.kt#L376

maniac103 commented 1 month ago

Oh, I completely forgot about that :-/ Anyways, the reason why widget state is preferred over item state is outlined in the commit that introduced it: https://github.com/openhab/openhab-android/commit/68cff1fcc2293a311524c6bd69b9a3ea3149618a ... AFAICT, that rationale also applies to roller shutters, doesn't it?

mueller-ma commented 1 month ago

In this special case I'd prefer the Item state over the Widget state. As the Widget state is only ON/OFF, there cannot be any UoM or number formatting applied.

maniac103 commented 1 month ago

I understand and agree for this specific special case, but the question is: how do you intend to separate this special case from all others properly?

maniac103 commented 1 month ago

Thinking about it more, I think this needs to be fixed in core, since otherwise there'll be inconsistencies between the various clients (and special case code tends to become hard to maintain). It's probably as easy as changing the check here to also cover groups of type Rollershutter. Something like this should work, I think (untested):

diff --git a/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java b/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.jav+a
index f5eccaf2b..cb4764d8c 100644
--- a/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java
+++ b/bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java
@@ -733,19 +733,22 @@ public class ItemUIRegistryImpl implements ItemUIRegistry {
             itemState = convertStateToWidgetUnit(quantityTypeState, w);
         }

-        if (w instanceof Switch && i instanceof RollershutterItem) {
-            // RollerShutter are represented as Switch in a Sitemap but need a PercentType state
-            returnState = itemState.as(PercentType.class);
-        } else if (w instanceof Slider) {
+        if (w instanceof Slider) {
             if (i.getAcceptedDataTypes().contains(PercentType.class)) {
                 returnState = itemState.as(PercentType.class);
             } else if (!(itemState instanceof QuantityType<?>)) {
                 returnState = itemState.as(DecimalType.class);
             }
         } else if (w instanceof Switch sw) {
-            StateDescription stateDescr = i.getStateDescription();
-            if (sw.getMappings().isEmpty() && (stateDescr == null || stateDescr.getOptions().isEmpty())) {
-                returnState = itemState.as(OnOffType.class);
+            if (i instanceof RollershutterItem
+                    || (i instanceof GroupItem gi && gi.getBaseItem() instanceof RollershutterItem)) {
+                // RollerShutter are represented as Switch in a Sitemap but need a PercentType state
+                returnState = itemState.as(PercentType.class);
+            } else {
+                StateDescription stateDescr = i.getStateDescription();
+                if (sw.getMappings().isEmpty() && (stateDescr == null || stateDescr.getOptions().isEmpty())) {
+                    returnState = itemState.as(OnOffType.class);
+                }
             }
         }
mueller-ma commented 1 month ago

I also think it should be fixed at core. @openhab/core-maintainers can you move this issue to the core repo?