-
-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic state description broken in snapshot 1693 #1040
Comments
Why did you think it is related to #1010? |
Because the choice of default widget depends on whether the state description is editable or not. It looks like now the state description seems to answer "not editable" while it should return "editable". |
Additionally, I don't know if the editable property is important for the build of dynamic state options but apparently they are no more built correctly. |
No, not really. The fix in #1010 addresses a |
OK. Were there other recent PRs impacting StateDescription ? |
#908 touched parts of the Did you test it with a specific binding? |
I saw it with Rotel and Sony Projector bindings. Forgot to check the Sonos binding as it seems to be a general bug. |
Hm, interesting. I set-up a test on OH2 snapshot 1696 using avmfritz binding. It is working for me in Paper UI as well as in Basic UI. My sitemap contains a "Default" and a "Selection" element. |
I just installed the snapshot 1698 and the problem is still there. The Sonos binding is impacted too, I can confirm it now. |
How can we proceed ? Thîs bug is relatively critical. Revert the PR ? |
@cweitkamp : can you please help fixing this major issue breaking the use of several bindings ? |
@lolodomo I would really love to help you. But as stated in #1040 (comment) I do not have a problem with avmfritz and Kodi bindings in latest snapshot 1701 in Paper UI / Basic UI - tested it again a few minutes ago. Let us specify:
|
All UIs are impacted, at least Basic UI, Classic UI, Android App and Paper UI. |
I could add logs in core UI to show you that the list of options is empty (or null ?) when requested (while it should not) ? |
I can try to add logs in BaseDynamicStateDescriptionProvider... |
Maybe we face a similar issue like https://github.com/openhab/openhab2-addons/issues/3619 (fixed by https://github.com/openhab/openhab2-addons/pull/3620)? We could add logging to the following method to get some information about which Lines 131 to 140 in f86d011
I already talked to @Hilbrand about that. His detailed analysis showed that this logic "seems very inefficient and also error prone". |
Here are some added logs:
when restarting the Rotel binding. We can see that options are set for channels. Then later, when I go through Basic UI:
getStateDescription from BaseDynamicStateDescriptionProvider is called several times for the same channel. Sometimes, it returns options; sometimes it doesn't ! |
Yes, it is a step forward. Can you add |
It looks like this line sometimes returns null, sometimes the list of options: |
Here are:
Ok this is the Netatmo binding dynamic options state provider that provides null. |
In fact, any other bidnings (Sonos, Netatmo, Sony projector) is called and of course returns null as options for this channel; only the rotel provider provides options.
Maybe core UI is not getting the correcr provider ? |
Even if I stop the other bindings, still not working. |
One more guess - Can you change the logging in |
readOnly is false as expected. |
No problem with setting options:
|
I missed one of your message. I will add logs to getDynamicStateDescription. |
The method getDynamicStateDescription seems to return options but we can see that the parameter "original" has no options. Is it expected ? |
The problem seems to be in the core UI where I got 0 state options for the item. I am adding logs in this place now. |
The state description got no options !
You can find the code of getNbOptions with the additional logs here: |
@cweitkamp : this would lead to there: |
I think I found the bug. The issue is here: |
Bingo, it is working with this change. |
Nice catch. Some seconds faster than me: 9e16bc1 |
And of course thank you very much for your research. Very much appreciated. |
* Fix dynamic options in UI * Added unit tests Fixes #1040 Also-by: Christoph Weitkamp <github@christophweitkamp.de> Signed-off-by: Laurent Garnier <lg.hc@free.fr>
I can confirm everything is ok again in snapshot 1703. |
@cweitkamp @lolodomo : I want to implement the dynamic mappings feature for the iOS app. I have already a PR. Can you let me know which binding is using the dynamic mappings? Is avmfritz making use of dynamic mappings? If so, which feature is using it? |
Have a look here to find out which bindings make use of this feature.
Yes, it does. The FRITZ!Box ( |
Thank you @cweitkamp |
Yes, there are different types of mapping. We distinguish between State and Command Descriptions. UIs should will render a Selection for Default elements. Selection and Switch elements are both valid. See #952 for more information - fair warning tl;dr). I guess @lolodomo can tell you a lot more about this. |
@cweitkamp: I have seen even a third variant: Spotify Binding uses a Frame element. I |
… agreed one of for basic ui and android : openhab/openhab-core#952, openhab/openhab-core#1040 Signed-off-by: Tim Müller-Seydlitz <timbms@gmail.com>
… agreed one of for basic ui and android : openhab/openhab-core#952, openhab/openhab-core#1040 (#483) Signed-off-by: Tim Müller-Seydlitz <timbms@gmail.com>
Can you provide me a link, a screenshot or an example? |
* Fix dynamic options in UI * Added unit tests Fixes openhab#1040 Also-by: Christoph Weitkamp <github@christophweitkamp.de> Signed-off-by: Laurent Garnier <lg.hc@free.fr> GitOrigin-RevId: 7b49f27
Dynamic state description is no more working, empty list are rendered by UI.
Default widget which should be a selection is no more working too, text widget is used.
This is probably a consequence of this recently merged PR:
#1010
The problem was introduced aftter snapshot 1678.
Coming back to snapshot 1678 until it is fixed.
The text was updated successfully, but these errors were encountered: