Kodi: bug in the dynamic state description provider

[lolodomo]

The implementation of the Kodi dynamic state desciption provider has a major issue.
https://github.com/openhab/openhab2-addons/blob/master/addons/binding/org.openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/KodiDynamicStateDescriptionProvider.java#L43
When the service is called for a channel of another binding, it returns the original state description but resetting the options.
It should return null in case the channel is not handled by the Kodi binding.

This has been discovered when trying to implement this feature: eclipse-archived/smarthome#4942

I will provide a fix.

[martinvw]

@cweitkamp fyi

[lolodomo]

Note that such provider implemented by a binding could directly impact the channels of other bindings.
It was what was happening due to this bug, the state options were reset for all the channels in the system except the Kodi channel.

Note that it is a MAJOR bug but only for users using this binding.

[martinvw]

IMHO we should also look into fixing or very explicitly documenting this behavior because null seems to mean something very specific in this case. Normally i see such patterns combined with something like an isSupported method.

[mhilbush]

I expect the squeezebox binding also may have the same issue.

[lolodomo]

I searched for DynamicStateDescriptionProvider and only found it in the Kodi binding. But maybe the squeezebox binding was not loaded in my IDE.

[lolodomo]

squeezebox binding is ok but I found the same issue in the Hyperion binding. The loxone binding is certainly to be investigated because very strange. Amazonecho is unusual too.

[lolodomo]

Amazonecho is ok as the channel type is checked. It returns original rather than null but it is ok too.

[lolodomo]

loxone binding is finally ok.
I created a PR for the hyperion binding.
All other bindings look good.

[cweitkamp]

Thanks for taking care of this. I can confirm that it works now.

I removed those lines in #3505 because my IDE complains about an unnecessary null check like it does now again. We have to change the annotations for this class as well.

[WARNING] ~/openhab2-addons/addons/binding/org.openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/KodiDynamicStateDescriptionProvider.java:[47] 
	if (options == null) {
	    ^^^^^^^
Null comparison always yields false: The variable options cannot be null at this location
1 problem (1 warning)

Suggestion - Let us change
https://github.com/openhab/openhab2-addons/blob/6f2212251087413fddb61608256c75b1af3a01cd/addons/binding/org.openhab.binding.kodi/src/main/java/org/openhab/binding/kodi/internal/KodiDynamicStateDescriptionProvider.java#L36
to

private final Map<ChannelUID, @Nullable List<StateOption>> channelOptionsMap = new ConcurrentHashMap<>(); 

[lolodomo]

Can you please apply the change in all concerned bindings ?

[cweitkamp]

@lolodomo @martinvw Done. PR #3623.

[tavalin]

I based the Hyperion implementation of the dynamic state description service on the Kodi binding as an example so it's likely affected too.

[lolodomo]

@tavalin : yes, it is already fixed.