Channels once more displayed in correct order in Main UI

[andrewfg]

Fixes #3442

As described in #3442 "we switched from a List<Channel> to a Map<ChannelUID, Channel> which does not necessarily preserve the order" -- with the consequence that in the Main UI, Thing Channels were no longer displayed in the order in which they were declared in the thing.xml file.

This PR uses LinkedHashMap<ChannelUID, Channel> instead of HashMap<ChannelUID, Channel> so that Thing Channels are once more displayed in the order in which they were declared in the thing.xml file.

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

[J-N-K]

LGTM

[wborn]

A unit test for checking the order would also be nice to prevent such regressions.

[andrewfg]

A unit test for checking the order would also be nice to prevent such regressions.

Ok. Coming up.

[andrewfg]

Oops. I realise that the current version of my test has 4 channels only so it has a 1 in 24 chance of passing by random. So I will add some more channels to reduce that risk.

[wborn]

I think ThingImpls are also stored in the JsonStorage using a OrderingMapSerializer (see code)
So it could still be that the channels have a different order when ThingImpls are deserialized that way.

[J-N-K]

I think ThingImpls are also stored in the JsonStorage using a OrderingMapSerializer (see code)
So it could still be that the channels have a different order when ThingImpls are deserialized that way.

The ThingImpl is not stored, the ThingStorageEntity is stored and that uses a List. The Map is generated when mapping ThingStorageEntity to ThingImpl.

[andrewfg]

The ThingImpl is not stored, the ThingStorageEntity is stored and that uses a List. The Map is generated when mapping ThingStorageEntity to ThingImpl.

Yes that is my impression too. And in my actual testing, I can confirm that (using LinkedHashMap) the ordering does indeed persist over a restart (because LinkedHashMap properly serialises to, and deserialises from, a List..)

[wborn]

Will it also reapply the order according to how they are ordered in the Thing Type after deserialization? Channels could get rearranged in newer OH versions or new channels may get added in between existing channels.

[mhilbush]

Makes me also wonder what happens wrt new channels during a binding update when the update instructions are applied. Will the update instructions result in the new channels being added to the end of the list of channels versus a new installation of the binding which will put the new channels wherever the developer put them in the thing xml?

[andrewfg]

Will the update instructions result in the new channels being added to the end of the list of channels

AFAIK, if you call ‘editThing’ you get the original channels as a List, and recreate the Thing via ‘.withChannels’ you supply the new channels as a List. So maintaining the sequence of entries in the List is the critical point. And if new channels are added via ‘List.add’ they are added at the end of the List, etc.

[andrewfg]

Will it also reapply the order according to how they are ordered in the Thing Type after deserialization?

AFAIK the channel order is defined by thing.xml when the thing is first instantiated. Thereafter the channel order should be serialised and deserialised in the same order on each restart. I am not 100% sure but I think that if a new binding version changes the order in thing.xml, that change is NOT directly applied to already defined things, so you have to delete and recreate the thing to get the new order. And it’s probably different for things defined via ‘.things’ file.

[mhilbush]

AFAIK, if you call ‘editThing’ you get the original channels as a List, and recreate the Thing via ‘.withChannels’ you supply the.new channels as a List

Sorry, I was referring to a new feature in 4.0 that let's you (the developer) provide instructions that indicate, new, changed, or deleted channels, which are applied to the thing without the need to delete/recreate the thing.

For example, see this PR.

In this case, the new channels were added to the end of the channels in thing-types.xml.

However, what if the new channels were inserted into the middle of the list of channels in thing-types.xml? In the case of a newly created thing, the order would be as the channels are defined in thing-types.xml. But in the case of an existing thing that's updated as a consequence of the update instructions, I assume the new channels would be added at the end of the list.

I don't think this is a really big deal. But it is behavior that we want to understand so that we know what to expect.

So for example, let's say I have two things of the same thing type. One thing was newly created and one was an existing thing that was updated according to the update instructions. I'm assuming the order of the channels shown in the UI might be different depending on where the new channels appear in the channel list in thing-types.xml.

[andrewfg]

I was referring to a new feature in 4.0

@mhilbush thanks for putting me on the right track.

I think your concern is not directly impacting the PR. This PR is essentially just about ensuring that whatever channels are created in a thing shall remain consistently in the same order in which they were originally added. i.e. both

• transient consistency via .withChannels() => getChannels(), and
• persistent consistency via .withChannels() => ThingDTOMapper.map(Thing) => ThingDTO => ThingDTOMapper.map(ThingDTO) => Thing => getChannels().

[mhilbush]

I think your concern is not directly impacting the PR

Yes, I agree.

Since your original issue had to do with the order of channels in the UI, I just wanted to raise awareness that there may still be some cases where the order of channels presented in the UI may not exactly match the order of channels in the thing xml.

[andrewfg]

may still be some cases where the order of channels presented in the UI may not exactly match the order of channels in the thing xml

@mhilbush digging into the OH core channel modification instruction code, it looks like there are (currently) two supported instructions, namely add resp. remove. So currently there is nothing that could change the channel sort order. Of course I do admit that the core might add support for an insertAt instruction in future, so the situation that you describe might indeed occur in future. But I suggest to focus on the current rather than the future.

openhab-core/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/update/ThingUpdateInstructionReader.java

Line 84 in 0894d25

if (instruction instanceof XmlAddChannel addChannelType) {

[mhilbush]

Right. So this is the case I was contemplating.

Original thing.xml channels

channel-a
channel-b
channel-c

New thing.xml channels with update instructions to add channel-a1 and channel-a2 to existing things.

channel-a
channel-a1
channel-a2
channel-b
channel-c

A newly created thing would show in the UI as this, correct?

a, a1, a2, b, c

What would an existing thing look like in the UI after the update instructions were applied? (I ask this as a question because I'm not sure what will actually happen)

a, b, c, a1, a2

[andrewfg]

New thing.xml channels with update instructions to add channel-a1 and channel-a2 to existing things.

I think you are wrong. An add instruction will append the new channel to the existing list. It will not insert into the middle of an existing list.

[mhilbush]

An add instruction will append the new channel to the existing list. It will not insert into the middle of an existing list.

I'm sorry for not being clear. Yes, that's exactly what I'm trying to say.

In my example above, the channels of a newly created thing will be in the order in which the channels appear in the thing.xml. In that example, the developer inserted new channels a1 and a2 into the middle of the list (presumably because they had some relationship to channel a).

The channels of a thing updated by the update instructions will not be in the order in which the channels appear in the thing.xml because the new channels will be appended.

So for those two things, won't the order of the channels in the UI be different?

[J-N-K]

Yes, that would be the case. But I don't think there is an easy way around it. While it may be possible to add something like before="channelId" to the add instruction, it'll become very complex for channel-groups. What if the developer just re-orders the channels? It is nice to preserve the order (especially when it is so easy like the change of the collection type) but it's not absolutely necessary.

[J-N-K]

LGTM, thanks.

[mhilbush]

@J-N-K Yep, I agree completely. Like I said earlier, I just wanted to bring some awareness to the behavior for when someone asks why their channels are "out of order." 😉