Sitemaps: validate icon when provided as string

[mherwege]

Closes #5706

Hypens are now allowed in all segments of an icon name, with a maximum of 3 segments separated by :.
Icons names provided as a string will also get validated with the same rules and throw an error when not valid.
For backward compatibility, file extensions are still allowed DSL definitions of icons (only when provided as a string in quotes), but will be dropped when loading the DSL file, or saving in the UI. This should avoid problems with old sitemaps and conversions to YAML.

[Copilot]

Pull request overview

This PR tightens and aligns sitemap icon-name validation between DSL (including quoted-string icon values) and YAML/UI behavior, while expanding allowed hyphen usage across all icon segments and keeping backward compatibility for legacy icon file extensions.

Changes:

• Updates sitemap DSL grammar to allow hyphens in every colon-separated icon segment (up to 3 segments).
• Ensures icons provided as quoted strings are validated using the same segment rules as unquoted icons.
• Allows legacy file extensions in quoted DSL icon strings, dropping the extension during conversion for compatibility with older sitemaps.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/valueconverter/SitemapConverters.java	Updates the Icon value converter to validate string-provided icons and handle legacy extensions while normalizing icon values.
bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/Sitemap.xtext	Adjusts the Icon grammar to permit hyphens in all icon segments (up to 3 colon-separated segments).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/valueconverter/SitemapConverters.java:48

• The file-extension stripping uses lastIndexOf('.') on the full icon string, so a dot that appears before a namespace separator (e.g. "oh.png:classic:switch") will silently truncate the icon to "oh" instead of rejecting it as invalid. This can mask user errors and change the icon value unexpectedly. Consider only stripping an extension when the dot is in the last segment (after the last :), and don’t strip a trailing . (let validation fail).

                // For backward compatibility, we allow the icon name to contain a file extension, but we remove it when
                // validating the name
                int lastDotIndex = trimmedString.lastIndexOf(".");
                trimmedString = lastDotIndex != -1 ? trimmedString.substring(0, lastDotIndex) : trimmedString;
                String[] segments = trimmedString.split(":", -1);

[lolodomo]

If I am not wrong a---b and a--- will be accepted in YAML.
Is it valid with your changed in DSL ? Probably not without quotes but is it at least with quotes ?

[wborn]

AI reviewed this PR first, before manual maintainer review.

The overall direction looks good: quoted icon values are now validated consistently with unquoted values, hyphens are allowed in all icon segments to match the YAML validation rules, and the earlier issues around whitespace handling and escaped identifiers have been addressed.

There is still one edge case in the file-extension compatibility handling where malformed multi-segment icon names can be silently converted into a different valid icon instead of being rejected. This should be fixed before merging.

Could we also add regression tests for the changed icon grammar and converter behavior? In particular, it would be useful to cover valid hyphens in each segment, quoted and unquoted values, more than three segments, invalid/empty segments, whitespace, escaped identifiers, legacy values such as "switch.png" / "oh:classic:switch.png", and malformed values such as "oh.png:classic:switch". This should help ensure the DSL validation remains aligned with the existing YAML icon validation and protects the backward-compatibility behavior from future regressions.

[wborn]

The previous extension-handling issue has been addressed: extensions are now stripped only from the final segment, so malformed values such as "oh.png:classic:switch" are no longer silently converted.

There is still a validation compatibility issue with quoted icon values. Purely numeric segments such as 123 are valid according to the YAML validator and Items DSL, but are rejected by this converter. This also breaks the converter's own round trip: internalToString("123") serializes the value as the quoted DSL token "123", which internalToValue then rejects when that token is parsed again.

The existing concern about repeated or trailing hyphens (a---b, a---) appears to be another instance of the same mismatch between the sitemap converter and the YAML/Items icon validation rules. It would be preferable for these validation rules to stay aligned.

Regression tests for the changed grammar and converter behavior are still missing. In particular, tests should cover quoted and unquoted values, numeric segments, repeated and trailing hyphens, escaped identifiers, one to three segments, invalid or empty segments, and the legacy file-extension handling.

[wborn]

Purely numeric icon segments are valid according to the YAML validator and the Items DSL, but this converter rejects them after unquoting because ID_EXT_PATTERN requires a letter or underscore after leading digits.

For example, given the Java value:

String value = "123";

internalToString(value) serializes it as the quoted sitemap DSL token:

"123"

If that token were represented as a Java string passed to internalToValue, it would be:

internalToValue("\"123\"", node)

The escaped quotes in this Java example represent the actual quotes in the sitemap DSL; they are not part of the icon name. After unquoting, the icon name is simply 123, which is then rejected by ID_EXT_PATTERN.

Similarly, the legacy quoted DSL value "123.png" is reduced to the icon name 123 after removing the extension and then fails the same validation.

This introduces a compatibility and round-trip regression for quoted values. Could the quoted-string validation use the same icon-segment rules as YAML/Items, or otherwise ensure that values emitted by internalToString can be parsed again?

[mherwege]

Purely numeric icon segments are valid according to the YAML validator and the Items DSL, but this converter rejects them after unquoting because ID_EXT_PATTERN requires a letter or underscore after leading digits.

It looks like they were never aligned in the first place. I only aligned the quoted icon definitions in a sitemap with the unquoted definitions in a sitemap DSL definition that was checked directly through the syntax before. The current code does exactly that I believe. Purely number segments in an unquoted definition were not accepted before. So the question is, should the constraints be relaxed here (allowing only number segments) or be stricter in the items and yaml code?

Here is where the difference started, more then 10 years ago: openhab/smarthome@6a41ae5

[mherwege]

@wborn I aligned the icon syntax in sitemaps DSL and items DSL now.

It is challenging:

• The ID syntax is different in the sitemaps and items DSL. It was changed at one point in items DSL and alignment was lost then. The Icon syntax references the ID syntax.
• I cannot use the items DSL ID syntax in the sitemap DSL as that would conflict with INT. INT is not defined in the items syntax, but is needed in the sitemap syntax.
• I therefore needed to extend the conversion and validation logic to make it work with a slightly adjusted version of the sitemap ID syntax (I was able to add -).
• Because of adding - in the ID syntax, I now also check there are no dashes for item names (aligned with the same check in items DSL).
• Adding - breaks the Period syntax, so I now also implemented enhanced Period syntax validation (and allow it to be defined as String).

With all of this, the DSL sitemap syntax validation should be stronger than before. I have tested all of these changes on a number of sitemaps, and they still load fine. But as this is becoming a much larger change then I set out doing, the risk is increasing as well.

[wborn]

Thanks for working through the side effects of allowing hyphens in icon segments. One thing that seems worth reconsidering is whether - needs to become part of the general ID terminal.

Since ID is used in many places in the sitemap grammar, broadening it also changes how item names, sitemap names, periods, labels, commands, states, etc. are tokenized. That seems to be what led to the additional validation and the new Period handling in this revision.

Could the hyphen support instead be kept local to IconSegment, for example by composing the existing ID / INT rules with '-', along the lines of:

IconSegment:
    (ID | INT) ('-' (ID | INT)?)*;

That would keep the existing ID semantics everywhere else and potentially avoid most of the additional sitemap/item-name validation and Period changes. The icon converter would still need to handle serialization of the individual hyphen-separated parts, but the change would stay much closer to the original goal of this PR and reduce the risk of affecting unrelated sitemap syntax.

Does that approach work here, or is there a reason the global ID rule needs to be extended?

[mherwege]

Thanks for working through the side effects of allowing hyphens in icon segments. One thing that seems worth reconsidering is whether - needs to become part of the general ID terminal.

I asked myself the same question. It is currently part of ID syntax in the items DSL. And this difference leads to the problem in the first place. Also in the items DSL, when hyphen was added, the extra validation for the item name was added to block hyphens in item names. So to avoid future inconsistencies, I was going for aligning the definitions as much as possible and handle the difference separatly.

Since ID is used in many places in the sitemap grammar, broadening it also changes how item names, sitemap names, periods, labels, commands, states, etc. are tokenized. That seems to be what led to the additional validation and the new Period handling in this revision.

True, but looking at it:

• item names are now treated exactly the same way as in items DSL, with an extra validation.
• labels should have no problem with hypens. It was already allowed before when quoted.
• periods were not properly validated before anyway, so this now does proper validation. It could be much simpler, just checking for the previous syntax, but I opted to make the validation complete. And there was a bug in the syntax for periods. It should be allowed to start with a hyphen for a future only period definition.
• commands and states should allow hyphens, before it did need to be quoted to make it possible.

I don't think there are any other uses of ID in the syntax.

Could the hyphen support instead be kept local to IconSegment, for example by composing the existing ID / INT rules with '-', along the lines of:

IconSegment:
    (ID | INT) ('-' (ID | INT)?)*;

That could potentially be an option (to be tested,). But that still doesn't cover all allowed syntax in the YAML validation. The YAML validation allows multiple hypens in a row. If that makes sense is another question, but it would still block the round trip.