Sitemap Events provide item states in a format that matches map entries #3332
Conversation
…ntries Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
There was a problem hiding this comment.
Some inline commens.
And: please also put a regression test for this behavior into ItemUIRegistryImplTest
| @@ -32,6 +32,7 @@ Import-Package: javax.imageio, | |||
| org.eclipse.smarthome.model.core, | |||
| org.eclipse.smarthome.model.items, | |||
| org.eclipse.smarthome.model.sitemap, | |||
| org.eclipse.smarthome.model.sitemap.impl, | |||
There was a problem hiding this comment.
Indeed, somehow slipped from testing into my commit...
| @@ -72,6 +76,7 @@ | |||
| * | |||
| * @author Kai Kreuzer - Initial contribution and API | |||
| * @author Chris Jackson | |||
| * @author Stefan Triller - convertState(Widget, State) | |||
There was a problem hiding this comment.
please state here what you did
| private State convertState(Widget w, State s) { | ||
| State returnState = null; | ||
|
|
||
| if (w instanceof SwitchImpl) { |
There was a problem hiding this comment.
please check for the interfaces here, not the implementations.
| if (s instanceof PercentType) { // catches also HSBType etc | ||
| returnState = ((PercentType) s).as(PercentType.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
Are these the only widget types that potentially need conversion? How about e.g. the Colorpicker?
There was a problem hiding this comment.
You're right, Colorpicker is expecting a HSBType, so that might be a good idea to try a conversion.
There was a problem hiding this comment.
For colopicker, a OFF or 0 could be converted to (0,0,0) but how will be converted a ON or a value > 0 ?
There was a problem hiding this comment.
A Colorpicker only makes sense for a Color item and this does have a HSBType state anyway. Why would anyone want to attach a ColorPicker to a Switch or Dimmer item for example? What would be the use case? I don't see any, which is why I added no conversion for the ColorPicker.
There was a problem hiding this comment.
You're right.
The real usage is more the reverse, that is using a switch or a dimmer for a color item for example and it is what ypu have covered.
| State returnState = null; | ||
|
|
||
| if (w instanceof SwitchImpl) { | ||
| if (s instanceof PercentType) { |
There was a problem hiding this comment.
you don't need to check for a specific type here - just use s instanceof Convertible so we keep the knowledge of what can be converted to what in a single place.
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
| if (w instanceof SwitchImpl) { | ||
| if (s instanceof PercentType) { | ||
| if (w instanceof Switch) { | ||
| if (s instanceof Convertible) { |
There was a problem hiding this comment.
Why not just: returnState = s.getStateAs(OnOffType.class);
This call is doing the test with Covertible.
|
I have not tested this PR but it makes really sense and will help all UIs. |
| @@ -476,6 +480,34 @@ public State getState(Widget w) { | |||
| } | |||
|
|
|||
| /** | |||
| * Converts an item state to the type the widget supports (if possible) | |||
| * | |||
| * @param w - Widget in sitemap that shows the state | |||
There was a problem hiding this comment.
Can you remove the "-" between the parameter name and the comment?
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
|
LGTM |
43816f5 to
4164053
Compare
|
To mee to. Thanks! |
BasicUI converted an item state to a switch On/Off type, but ClassicUI did not.
ItemUiRegistry now takes care of such a state conversions for Widgets of type Switch and Slider.
Fixes #866
Signed-off-by: Stefan Triller stefan.triller@telekom.de