Fix item channel links not properly initialized

[wborn]

I've tested the ChannelLinkNotifier by @splatch (#1385 (comment)) and it solves the channel linking issues for me.

I made a few small changes:

• add @NonNullByDefault
• tune logging
• addRegistryChangeListener before notifying handlers to prevent events getting lost (if channels can be unlinked during this call synchronization may be necessary too)
• removeRegistryChangeListener on @Deactivate

With the channels being properly linked, adding Things via the inbox and adding them to the model works a lot better. :-)

Fixes #1596

[cweitkamp]

Code Looks great. Thanks to both of you. To avoid future regressions do you think it is possible to add an integration test - in a simple way - for this feature?

[wborn]

I haven't added any tests because I first wanted to know from the other @openhab/core-maintainers if we still think this is the way to go (and it was getting late 😉).

[cweitkamp]

As I said, I like it and I agree on it 😉.

[kaikreuzer]

And I already stated in #1385 (comment) that I like it 😎 .

[wborn]

There may be issues with this implementation when you link multiple items to one thing channel. If you remove one of the links it will send a "channelUnlinked" event to the handler. Which may use the event to stop a channel update mechanism (polling).

I think the handler should only be informed of these changes if:

• the number of linked items to the channel changed from 0 to at least 1 -> channelLinked
• the number of linked items to the channel changed from at least 1 to 0 -> channelUnlinked

It looks like this may already have been an issue with the old implementation.

Similarly it will call "channelLinked" multiple times when the ChannelLinkNotifier is activated and there are several item channel links for the same thing channel.

Another improvement may be to use a scheduler for these events to reduce the impact of thing handlers hijacking threads for long running tasks. It was also used in the old implementation.

[splatch]

Another improvement may be to use a scheduler for these events to reduce the impact of thing handlers hijacking threads for long running tasks. It was also used in the old implementation.

Looking at issue and how it looks a like I believe that we should rather focus on keeping link identifier as part of notification. If we have channel linked which is related to link which have been just created or removed the divider is link and not channel.

There might be cases when binding wants to keep distinct subscriptions for each and every link due to let say latency or better concurrency. Letting binding developer decide how to handle above scenario is better than building an extra logic in framework notifier itself.

Best,
Łukasz

[wborn]

I've now added some pretty decent integration test coverage as well.

@splatch adding such details would be breaking API changes and I was under the impression the details of links and items were hidden from handlers on purpose. But perhaps the other @openhab/core-maintainers can also comment on that.

[splatch]

adding such details would be breaking API changes and I was under the impression the details of links and items were hidden from handlers on purpose.

@wborn Given that this is one of less popular framework features coming next major release is great opportunity to introduce breaking change. If a whole intention is to keep handler unaware of items and links then idea of having link notifications in handler was wrong in first place.
Given that Link bridges Item together with Channel it seems to play key role in keeping both separated, yet it can't and shouldn't stay hidden or belong just to an item.

[kaikreuzer]

I think the handler should only be informed of these changes if:
the number of linked items to the channel changed from 0 to at least 1 -> channelLinked
the number of linked items to the channel changed from at least 1 to 0 -> channelUnlinked

Yes, I fully agree.

the details of links and items were hidden from handlers on purpose

Correct. The purpose of these methods was only for the handler to know, whether a certain channel is at all in use as it might otherwise skip expensive internal operations or unnecessary communication.

[wborn]

After some more thoughts I think it might still be necessary to send "channelLinked" events for already linked channels. That may be the only way for newly linked items to receive the current channel state.

[robnielsen]

The Insteon binding relies on channelLinked and channelUnlinked methods to build the bridge between OH and the Insteon network. It was reported in the OH community that the Insteon binding isn't working with 3.0.0-shapshot . I was able to reproduce the same behavior with things defined in .things files and items defined in .items files, not the UI.

[kaikreuzer]

lgtm, many thanks!