BasicUI: Treat Switch on NumberItem not as ON/OFF Switch

[triller-telekom]

Fixes #3472
Signed-off-by: Stefan Triller stefan.triller@telekom.de

[lolodomo]

I think it is not the appropriate fix.
For a.normal switch in the sitemap, the conversion is not a bad idea.
What we have to avoid is I think any conversion when the switch has mappings, whatever the kind of item.

[lolodomo]

A switch with mappings is not really a switch but more like a selection.

[triller-telekom]

If I create a Switch in the items file and in the Sitemap a Switch with mappings for that item.

items:

Switch Ventilation_Fan_Level "Fan [%d]" {comfoair="fan_level"}

sitemap:

Switch item=Ventilation_Fan_Level label="Fan" mappings=[1="A", 2="1", 3="2", 4="3"]

and then I want to update this item with a value in the mapping I get:

smarthome update Ventilation_Fan_Level 3
Error: State '3' is not valid for item 'Ventilation_Fan_Level'
Valid data types are: ( OnOffType UnDefType )

Which is correct because the item is a Switch that does not understand the values in the sitemap mappings, i.e. they are wrong.

If I change the sitemap to react on the correct values of the item, which is Switch like this:

Switch item=Ventilation_Fan_Level label="Fan" mappings=[ON="An", OFF="Aus"]

then it works as expected.

Where exactly do you think it doesn't work?

[lolodomo]

Your example is of course wrong becase you cannot use a number value as state for a switch item.

The switch element in sitemap is in fact very similar to the selection element; the difference is mainly how it is rendered by UI. The user can switch between "switch" and "selection" easily.

The switch element with mappings in the sitemap can be attached more or less to any kind of items, a switch item, a number item, a contact item, ... etc.
When you have a switch with mappings, you should not convert the item state to OnOffType and so you retrieve as key for your mappings, a number or why not OPEN/CLOSE for a contact item.

[triller-telekom]

Thank you for the long explanation, I agree that checking for mappings is the right way to go because the conversion should only be for plain on/off type switches and for all others just return whatever state they have.

[lolodomo]

LGTM

Before merging, someone should check if there is a valid reason for failing travis checks.

[sjsf]

Don't know how valid the reason is, but definitely can't stay like that:

testStateConversionForSwitchWidgetThroughGetState(org.eclipse.smarthome.ui.internal.items.ItemUIRegistryImplTest)  Time elapsed: 0.021 sec  <<< ERROR!
java.lang.NullPointerException: null
	at org.eclipse.smarthome.ui.internal.items.ItemUIRegistryImpl.convertState(ItemUIRegistryImpl.java:509)
	at org.eclipse.smarthome.ui.internal.items.ItemUIRegistryImpl.getState(ItemUIRegistryImpl.java:485)
	at org.eclipse.smarthome.ui.internal.items.ItemUIRegistryImplTest.testStateConversionForSwitchWidgetThroughGetState(ItemUIRegistryImplTest.java:332)

[sjsf]

.

[triller-telekom]

The test is using a mock for SwitchImpl and thus could not call getMappings(). So it was a bug in the test. Lets wait for travis and then it can be merged.

[sjsf]

This time you even had to touch a test, but still did not think of implementing a regression test for your assumption "only convert plain switches with no mappings".

With all the regressions that you are facing right know after attempting to bug in the UI layer you should know best how valuable it would have been if there were any such tests. I really don't get it... 😢

[sjsf]

Such a comment is completely useless: It is dead and only states the obvious!

Next time you feel the urge to write such a comment: Pleeeeease write a regression test case instead!!!

[sjsf]

Thank you!