UI: provide a default pattern for string and number channels if not defined #3822

lolodomo · 2017-07-10T14:05:06Z

Fix issue #3741

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

sjsf

Thanks for providing this PR so quickly!
Nevertheless, I would prefer to keep separate things separate and rather split them up into several PRs.

Also I'm missing some test-cases which prevent any regressions - so could you please put down the new assumptions into ChannelStateDescriptionProviderOSGiTest?

sjsf · 2017-07-10T15:03:53Z

...e/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/items/GenericItem.java

                }
            }
+            if (found) {
+                fullStateDescription = new StateDescription(minimum, maximum, step, pattern, readOnly, options);


The idea here is to mix up state descriptions of different providers, right?
This will lead to quite unexpected results, therefore I don't think this is a good idea. Also I wouldn't want to mix this into a PR which intends to add a default state description at all (or I did not understand how it is related).

Yes, that is exactly the idea. It gives us a chance to get options from channel for example. I could add a comment.
What are the unexpected results ?
Do you prefer I keep this method unchanged and i create another one with that code ?
My PR is clearly to fix two different things that are more or less linked to state description. Splitting in two PRs means more work, more time. But ok, I will take the time !

sjsf · 2017-07-10T15:06:43Z

...e.smarthome.ui/src/main/java/org/eclipse/smarthome/ui/internal/items/ItemUIRegistryImpl.java

@@ -307,6 +309,14 @@ public String getLabel(Widget w) {
                            }
                        } else {
                            state = item.getState();
+                            if (stateDescription != null && stateDescription.getOptions() != null) {


How are the changes within this file related to the PR? I would not have expected any changes needed in the UI layer for this.

It is the fix for options. If you have options defined for a string channel, geLabel(w) should translate the value to its label. By the way, I think my code is not ewactly at the right place, I have to consider the case where the user requests for a transformation.

sjsf · 2017-07-10T15:10:01Z

...src/main/java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProvider.java

+                            pattern = "%.0f";
+                        }
+                        if (pattern != null) {
+                            logger.debug("Provide a default pattern {} for item {}", pattern, itemName);


please reduce it to trace level

sjsf · 2017-07-10T15:13:35Z

...sic/src/main/java/org/eclipse/smarthome/ui/basic/internal/render/AbstractWidgetRenderer.java

-        snippet = StringUtils.replace(snippet, "%label%", getLabel(w));
-        snippet = StringUtils.replace(snippet, "%value%", getValue(w));
-        snippet = StringUtils.replace(snippet, "%has_value%", new Boolean(hasValue(w)).toString());
+        // Optimization: avoid calling 3 times itemUIRegistry.getLabel(w)


While I agree this makes sense, I also would have preferred to keep it separate.

I will create a separate PR !

lolodomo · 2017-07-10T17:38:08Z

@SJKA : split done, you have here only the fix for issue #3741

lolodomo · 2017-07-10T20:39:23Z

Regarding ChannelStateDescriptionProviderOSGiTest, my Eclipse IDE is showing an error at this line: https://github.com/eclipse/smarthome/blob/master/bundles/core/org.eclipse.smarthome.core.thing.test/src/test/java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProviderOSGiTest.java#L76
JavaOSGiTest.bundleContext is not visible.

Same error in TranslationProviderOSGiTest.

Is there something broken with current tests ?

lolodomo · 2017-07-10T21:13:02Z

mvn clean install on org.eclipse.smarthome.core.thing.test succeeeded.
Should I just ignore the error in Eclipse ?

lolodomo · 2017-07-10T23:43:47Z

Tests have been added.

htreu

I did a quick review, please find some minor remarks in the java test. Which btw is awesome to have! Thanks for that.

htreu · 2017-07-11T07:00:06Z

.../java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProviderOSGiTest.java

+        Item item;
+        try {
+            item = itemRegistry.getItem("TestItem");
+        } catch (ItemNotFoundException e) {


The test could also be declared as throws Exception and fail happily if this happens. that would reduce lines 218 - 223 to Item item = itemRegistry.getItem("TestItem");.

If I do that, the next tests will not be run. Not a problem ?

The next assertions in this test method will not run, but as this aspect fails anyway this is not a problem. The other test methods should be executed individually.

htreu · 2017-07-11T07:00:34Z

.../java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProviderOSGiTest.java

@@ -182,6 +238,77 @@ public void presentItemStateDescription() {
        Assert.assertEquals("SOUND", opt.getValue());
        Assert.assertEquals("My great sound.", opt.getLabel());

+        try {
+            item = itemRegistry.getItem("TestItem2");
+        } catch (ItemNotFoundException e) {


htreu · 2017-07-11T07:01:56Z

.../java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProviderOSGiTest.java

@@ -153,22 +193,38 @@ public void presentItemStateDescription() {
                null, "test thing", new Configuration());
        Assert.assertNotNull(thing);


It would be nice to have all assert* methods to be static imports like in the other java tests.

htreu · 2017-07-11T07:09:28Z

.../java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProviderOSGiTest.java

        }
-        Assert.assertNotNull(numberItem);
+        Assert.assertNotNull(item);


This is obsolete since the next line will fail if item is null. Please also review other assertNotNull checks.

…efined Fix issue #3741 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2017-07-11T11:41:35Z

Ok, tests are now fixed according to your suggestions.

lolodomo · 2017-07-11T12:54:59Z

The Travis error has apparently nothing to do with my changes.

lolodomo · 2017-07-11T15:51:15Z

To be very clear, this PR changes the current behaviour.

The old behavior is to display a value for a string or number item only if a pattern is found either in the sitemap or in the item or in the thing channel, and only if there is no [] in the label at the first level. It could lead to no display of value, a frequent case for users using Paper UI to automatically link channels to items.

The new behavior is to always display a value for a string or number item except if there is a [] in the label at the first level. The pattern is taken first in the sitemap then in the item then in the thing channel and finally a default one is considered. The default one is [%s] for string item and [%.0f] for number items.

There is no change for other kinds of items.

lolodomo · 2017-07-11T15:53:19Z

Maybe we should consider a default pattern for DateTime items too.

lolodomo · 2017-07-14T09:06:28Z

Are you still waiting for me on this PR ? I think I handled all requests ?

sjsf · 2017-07-14T10:48:22Z

Thanks for the ping. No, the change looks good to me now.
Due to it's nature of changing the current behavior (to the better in my belief though), I would like to give @kaikreuzer / @maggu2810 also a change to express their consent here.

kaikreuzer

Thanks, looks good to me, just a few very minor remarks.

kaikreuzer · 2017-07-16T07:22:14Z

...src/main/java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProvider.java

+                    StateDescription state = channelType.getState();
+                    if ((channelType.getItemType() != null) && ((state == null) || (state.getPattern() == null))) {
+                        String pattern = null;
+                        if (channelType.getItemType().equalsIgnoreCase("String")) {


Better use the constant CoreItemFactory.STRING.

org.eclipse.smarthome.core.library is not exported.

Interesting. This is against our own guidelines, so feel free to add this export as part of this PR :-)

kaikreuzer · 2017-07-16T07:22:31Z

...src/main/java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProvider.java

+                        String pattern = null;
+                        if (channelType.getItemType().equalsIgnoreCase("String")) {
+                            pattern = "%s";
+                        } else if (channelType.getItemType().equalsIgnoreCase("Number")) {


Better use the constant CoreItemFactory.NUMBER.

kaikreuzer · 2017-07-16T07:24:16Z

...src/main/java/org/eclipse/smarthome/core/thing/internal/ChannelStateDescriptionProvider.java

@@ -57,7 +61,29 @@ public StateDescription getStateDescription(String itemName, Locale locale) {
            Channel channel = thingRegistry.getChannel(channelUID);
            if (channel != null) {
                ChannelType channelType = thingTypeRegistry.getChannelType(channel, locale);
-                return channelType != null ? channelType.getState() : null;
+                if (channelType != null) {
+                    StateDescription state = channelType.getState();


Could you call the variable stateDescription? (I know that the name of the getter method of ChannelType is not ideal...)

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

lolodomo · 2017-07-17T17:27:00Z

Done

sjsf · 2017-07-17T19:27:59Z

Great, thanks!

lolodomo mentioned this pull request Jul 10, 2017

Sonos binding: add state description on few channels #3800

Merged

sjsf suggested changes Jul 10, 2017

View reviewed changes

lolodomo changed the title ~~UI: handle channel options and provide a default pattern~~ UI: provide a default pattern for string and number channels if not defined Jul 10, 2017

htreu reviewed Jul 11, 2017

View reviewed changes

UI: provide a default pattern for string and number channels if not d…

e3ed686

…efined Fix issue #3741 Signed-off-by: Laurent Garnier <lg.hc@free.fr>

sjsf mentioned this pull request Jul 11, 2017

UI: display mapped option value #3825

Merged

sjsf added the Core label Jul 11, 2017

sjsf approved these changes Jul 14, 2017

View reviewed changes

kaikreuzer reviewed Jul 16, 2017

View reviewed changes

lolodomo added 2 commits July 16, 2017 14:03

Variable renamed in getStateDescription

dce9d7c

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

Use CoreItemFactory constants

230ab7c

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

sjsf merged commit ca531c4 into eclipse-archived:master Jul 17, 2017

lolodomo deleted the state_description branch July 17, 2017 19:29

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

lolodomo mentioned this pull request Aug 25, 2018

[Basic UI] Element Type Default for item of type Location or "How to render a PointType in a sitemap" #6110

Closed

lolodomo mentioned this pull request Jul 14, 2023

[Sitemap] Provide default value formatting for DateTime items openhab/openhab-core#3835

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: provide a default pattern for string and number channels if not defined #3822

UI: provide a default pattern for string and number channels if not defined #3822

lolodomo commented Jul 10, 2017 •

edited

sjsf left a comment

sjsf Jul 10, 2017

lolodomo Jul 10, 2017

sjsf Jul 10, 2017

lolodomo Jul 10, 2017

sjsf Jul 10, 2017

sjsf Jul 10, 2017

lolodomo Jul 10, 2017

lolodomo commented Jul 10, 2017

lolodomo commented Jul 10, 2017

lolodomo commented Jul 10, 2017

lolodomo commented Jul 10, 2017

htreu left a comment

htreu Jul 11, 2017

lolodomo Jul 11, 2017

htreu Jul 12, 2017

htreu Jul 11, 2017

htreu Jul 11, 2017

htreu Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 14, 2017 •

edited

sjsf commented Jul 14, 2017

kaikreuzer left a comment

kaikreuzer Jul 16, 2017

lolodomo Jul 16, 2017

kaikreuzer Jul 16, 2017

kaikreuzer Jul 16, 2017

kaikreuzer Jul 16, 2017

lolodomo Jul 16, 2017

lolodomo commented Jul 17, 2017

sjsf commented Jul 17, 2017

		@@ -153,22 +193,38 @@ public void presentItemStateDescription() {
		null, "test thing", new Configuration());
		Assert.assertNotNull(thing);

UI: provide a default pattern for string and number channels if not defined #3822

UI: provide a default pattern for string and number channels if not defined #3822

Conversation

lolodomo commented Jul 10, 2017 • edited

sjsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo commented Jul 10, 2017

lolodomo commented Jul 10, 2017

lolodomo commented Jul 10, 2017

lolodomo commented Jul 10, 2017

htreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo commented Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 11, 2017

lolodomo commented Jul 14, 2017 • edited

sjsf commented Jul 14, 2017

kaikreuzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo commented Jul 17, 2017

sjsf commented Jul 17, 2017

lolodomo commented Jul 10, 2017 •

edited

lolodomo commented Jul 14, 2017 •

edited