Fix UI defined sitemaps #3850

mherwege · 2023-10-18T10:12:09Z

Recent enhancements to the sitemap syntax completely broke UI defined sitemaps.

This PR makes sitemap definitions in the UI work again, and is a quick fix.

However, most of the new syntax will still not work in the UI. This requires a larger refactoring both in core and webui.

EDIT:
With the second commit, AND conditions in visibility, color and icon rules are now allowed.
The JSON config format has not been changed (except for the addition of iconrules) to keep full backward compatibility and avoid major refactoring on the UI side. Logic has been created to correctly interpret the condition text strings.

This logic has one inherent problem. Condition strings will be split on ' AND ' strings in their composing parts. If any of the comparisons contains an ' AND ' string (in capitals, including the blanks before and after), provider will give the wrong result.

I will follow up with a UI PR to introduce icon rules and allow AND conditions.

@lolodomo Could you please have a look.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo

Partial review

lolodomo · 2023-10-21T07:49:57Z

...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java

-        sitemap.setLabel(rootComponent.getConfig().get("label").toString());
+        Object label = rootComponent.getConfig().get("label");
+        if (label == null) {
+            return sitemap;


Why returning ? In this case, you will not set the children.

Label is a required parameter. It being null should not happen as it is verified in the UI when constructing the sitemap. But this avoids a null waring in Eclipse.

Label is absolutely not mandatory but optional.
https://github.com/openhab/openhab-core/blob/main/bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/Sitemap.xtext#L10

Sorry, my mistake. Previous code would have failed also if no label existed.

Checking UI code, UI configured sitemaps always imposed a label. I will remove the requirement in the UI.

Checking again, the UI heavily relies on the label being available. The name of the sitemap will always be something like 'uicomponents_...', so is not very good anyway. I propose to keep enforcing a label for UI provided sitemaps.

Ok, makes sense to me.

lolodomo · 2023-10-21T07:55:20Z

...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java

+            return;
+        }
+        Object staticIcon = component.getConfig().get("staticIcon");
+        if (staticIcon != null && Boolean.valueOf(ConfigUtil.normalizeType(staticIcon).toString())) {


Not sure to understand: staticIcon is not a boolean but a string containing the icon.

It is not in the JSON db and JSON config. It is only in the grammar. The UI simply shows a switch turning staticIcon on or off. The state of that switch is what goes into the field. For the UI that is a lot nicer than showing 2 icon fields and having to move the value between them if switching to static. It could obviously be solved differently in the UI, but I see no reason to make it more complex there.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo

End of review

lolodomo · 2023-10-21T10:18:44Z

...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java

+        String conditions = rule;
+        if (argument != null) {
+            conditions = rule.substring(0, rule.lastIndexOf(argument)).trim();
+            if (conditions.endsWith("=")) {


And if there is one or several spaces between = and the "argument" ?

The full rule will be something like:

Item>=5 = red

The passed in argument would be red.
If there is an argument (!= null), it is split off and conditions should end with = (after trim). If there is a second = before, it should actually be part of the state string (5 in the example). So I think this is right.

Spaces after = are removed using trim.

I did not take attention that the previous line ended with a call to trim(). So, no problem, you can ignore this comment.

lolodomo · 2023-10-21T10:19:19Z

...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java

+                conditions = conditions.substring(0, conditions.length() - 1);
+            }
+        }
+        List<String> conditionsList = List.of(conditions.split(" AND "));


Does UI systematically adds space around the AND ?

Yes, it does. And the chance of having an erronuous match is lower when adding the spaces in the separator string.

lolodomo · 2023-10-21T10:21:22Z

...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java

-                        condition.setSign(matcher.group("sign"));
-                        condition.setState(matcher.group("state"));
-                        colorArray.eSet(SitemapPackage.COLOR_ARRAY__CONDITIONS, condition);
+                        colorArray.eSet(SitemapPackage.COLOR_ARRAY__ARG, argument);


Using colorArray.setArg is probably proper.

lolodomo · 2023-10-21T10:21:41Z

...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java

-                        iconDef.add(iconRule);
-                    } else {
-                        logger.warn("Syntax error in icon rule '{}' for widget {}", sourceIcon, component.getType());
+                        iconRule.eSet(SitemapPackage.ICON_RULE__ARG, argument);


Using iconRule.setArg is probably proper.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo

LGTM, thank you a lot

mherwege · 2023-10-21T12:02:17Z

@lolodomo Thanks for the quick review.

lolodomo · 2023-10-22T21:24:44Z

@openhab/core-maintainers : a merge would be appreciated.

kaikreuzer

Thanks @mherwege!

Fixes #2149: Syntax validation errors when command mappings contain special characters like [ and ]. Sitemaps now support icon rules and AND conditions in icon, color and visibility rules. Mappings now also support an icon. This PR introduces support to configure this in the sitemap editor. It also contains a fix for staticIcon support. Depends on openhab/openhab-core#3850. Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

fix UI defined sitemaps

0f56c22

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege requested a review from a team as a code owner October 18, 2023 10:12

allow AND conditions

a0ca197

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

mherwege mentioned this pull request Oct 20, 2023

Sitemap editor: Support AND conditions and icon rules openhab/openhab-webui#2145

Merged

lolodomo reviewed Oct 21, 2023

View reviewed changes

label not required

05df117

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo suggested changes Oct 21, 2023

View reviewed changes

review adjustments

61774ed

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

lolodomo approved these changes Oct 21, 2023

View reviewed changes

kaikreuzer added the bug An unexpected problem or unintended behavior of the Core label Oct 26, 2023

kaikreuzer approved these changes Oct 26, 2023

View reviewed changes

kaikreuzer changed the title ~~fix UI defined sitemaps~~ Fix UI defined sitemaps Oct 26, 2023

kaikreuzer added this to the 4.1 milestone Oct 26, 2023

kaikreuzer merged commit d75d6df into openhab:main Oct 26, 2023
3 checks passed

mherwege deleted the ui_sitemap branch October 27, 2023 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UI defined sitemaps #3850

Fix UI defined sitemaps #3850

mherwege commented Oct 18, 2023 •

edited

Loading

lolodomo left a comment

lolodomo Oct 21, 2023

mherwege Oct 21, 2023

lolodomo Oct 21, 2023 •

edited

Loading

mherwege Oct 21, 2023

mherwege Oct 21, 2023

mherwege Oct 21, 2023

lolodomo Oct 21, 2023

lolodomo Oct 21, 2023

mherwege Oct 21, 2023 •

edited

Loading

lolodomo Oct 21, 2023

lolodomo left a comment

lolodomo Oct 21, 2023

mherwege Oct 21, 2023 •

edited

Loading

lolodomo Oct 21, 2023

lolodomo Oct 21, 2023

mherwege Oct 21, 2023 •

edited

Loading

lolodomo Oct 21, 2023

mherwege Oct 21, 2023

lolodomo Oct 21, 2023 •

edited

Loading

mherwege Oct 21, 2023

lolodomo left a comment

mherwege commented Oct 21, 2023

lolodomo commented Oct 22, 2023

kaikreuzer left a comment

Fix UI defined sitemaps #3850

Fix UI defined sitemaps #3850

Conversation

mherwege commented Oct 18, 2023 • edited Loading

lolodomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo Oct 21, 2023 • edited Loading

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

mherwege Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mherwege Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mherwege Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

mherwege commented Oct 21, 2023

lolodomo commented Oct 22, 2023

kaikreuzer left a comment

Choose a reason for hiding this comment

mherwege commented Oct 18, 2023 •

edited

Loading

lolodomo Oct 21, 2023 •

edited

Loading

mherwege Oct 21, 2023 •

edited

Loading

mherwege Oct 21, 2023 •

edited

Loading

mherwege Oct 21, 2023 •

edited

Loading

lolodomo Oct 21, 2023 •

edited

Loading