Added 'ChannelDescriptionChangedEvent'

[cweitkamp]

• Added ChannelDescriptionChangedEvent

Problem statement:

OHC framework supports dynamic StateDescriptions and dynamic CommandDescriptions which are set by bindings and may change during runtime. When the state options / command options are changed the UI doesn't update automatically. You need to do a page refresh to pick up the changes. This approach introduces a new event (ChannelDescriptionChangedEvent) which UIs can subscribe to and reload related widgets. The event payload contains the ChannelUID, a field identifier to give a hint which field has changed and a list of linked item names.

See e.g. openhab/openhab-addons#7396 (comment)

Closes #1753

Signed-off-by: Christoph Weitkamp [email protected]

[kaikreuzer]

I so far always considered those descriptions rather static and the "dynamic" providers mainly being a means to have the list defined at runtime and not at compile time (as with the xml files) - thus I never expected UIs to react upon changes.

If you think this is worthwhile to have, it should be fine to add it. You should just be clear of the implications: You should convince all UI devs to react to those events sooner or later and this includes Basic UI, the new Default UI as well as Android, iOS and Windows apps...

[cweitkamp]

Most of the implementations in bindings I am aware of use it to transfer "really" dynamic, user-defined data (e.g. favorites, playlists, scenes or template s) including a frequently polling for it. So from my POV it could be a useful feature - even if a binding developer has to dispatch these events on his own if he does not use the abstract dynamic providers.

I considered this to be a draft so everyone is invited to leave feedback on it. I am happy to ask @openhab/webui-maintainers, @openhab/android-maintainers, @openhab/ios-maintainers, @openhab/windows-maintainers and everyone else for their opinion.

[cdjackson]

I think this is a useful feature - even if UIs don't implement it, we are no worse off than now. If UIs were to implement this event (which should be be recommended of course), then it's definitely an improvement.

[ghys]

I agree with @cdjackson that it's good to have the info no matter what's done with it.

A few notes however:

• Sitemap rendering UIs (i.e. mobile apps, Windows, Basic UI, etc.) listen to events on a dedicated SSE connection (/rest/sitemaps/{sitemapname}/{pageid}), not the "regular" event bus (/rest/events), Would this event appear in those?
• End user UIs usually deal with items, and don't care about channels, so even if the event contains the linked items for the channel, maybe it would be more natural to issue events for the affected item's themselves rather than the channel;
• The new "main" UI tries not to listen to the event bus or maintain a local copy of item descriptions for end-user pages (for performance reasons), but only tracks select items' states with /rest/events/states. Command options are currently displayed with "action sheets" like this by default:

In this case, since the options are not displayed until the user clicks on the widget, it would be safer just to make a quick call to /rest/items/{itemName} to get the up-to-date command options before displaying the actions. I suppose there will be a need for alternative rendering of these command options (e.g. with buttons when their number is limited), then of course it's another story. Nonetheless, maybe it would be worth looking into adding state descriptions/command descriptions and their updates to the StateDTO the UI receives whenever an item tracked with /rest/events/states changes.

[cweitkamp]

Thanks for you useful input. The pointer to SSE events was a good hint. It obviously makes a lot of sense to emit the ChannelDescriptionChangedEvent via them too. I will look into that and try to enhance my current implementation to add support for it.

[mueller-ma]

Sitemap rendering UIs (i.e. mobile apps, Windows, Basic UI, etc.) listen to events on a dedicated SSE connection (/rest/sitemaps/{sitemapname}/{pageid}), not the "regular" event bus (/rest/events), Would this event appear in those?

Ideally a SSE will be sent to all affected sitemap elements as well.

/cc @maniac103

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/dlna-upnp-binding/1790/186

[lolodomo]

Thanks for you useful input. The pointer to SSE events was a good hint. It obviously makes a lot of sense to emit the ChannelDescriptionChangedEvent via them too. I will look into that and try to enhance my current implementation to add support for it.

For the remote openHAB binding, an event on the topic openhab/items/*/xxx would be ideal for me. But if I have to listen to another topic, why not.
I have imagined it could have be done through the existing event ItemUpdatedEvent.

Do you plan to rework and finish this enhancement ? Before OH3 is released ?

[cweitkamp]

Do you plan to rework and finish this enhancement ? Before OH3 is released ?

I added it to the openHAB 3 issue tracking. So yes, I am planning to do further work on it.

[lolodomo]

Any progress on this enhancement?

[cweitkamp]

I rebased the branch onto latest main. From my POV the PR is in a final state and ready for review.

I have to admit that I was not able to look deeper into SSE connections. Feel free to give me a hand on that.

[lolodomo]

Does it mean that any binding state options provider should then be updated to set the references to ItemChannelLinkRegistry and EventPublisher?

[cweitkamp]

Does it mean that any binding state options provider should then be updated to set the references to ItemChannelLinkRegistry and EventPublisher?

AFAIR yes. At least when we want to use constructor injection. I did not test if field injection is working when using abstract base classes. Method injection did not work for it in the past.

There are other services in the OH framework which have to be implemented in a similar way e.g. AbstractDiscoveryService. If you want to use the translation feature the extending class has to pass the I18nProvider reference.

[lolodomo]

Isn't it possible to provide the new state description in the new event? This would avoid additional request(s) to get it.

Rather a "field" to tell what kind of information was changed, I would prefer the new state description in the event.

[cweitkamp]

Sounds like a reasonable idea.

[kaikreuzer]

lgtm, thanks!

[lolodomo]

Very good.
Can you please trigger a new core build so that we can have this enhancement in the next snapshot ? I need it ot test my changes in the remoteopenhab binding.

Regarding impact on UI (sitemap), I know how to handle that. First a little change is needed in the IO REST sitemap bundle: my idea is to add a boolean field to the widget SSE event to indicate if the item description was changed. This will have no impact on UIs not reading this field. In Basic UI, I will read this field and when set to true, I will trigger the reload of the current page. Nothing very difficult I believe, I just need to validate that it works as I imagine.

[kaikreuzer]

@lolodomo I had triggered a build right after merging this PR: https://ci.openhab.org/job/openHAB-Core/793/

[lolodomo]

Perfect.

Note that we have already 3 PRs for addons ready to be merged that enhance bindings with this new feature.
@cweitkamp is working on the PR for the hue binding.
I will try to create another PR covering the remaining bindings this evening.

[lolodomo]

@cweitkamp : what is smarthome/j ?

[cweitkamp]

A third party repository for OHC based add-ons.

[lolodomo]

I identified 3 bindings implementing a state description provider extending BaseDynamicStateDescriptionProvider in an unusual way: spotify, bticinosmarther and heos. These bindings require a special look. All the other (except hue) are now updated.

And for the 3 bindings implementing a command description provider extending BaseDynamicCommandDescriptionProvider, only the nanoleaf binding is not yet enhanced.

As a summary, we have still 5 bindings to check/enhance.

[lolodomo]

@cweitkamp : I just discovered we have many bindings directly implementing DynamicStateDescriptionProvider. Those ones will not benefit this enhancement?

[cweitkamp]

Yes, that is correct. They have to take care on their own.