UI: display mapped option value

[lolodomo]

Fix issue #2729

Signed-off-by: Laurent Garnier lg.hc@free.fr

[htreu]

If I understand the code correctly this would for all DSL based items inherit the full stateDescription from the linked channel-type and then render a state even if the user set the label of the item to be w/o a pattern. According to the discussion in #3741 the idea was to somehow migrate the items files and add a dedicated pattern attribute to the items definition to overcome this. Will this also be part of the PR?

[lolodomo]

Updating the DSL to split label and pattern is not the object of this PR.
The main purpose here is to retrieve the channel options and display the mapped option value.

[lolodomo]

PR ready, I added two tests for options.

[htreu]

The JavaDoc tells the default locale is used. You pass in null here instead. Please also review the getStateDescription method which behaves analogous. Alternatively the JavaDoc in Item should be changed.

[lolodomo]

I have done the same thing as for getStateDescription. It looks like it exists a mechanism to define pattern in channel for different languages ? We don't care about the locale here, I have no idea if it is a bug or not. I would suggest to open a new issue if there is a problem with that.

[htreu]

The main purpose here is to retrieve the channel options and display the mapped option value.

...which should work seamlessly with this PR. Nonetheless does it also change the current behaviour on item rendering by inheriting the state pattern from the channel-type. This will change a lot of sitemap entries.

[lolodomo]

getStateDescription was already retrieving the pattern from the channel if not defined in the item. Nothing is changed here by using getMixedStateDescription. If no provider returns a state description with pattern, no value is displayed.

What is changed by the other PR is that now ChannelStateDescriptionProvider will now build and provide a pattern for String and Number channels even if not defined in the channel definition. So in this case, the value will be displayed even if the user just use "label" as label. It is exactly what we try to fix (in the other PR). The user still has the ability to use "label []" as item or sitemap label to disable the display of a string or number channel.
All your concerns are in fact relative to the other PR but not this one.

[kaikreuzer]

relative to the other PR but not this one.

So what does this one exactly do/fix? I am also very surprised by changed to core classes such as GenericItem, which we should really avoid whenever possible. And a new method like getMixedStateDescription really does not sound obvious to me - seeing it, I have no clue what to expect from it... It rather sounds like some hack that covers some underlying architectural issue.

[sjsf]

To my understanding it builds a state description by mixing the properties contributed by several/all providers. For each property, the non-null value from the provider with the highest ranking will be used.

As I already pointed out in #3822 (comment), I'm pretty sure this will lead to funny surprises and therefore IMHO is not a good idea to do.

[lolodomo]

Hey guys, I try to address the problem that was discussed in the original issue. The current method does not return the channel options in case you have a state description that is provided by the item provider that has a higher priority (for example with a pattern in the item label).
Let me know how I can retrieve the channel options ?
I am sure there will be no surprise with my current implementation.
But if you have another idea, let me know.

[lolodomo]

In case it is still not clear, this PR fixes issue #2729 , display of mapped option value.

[lolodomo]

That being said, using getMixedStateDescription rather than getStateDescription will fix a bug with pattern too in a very particular case. The use case is you have a state description returned by the item provider that contains for example a min and a max value but without any pattern; a pattern is defined for the channel and a state description with this pattern is returned by the channel state description provider. In this case, no value will be displayed because getStateDescription stops at the first state description (without pattern) while getMixedStateDescription will provide a state description containing the min and max values + the pattern returned by the channel state description provider.

[htreu]

@lolodomo I can follow your arguments and your code does indeed fix the original problem. My concerns are in the direction of @kaikreuzer and @SJKA: by introducing a new getMixed... method we indicate that the original getStateDescription method might not be the right one to call. And even if the mixing can be described via JavaDoc it is not clear what really happens under the hood w/o reading the code and knowing the exact ranking configuration of the runtime. On top of this there might be additional StateDescriptionProvider implementations and the mixing will become more and more unintuitive.
As for this issue I would suggest a little less intrusive fix: retrieve the stateDescription just like it is done now but then loop through all the stateDescriptionProviders and add the first options list you can find if none was set already. Like this htreu@b56e0b0

wdyt?

[lolodomo]

What you are doing is exactly what I was doing except that you do the mix only for the options.
Doing it inside getStateDescription rather than creating a new method means that you are impacting other calls to getStateDescription. That's probably ok but I am not 100% sure. This method is used in the REST API if I correctly remember.
Of course, with your code, the fix for mapped option value will work too.
But as I explained in my previous message, we need the mix on the pattern too (not for the issue relative to options but for the display of value in general).

[htreu]

Yes, my proposal is to only fix the options issue with the same method as you did. But leave the rest as is which gives us the opportunity to come up with a good concept on how to provide consistent state descriptions (if this is still an issue then).

[lolodomo]

Before I change my code, is htreu's proposal ok for everybody ?
I will add a TODO in the code of getLabel to explain the use case that is not working for getting the pattern. Hopefully, it is not a common use case and the user has a workaround.

[lolodomo]

@SJKA @kaikreuzer : is it ok for you if I change getStateDescription as @htreu's proposed, to get a mix only for options in StateDescription (leaving a little bug in pattern handling) ?

[lolodomo]

Can I have a feedback so that I can finish this bug fix ?

[kaikreuzer]

Sorry @lolodomo, this got lost during my vacation. Yes, the suggestion from @htreu is ok for me!

[lolodomo]

Ok, done.

[kaikreuzer]

Thank you!