Sitemaps: widget confirm command parameter - #5769
Conversation
4c118dd to
ad0173b
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new confirmCmd widget parameter (boolean or conditional-rule based) to support UI confirmation prompts before sending commands from sitemap-based UIs. This extends the sitemap model, DTOs/REST payloads, and both DSL and YAML sitemap parsing/serialization paths so UIs can determine when confirmation is required.
Changes:
- Introduces
confirmCmdandconfirmCmdRulesinto the coreWidgetmodel and UI registry evaluation (ItemUIRegistry). - Propagates
confirmCmdthrough REST sitemap responses/events and UI component sitemap mapping/provider logic. - Extends DSL (Xtext) and YAML sitemap parsing/validation/conversion to support boolean + rule-based
confirmCmd.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/items/ItemUIRegistry.java | Adds getConfirmCmd(Widget) API to expose confirmation requirement. |
| bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java | Implements confirmation rule evaluation for widgets. |
| bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java | Maps confirmCmd config and rule parsing when building sitemaps from UI components. |
| bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapMapper.java | Exports confirmCmd and its rules when mapping widgets to UI components. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/Widget.java | Extends the widget contract with confirmCmd and confirmCmdRules. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/internal/WidgetImpl.java | Stores/returns confirmCmd and its rules in the default widget implementation. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/AbstractWidgetDTO.java | Adds confirmCmd to widget definition DTO base. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/WidgetDefinitionDTO.java | Adds confirmCmdRules to widget definition serialization model. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java | Maps confirmCmd and confirmCmdRules into/from definition DTOs. |
| bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/sitemaps/YamlWidgetDTO.java | Adds/validates confirmCmd for YAML widgets (boolean or rule form). |
| bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/sitemaps/YamlSitemapProvider.java | Applies YAML confirmCmd into runtime widget model. |
| bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/sitemaps/fileconverter/YamlSitemapConverter.java | Serializes confirmCmd/rules back into YAML output. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/validation/SitemapValidator.xtend | Adds validation warning for conflicting button params (confirmCmd + release cmd). |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/Sitemap.xtext | Extends DSL grammar to allow confirmCmd and introduces generalized boolean-rule list types. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java | Exports confirmCmd and rules when converting sitemap model to DSL AST. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/DslSitemapProvider.java | Imports confirmCmd and rules from DSL AST into runtime widget model. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/WidgetsChangeListener.java | Includes confirmCmd in widget events and change detection dependencies. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/WidgetDTO.java | Adds confirmCmd to REST widget DTO. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapWidgetEvent.java | Adds confirmCmd to server-sent widget event payload. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java | Includes confirmCmd in generated page/widget REST responses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (4)
bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/WidgetsChangeListener.java:285
- Confirmation-only dependency items never reach this predicate:
receivefilters events throughfilterItems, butgetAllItemsdoes not add items referenced bygetConfirmCmdRules(). If a rule depends on an item that is not otherwise used on the page, changing that item sends no widget event and clients keep a staleconfirmCmdvalue. Add confirmation-rule conditions toWidgetsChangeListener.getAllItems, alongside visibility/color/icon rules.
|| w.getConfirmCmdRules().stream().anyMatch(r -> conditionsDependsOnItem(r.getConditions(), name))
bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/sitemaps/YamlWidgetDTO.java:354
- Reject unsupported scalar values here. Currently
confirmCmd: "false"passes validation;YamlSitemapProvider.addWidgetRulesthen treats that string as a rule with no conditions, andgetConfirmCmdevaluates the empty condition list as true. Thus quoting a boolean silently reverses the user's intent. Only Boolean values should bypass rule validation; other non-null scalars should produce an error.
}
bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/validation/SitemapValidator.xtend:271
confirmCmdis generated fromEBooleanObject, so it is null when the parameter is omitted. Using it directly as the left operand of&&unboxes that null and can make validation fail with an NPE for ordinary Button widgets. Compare null-safely before checking the release command.
if (w.confirmCmd && w.releaseCmd !== null) {
bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/items/ItemUIRegistry.java:222
- Remove the extra article so the return description reads grammatically.
* @return true if the changing the item state requires a confirmation dialog
|
Awesome! Would it be possible to add the possibility to add a custom confirmation message, e.g. with |
Is it an idea to allow a string as input instead of boolean. If it is a string, we use that as the message. I would then extend the rule to a rule with an argument for the message. |
|
Whatever works. I'm just looking for a way to change the "Are you sure" text. |
It can still work with something like: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/validation/SitemapValidator.xtend:273
- This presence check warns for explicit
confirmCmd=false, although false requires no confirmation. Conversely, the conditional form populatesconfirmCmdRulesand leavesconfirmCmdnull, so that incompatible release-command combination gets no warning. Check for a true Boolean or rule-list presence instead.
if (w.confirmCmd != null && w.releaseCmd !== null) {
warning(buildMsgWithLineNb("Button widget has both confirmCmd and release command defined", w, null, null), w, null)
}
bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java:1234
- The new evaluator has no unit coverage, although the adjacent
getVisiblityevaluator is covered inItemUIRegistryImplTest. Add tests for the no-rule Boolean fallback, matching and nonmatching conditions, and rule precedence over the raw flag so regressions in REST/event output are caught.
public boolean getConfirmCmd(Widget w) {
I implemented it in the rules. If you give the rule an argument, that will be the message. A rule with only an argument just gives a new message. |
wborn
left a comment
There was a problem hiding this comment.
This PR is being reviewed with AI first.
The overall implementation looks consistent across the sitemap model, REST events, DSL, YAML, and UI-component mapping, but three functional issues remain around configuration and YAML handling. These should be addressed before merge.
The current CI build succeeds, and the attached static-analysis report does not show additional findings introduced by these changes.
wborn
left a comment
There was a problem hiding this comment.
The previous blocking findings have been addressed, but two functional issues remain in the current changes. Condition-only confirmation rules are not preserved correctly by the UI-component round trip, and YAML still accepts an empty confirmation rule that becomes unconditional at runtime.
The UI-component round-trip issue changes valid confirmation-rule semantics and should be addressed before merge. The YAML validation issue should also be corrected for consistency with the DSL validation.
This review was AI-assisted.
| addWidgetRules(widget.getValueColor(), component, "valuecolor"); | ||
| addWidgetRules(widget.getIconColor(), component, "iconcolor"); | ||
| addWidgetRules(widget.getIconRules(), component, "iconrules"); | ||
| addWidgetRules(widget.getConfirmCmdRules(), component, "confirmcmdrules"); |
There was a problem hiding this comment.
confirmcmdrules cannot use the generic non-visibility parsing path here because a confirmation rule's message is optional. For example, a condition-only rule such as DemoNumber>"25" is serialized without a message, but getRuleArgument() uses lastIndexOf("=") + 1. Since this rule contains no =, the entire rule is interpreted as the argument and no conditions remain, turning the conditional confirmation into an unconditional one with the rule text as its message.
Rules using ==, !=, <=, or >= are also affected because an = from the comparison operator can be mistaken for the message separator. Please parse confirmation rules so the optional message is distinguished from comparison operators instead of assuming every non-visibility rule has an argument.
There was a problem hiding this comment.
The comparison-operator case is fixed now, but a few parsing cases still remain. These can be reproduced with a sitemap like:
sitemap parserTest label="Parser test" {
Switch item=DemoSwitch label="Condition only" confirmCmd=[DemoNumber>"25"]
Switch item=DemoSwitch label="Equals in message" confirmCmd=[DemoSwitch==ON="Set x=10?"]
Switch item=DemoSwitch label="Equals in condition value" confirmCmd=[DemoString=="a=b"]
Text item=DemoString label="Visibility" visibility=[DemoString=="a=b"]
}
For DemoNumber>"25", lastIndexOf("=") returns -1, so substring(lastEqualsIndex + 1) becomes substring(0). The complete rule is therefore interpreted as the argument instead of as a condition. It should require confirmation only when DemoNumber > 25, using the default confirmation message.
lastIndexOf("=") is also not quote-aware. In DemoSwitch==ON="Set x=10?", the last = is inside the confirmation message, so it is mistaken for the separator. A condition-only rule such as DemoString=="a=b" has the same problem because the last = is inside the quoted condition value.
This also affects existing visibility rules now that visibility calls getRuleArgument() as well; visibility=[DemoString=="a=b"] has no rule argument at all, but the = inside the quoted state can be mistaken for one.
Please detect the optional argument/message separator outside quoted strings and only when it is not part of ==, !=, <=, or >=. Round-trip tests using sitemap examples like the above would cover these cases well.
wborn
left a comment
There was a problem hiding this comment.
The previous configuration, YAML scalar, and message-only rule findings have been addressed, but two functional issues remain.
The UI-component rule parser still mishandles condition-only confirmation rules, and the attempted fix now also regresses existing visibility-rule parsing. YAML validation also still accepts an empty confirmation rule through the and representation.
The parser regression should be addressed before merge. The YAML validation issue is a continuation of the previously reported empty-rule problem and should also be corrected for consistency.
This review was AI-assisted.
230a4fa to
b226060
Compare
wborn
left a comment
There was a problem hiding this comment.
The YAML empty-rule cases from the previous review are fixed on the current revision, and the current CI run passes. One correctness issue remains in the UI-component rule parser: condition-only </> rules and = characters inside quoted values/messages can still be misparsed, including existing visibility rules. The existing parser thread has been updated with concrete sitemap examples and the remaining cases.
This review was AI-assisted.
This should be fixed (and tests added) now. Ultimately, it would probably better to remove storing managed sitemaps as UI components and create its own store for it, but it would mean creating an upgrade tool to migrate the sitemap storage. That would avoid the whole mapping and parsing, and avoid storing a combination of fields into a string requiring string parsing. That's a separate thing to do and not part of this PR. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
6730028 to
9e8cc93
Compare
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
| public String labelcolor; | ||
| public String valuecolor; | ||
| public String iconcolor; | ||
| public String confirmCmdMessage; |
There was a problem hiding this comment.
Can this one be named confirmCommandMessage or even better commandConfirmationMessage for consistency to releaseCommand?
There was a problem hiding this comment.
Can this one be named
confirmCommandMessageor even bettercommandConfirmationMessagefor consistency toreleaseCommand?
I tried to keep the terminology consistent from the configuration through to the client DTO field and wanted to keep it compact. But I can change that of course. Do you want this changed only in the client widget DTO?
There was a problem hiding this comment.
I don't have a strong preference either way, as a client maintainer I mostly just care about the REST API 😉
Thinking about it, though, how does a single message suffice if the sitemap syntax allows for different messages for different commands? Or am I wrong and this is not actually possible? Switch item=SomeSwitch confirmCmd=[ON = "Do you want to switch on?", OFF = "Do you want to switch off?"]
There was a problem hiding this comment.
Thinking about it, though, how does a single message suffice if the sitemap syntax allows for different messages for different commands? Or am I wrong and this is not actually possible?
Switch item=SomeSwitch confirmCmd=[ON = "Do you want to switch on?", OFF = "Do you want to switch off?"]
That is indeed a misunderstanding. It will check the current state, not the target state. So in this example, the syntax would be: Switch item=SomeSwitch confirmCmd=[OFF = "Do you want to switch on?", ON = "Do you want to switch off?"].
That aligns with all handling of icon, color and visibility. You would indeed need more logic in the client to be able to do it based on the command itself. The current logic is fine for e.g. a switch, but could not differentiate between target states of e.g. a selection or slider widget. Do we need that extra complexity? It is very different logic from what we already do for other rules and moves the responsability of rule handling for this dialog to the client. The current mechanism also handles conditions not related to the item being commanded, e.g. when you are at home you don't ask for confirmation, but if you are not you do.
I started with just a simple message, but then added the ability to change the message content and support rules in analogy with visibility, icon and color handling. All of these are transparent to the client. The client would just see a message to show. Much of this could be achieved just with visibility rules and double definitions, but directly supporting rules makes things easier on the configuration side. And it allowed me to easily have custom messages.
|
How does this
become this:
And how are
Also, what is the expected timeline for rolling out this change? |
|
@mherwege : Thanks for mentioning the App maintainers, indeed it needs to be implemented within openHAB Sailfish OS App as well - I will put it on the Agenda |
Unless I am mistaken clients won't see operator and argument. The two code snippets you quoted are just 2 ways of describing a sitemap on the server side. The clients should just see a string |
Indeed. And to complete this, that
These were just configuration examples in 2 different formats (DSL and YAML) and are not the same configuration. This is not visible to the clients. |
The core, functionality in this PR has already gone through a number of review rounds and is the basis for it. As soon as this gets merged, the functionality should be available to be adopted in UI's. If the UI does not support it, configuration will just not have any impact. Merging this PR depends on review capacity and progress. The UI configuration PR and BasicUI PR are code complete, but still need full review. They are not needed for implementation in other clients. Ideally, we would have this supported in as many clients as possible for 5.3.0. But that of course depends on client side development and review capacity. I focussed on what is needed from a core and webui perspective, which gets delivered with the OH release. Other clients are more flexible in their release cycle. |
|
Question for all, should I change |
Slight correction: suggested was |
Yes, sorry, I did not think that through. I was looking at the YAML and thinking of the REST API JSON.
Would that message always be present, or could there also simply be a true value that should fall back to a built-in default message, similar to how it works in the YAML? |
If it is not present or null, no confirmation dialog should be presented. Otherwise, the message is always there, and the text should be shown. The default is defined in core and, if the user does not override it, will be sent. The default can be changed by system configuration in core (and could also be translated through the translation layer in core). |
The requirement to confirm the command before sending it to the OH server when using a control in a sitemap based UI has come up several times, recently here: https://community.openhab.org/t/announcing-wear-os-app-for-openhab/169918/37
This PR implements a
confirmCmdparameter on all sitemap widgets, if set should open a confirmation dialog when using the control before committing the new state to OH. This confirmation dialog of course needs to be implemented for each sitemap based UI to take effect.Here is an example of a sitemap using this parameter on various controls:
The
confirmCmdparameter can be defined as a straight boolean parameter (default is false), or using a rule with the same syntax as visibility rules. This allows making the need for confirmation depend on conditions, as shown in 2 examples in the sitemap above.This is the core part of the implementation. It is tested with DSL provided sitemaps, but is also implemented for YAML.
I have also implemented this functionality in BasicUI.
In BasicUI, this functionality looks like:

As an extra, I now also implemented a variable text for the dialog. Examples:
One remark, in a DSL rule with a confirmation text, even without conditions, the
=before the text message is required. This is needed to avoid a syntax ambiguity (you could have a condition with only a condition value, and that would be interpreted the same as the argument). So that is a slight deviation from the requirements for other rules.There is a configuration property with the default confirmation dialog text, that can be changed system wide. The default can be translated.
Here is a YAML example configuration:
Notice the last widget example will use the literal
trueas a confirmation message text, while the first example will use the default message text.The implementation for BasicUI can be found here: https://github.com/openhab/openhab-webui/4448
UI sitemap editor support is in: https://github.com/openhab/openhab-webui/4462
FYI @openhab/android-maintainers @openhab/ios-maintainers @openhab/sailfishos-maintainers @openhab/garmin-maintainers This is potentially interesting to implement in the mobile apps sitemap support. We can still adjust the core functionality if needed, but clients need to implement it. Let me know if you see issues.