the returned StateDescription combines values from GenericItemProvide…

[lpapazow]

Fix for issue 2267.

This fix relies on the fact that ChannelStateDescriptionProvider has service ranking -1 and therefore when iterating through the stateDescriptionProviders in the getStateDescription method, ChannelStateDescriptionProvider will come after GenericItemProvider.

If an item that is provided from the .items file and has a custom pattern, then the GenericItemProvider will provide a non null StateDescription and on the next iteration, the result will take the isReadOnly value from the ChannelStateDescriptionProvider.

[kaikreuzer]

If we want to go further into merging several state descriptions into a single result, I think we should cover all its properties (i.e. also min/max, etc.)

I am not yet completely sure if this is what we want in the end or if we are overlooking some issues that can come up by such a merged description.

@lolodomo @SJKA @maggu2810 Any opinions here?

[kaikreuzer]

You are here in any case overwriting a previous readOnly information, I don't think this is a valid approach.

[htreu]

We had a similar discussion once in #2729. The outcome was that only the state description options where taken from the ChannelStateDescriptionProvider in case the GenericItemProvider had its own state description to offer.

One solution would be to take all properties from the ChannelStateDescriptionProvider the "other" StateDescriptionProviders can not offer (e.g. readOnly, options, min,max,step). But that would be a convention (which I dont like btw.).

[kaikreuzer]

which I dont like btw.

Could you elaborate on why you do not like it? Taking the info from the first (ordered by prio) provider that serves a value (and not null), sounds to me like a valid approach (but as stated above, I have the feeling that I am missing something, so your answer to this question might help on this...)

[htreu]

My reasons for dislike for a conventional "merging" of two or more StateDescriptionProviders:

• Its conventional and thus must be explained to everyone. Its not an easy algorithm but has exceptions which treat some properties different then others.
• It does not scale. While the "fix" might help us right now with 2 StateDescriptionProviders it will become more cumbersome when adding more of them: with the exceptions made (e.g. the readOnly property) we have a hard time telling for the n-th StateDescriptionProvider if readOnly=false is its default or set by intention. Although we might get around this by using Boolean instead of boolean here (but API breaking...).

[ppieczul]

What if getStateDescription was modified to also pass the current (from the previous provider iterated) state description, so the queried provider can modify just he parameters it cares about and keep the others intact? This would be kind of distributed merging, would not break the API used by the bindings and need modifications of the providers only.

[htreu]

@ppieczul what I like about the 'distributed merging' idea is the fact that we don´t have to come up with one algorithm now which will then be hard to modify in the future but leave the decision to every single implementation. It will although also be API breaking since StateDescriptionProviders would have to incorporate the new behaviour.

[htreu]

Maybe we can gather more brain power, ideas and comments:
@lolodomo you did the last implementation of merging the state options in #3825 , wdyt?

From a framework perspective we could also factor out some StateDescriptionMergingService and do a default implementation which does a most "simple" in place merging (see #4428 (comment)) but leave flexibility for solutions to define their own behaviour.

@kaikreuzer wdyt? /cc @SJKA & @maggu2810

[lolodomo]

On my side, I was in favor of a merge because it is required to fix a current bug we have to get the right label. This was my reason to initially implement the merge solution.

[maggu2810]

If the getStateDescription method get the current one return a new / modified one, we need to call this method for every state description provider starting from the lowest service ranking to the highest service ranking. Correct?

We could also keep the method as it is and create a StateDescriptionMergingService that calls every provider for a state description starting from the highest service ranking to the lowest as long as there is a value that is null for all previously called providers. All values from the highest ones are used, if there is a null value, the next (priority sorted) provider is called and that value is used (if non-null, otherwise call the next)...
If we use a migration service that handled the merging logic a solution could provide its own implementation with a higher service ranking and another merging strategy could be used.

@htreu Does this fit to your suggestion? Keeping the getStateDescription as it is and implement a service that merges the description using a custom (default the above one) merge strategy?

[htreu]

@maggu2810 yes that was also my intention. The current getStateDescription does only merge state options but the default implementation of a StateDescriptionMergingService should merge all properties and replace the implementation from getStateDescription.

[lpapazow]

The idea about a StateDescriptionMergingService seems great. I will try to implement one and create another PR within the next week.

[htreu]

@lpapazow I guess this is superseded by #4882 now? Can we close this one then?

[lpapazow]

Yes, I forgot to mention it in #4882 but this one should be closed.