Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

BasicUI does not map option values while PaperUI does #2729

Closed
crnjan opened this issue Jan 3, 2017 · 10 comments
Closed

BasicUI does not map option values while PaperUI does #2729

crnjan opened this issue Jan 3, 2017 · 10 comments

Comments

@crnjan
Copy link

crnjan commented Jan 3, 2017

I have a binding with one of the channels defined as below:

<channel-type id="lastErrorCode">
    <item-type>String</item-type>
    <label>Last error information</label>
    <category>Error</category>
    <state readOnly="true" pattern="%s">
        <options>
            <option value="0">Sensor radiator return (GT1)</option>
            ...
            <option value="20">Three phase incorrect order</option>
            <option value="21">Power failure</option>
            <option value="22">High delta GT8/GT9</option>
        </options>
    </state>
</channel-type>

When channel has a value, i.e. 22, PaperUI displays the option value, as expected - "High delta GT8/GT9", but within BasicUI I see the actual value, that is 22 (same on iOS client).

Code can be found here.

@sjsf
Copy link
Contributor

sjsf commented Jan 4, 2017

This is not necessarily a flaw in the BasicUI, but could be solved in the core framework instead. We have two StateDescriptionProviders: The GenericItemProvider which has the information from the items-DSL (including any custom formatting pattern) and the ChannelStateDescriptionProvider which has the information from the linked channel (including the state options as you outlined above).

The GenericItem just takes the StateDescription from whatever provider is first in the list. Therefore the info from the second one gets lost.

IMHO the fact that we can have multiple StateDescriptionProvider makes sense. But they should be disjunct so we don't have two different ones answering for the same item. The ChannelStateDescriptionProvider somehow has a special role (i.e. it looks up default from the channel definitions). It should NOT be registered as StateDescriptionProvider and should be used to lookup the defaults which another StateDescriptionProvider can override.

Of course, the ItemUIRegistryImpl in the end also has to deal with these options and replace the options' values with labels for rendering purposes.

In that way, not every UI needs to do the lookup for item -> linked channels -> channel types -> options on its own.

@kaikreuzer, @cdjackson: WDYT?

@kaikreuzer
Copy link
Contributor

It should NOT be registered as StateDescriptionProvider and should be used to lookup the defaults which another StateDescriptionProvider can override.

This would introduce a dependency from the items to the things, which we try to avoid.
An alternative would be to add a priority to StateDescriptionProviders and merge their content together. This would still treat them equally, while there is still the possibility to say that one should overrule another.

@sjsf
Copy link
Contributor

sjsf commented Jan 4, 2017

That would be fine by me too.

@kaikreuzer
Copy link
Contributor

@SJKA As far as I am aware, such a priority has been implemented meanwhile, right?

@lolodomo
Copy link
Contributor

lolodomo commented Jul 9, 2017

With a service ranking set to -1 for ChannelStateDescriptionProvider and not set for GenericItemProvider.
What is the impact on this issue and more generally ?

@lolodomo
Copy link
Contributor

lolodomo commented Jul 9, 2017

getStateDescription in GenericItem still returns the first of the list. Is the service ranking taken into account automatically when the list is built ? Which one will be returned first ?

@sjsf
Copy link
Contributor

sjsf commented Jul 10, 2017

Yes, they are list that GenericItem gets is already ordered. They are sorted in descending order. So the one with the highest number as the rank will be the one which is the first in the list and will be winning.

@lolodomo
Copy link
Contributor

Ok, so no problem with the order.

In my opinion, we should update the method getStateDescription from GenericItem or add a new one named getFullStateDescription that will return a state description being a merge of all state descriptions returned by each provider (with first provider being higher priority) : https://github.com/eclipse/smarthome/blob/c4435ea590a5a6f9d35dfdf2063903fcd7ad844d/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/GenericItem.java#L405

Then to fix this issue, we just have to consider state options in the method getLabel from ItemUIRegistryImpl: https://github.com/eclipse/smarthome/blob/067bd5122b4666452bda319cb4e1de13eda9ab00/bundles/ui/org.eclipse.smarthome.ui/src/main/java/org/eclipse/smarthome/ui/internal/items/ItemUIRegistryImpl.java#L269

If we are ok, I will work on a fix.

@lolodomo
Copy link
Contributor

PR submitted for your review.

kaikreuzer pushed a commit that referenced this issue Aug 4, 2017
Fix issue #2729

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@crnjan
Copy link
Author

crnjan commented Aug 7, 2017

I can confirm that this got fixed with #3825. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants