-
-
Notifications
You must be signed in to change notification settings - Fork 457
Sitemap DSL and YAML serialization #4945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mark Herwege <[email protected]>
It looks like my change to the grammar made the grammar too big the the Language Server. All else looks like it is working. |
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
The grammar being too big has been resolved. I move the variable sequence of unique properties out of the syntax and build extra validation checks to cope with this. XText is apparantly very bad at these variable sequences (using the I now also force the use of the serializer, so sitemaps created programatically and not from a file get formatted properly. |
Signed-off-by: Mark Herwege <[email protected]>
Feature verification fails when running a parallel build. It succeeds when running locally with a sequential build. I don't know how to solve that. |
Signed-off-by: Mark Herwege <[email protected]>
The last commit adds YAML serialization. Here is an example output:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends DSL serialization capabilities from items and things to include sitemap definitions, enabling serialization to both DSL and YAML formats. This change serves as a foundation for potentially removing the specific sitemap parser from the UI and using REST calls instead.
The key changes include:
- Modified sitemap grammar to use named lists instead of unnamed lists for better serialization
- Updated all existing code to work with the new list wrapper objects
- Added DSL and YAML file converters for sitemaps
- Enhanced validation and formatting for sitemaps
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
File | Description |
---|---|
ItemUIRegistryImplTest.java |
Updated tests to work with new list wrapper objects |
ItemUIRegistryImpl.java |
Modified to access elements through list wrapper objects |
UIComponentSitemapProvider.java |
Updated parameter types and element access patterns |
YamlSitemapFileConverter.java |
New YAML converter for sitemap serialization |
YamlWidgetDTO.java |
New DTO for YAML widget serialization |
YamlSitemapDTO.java |
New DTO for YAML sitemap serialization |
YamlRuleDTO.java |
New DTO for YAML rule serialization |
YamlMappingDTO.java |
New DTO for YAML mapping serialization |
DslSitemapFileConverter.java |
New DSL converter for sitemap serialization |
SitemapFormatter.xtend |
Enhanced formatting rules for sitemap DSL |
Sitemap.xtext |
Modified grammar to use named list containers |
SitemapValidator.xtend |
Enhanced validation with line number reporting |
Various other files | Updated to work with new serializer infrastructure |
@@ -0,0 +1,10 @@ | |||
package org.openhab.core.model.sitemap.formatting; |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is missing the standard copyright header that is present in all other files in the project.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
} else if (w instanceof Switch sw) { | ||
StateDescription stateDescr = i.getStateDescription(); | ||
if (sw.getMappings().isEmpty() && (stateDescr == null || stateDescr.getOptions().isEmpty())) { | ||
if ((sw.getMappings() == null || sw.getMappings().getElements().isEmpty()) |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check pattern for getMappings() is repeated multiple times. Consider extracting this into a helper method like hasMappings(Switch sw)
to reduce code duplication.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only a single call to this in the class.
private @Nullable String processColorDefinition(Widget w, @Nullable ColorArrayList colorArrayList, | ||
String colorType) { | ||
if (colorArrayList == null) { | ||
return null; | ||
} | ||
List<ColorArray> colorList = colorArrayList.getElements(); | ||
// Sanity check |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check and getElements() pattern is repeated across multiple methods (processColorDefinition, getVisiblity, getConditionalIcon). Consider creating a utility method to safely get elements from these list wrapper objects.
private @Nullable String processColorDefinition(Widget w, @Nullable ColorArrayList colorArrayList, | |
String colorType) { | |
if (colorArrayList == null) { | |
return null; | |
} | |
List<ColorArray> colorList = colorArrayList.getElements(); | |
// Sanity check | |
/** | |
* Utility method to safely get elements from a list wrapper object. | |
* Returns an empty list if the wrapper is null or contains no elements. | |
*/ | |
private <T> List<T> safeGetElements(@Nullable Object wrapper) { | |
if (wrapper == null) { | |
return Collections.emptyList(); | |
} | |
if (wrapper instanceof ColorArrayList) { | |
List<ColorArray> list = ((ColorArrayList) wrapper).getElements(); | |
return list != null ? list : Collections.emptyList(); | |
} else if (wrapper instanceof VisibilityRuleList) { | |
List<VisibilityRule> list = ((VisibilityRuleList) wrapper).getElements(); | |
return list != null ? list : Collections.emptyList(); | |
} else if (wrapper instanceof IconRuleList) { | |
List<IconRule> list = ((IconRuleList) wrapper).getElements(); | |
return list != null ? list : Collections.emptyList(); | |
} | |
return Collections.emptyList(); | |
} | |
private @Nullable String processColorDefinition(Widget w, @Nullable ColorArrayList colorArrayList, | |
String colorType) { | |
List<ColorArray> colorList = safeGetElements(colorArrayList); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getElements does not return List, but EList so this suggestion does not work.
dto.icon = widget.getStaticIcon() != null ? widget.getStaticIcon() | ||
: (widget.getIconRules() == null ? widget.getIcon() : null); | ||
IconRuleList iconRuleList = widget.getIconRules(); | ||
if (iconRuleList != null && iconRuleList.getElements() != null && iconRuleList.getElements().size() > 0) { |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use !iconRuleList.getElements().isEmpty()
instead of iconRuleList.getElements().size() > 0
for better readability and performance.
if (iconRuleList != null && iconRuleList.getElements() != null && iconRuleList.getElements().size() > 0) { | |
if (iconRuleList != null && iconRuleList.getElements() != null && !iconRuleList.getElements().isEmpty()) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
} | ||
if (widget instanceof Switch switchWidget) { | ||
for (Mapping mapping : switchWidget.getMappings()) { | ||
for (Mapping mapping : switchWidget.getMappings().getElements()) { |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: getMappings() could return null, but there's no null check before calling getElements(). This should be guarded with a null check like other similar patterns in the codebase.
Copilot uses AI. Check for mistakes.
} | ||
// Consider items involved in any icon condition | ||
items.addAll(getItemsInIconCond(widget.getIconRules())); | ||
items.addAll(getItemsInIconCond(widget.getIconRules().getElements())); |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: getIconRules() could return null, but there's no null check before calling getElements(). This should be guarded with null checks like the pattern used in WidgetsChangeListener.
items.addAll(getItemsInIconCond(widget.getIconRules().getElements())); | |
if (widget.getIconRules() != null) { | |
items.addAll(getItemsInIconCond(widget.getIconRules().getElements())); | |
} else { | |
items.addAll(getItemsInIconCond(List.of())); | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
items.addAll(getItemsInColorCond(widget.getLabelColor())); | ||
items.addAll(getItemsInColorCond(widget.getValueColor())); | ||
items.addAll(getItemsInColorCond(widget.getIconColor())); | ||
items.addAll(getItemsInVisibilityCond(widget.getVisibility().getElements())); |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: getVisibility() could return null, but there's no null check before calling getElements(). This should be guarded with null checks like the pattern used in WidgetsChangeListener.
items.addAll(getItemsInVisibilityCond(widget.getVisibility().getElements())); | |
if (widget.getVisibility() != null) { | |
items.addAll(getItemsInVisibilityCond(widget.getVisibility().getElements())); | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
items.addAll(getItemsInColorCond(widget.getValueColor())); | ||
items.addAll(getItemsInColorCond(widget.getIconColor())); | ||
items.addAll(getItemsInVisibilityCond(widget.getVisibility().getElements())); | ||
items.addAll(getItemsInColorCond(widget.getLabelColor().getElements())); |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: getLabelColor() could return null, but there's no null check before calling getElements(). This should be guarded with null checks like the pattern used in WidgetsChangeListener.
items.addAll(getItemsInColorCond(widget.getLabelColor().getElements())); | |
if (widget.getLabelColor() != null) { | |
items.addAll(getItemsInColorCond(widget.getLabelColor().getElements())); | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
items.addAll(getItemsInColorCond(widget.getIconColor())); | ||
items.addAll(getItemsInVisibilityCond(widget.getVisibility().getElements())); | ||
items.addAll(getItemsInColorCond(widget.getLabelColor().getElements())); | ||
items.addAll(getItemsInColorCond(widget.getValueColor().getElements())); |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: getValueColor() could return null, but there's no null check before calling getElements(). This should be guarded with null checks like the pattern used in WidgetsChangeListener.
items.addAll(getItemsInColorCond(widget.getValueColor().getElements())); | |
items.addAll(getItemsInColorCond(widget.getValueColor() != null ? widget.getValueColor().getElements() : org.eclipse.emf.common.util.ECollections.emptyEList())); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
items.addAll(getItemsInVisibilityCond(widget.getVisibility().getElements())); | ||
items.addAll(getItemsInColorCond(widget.getLabelColor().getElements())); | ||
items.addAll(getItemsInColorCond(widget.getValueColor().getElements())); | ||
items.addAll(getItemsInColorCond(widget.getIconColor().getElements())); |
Copilot
AI
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: getIconColor() could return null, but there's no null check before calling getElements(). This should be guarded with null checks like the pattern used in WidgetsChangeListener.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Signed-off-by: Mark Herwege <[email protected]>
From a functional perspective, this PR is complete. It will generate DSL and YAML representation of sitemaps defined in files or through the UI. A next step would be be able to parse sitemaps from YAML files. The issue with including it here is that it does require architectural changes to how sitemaps are managed. There currently is no single sitemap registry, rather a registry in the model (used to generate YAML as well) and a separate registry for UI managed sitemaps (that also has a provider for the model in the sitemap model classes). All sitemap uses are from the model classes. The UI sitemap registry does not have visibility in the xtext model sitemap, therefore the non-managed sitemaps configuration is not visible in the UI. This is a bigger refactoring which I am happy to take on. But I first would want to know if I am on an acceptable track with this. I probably should then do the registry creation and resulting refactoring before I get this PR updated. |
resource.getContents().add(modelContent); | ||
resource.save(out, Map.of(XtextResource.OPTION_ENCODING, StandardCharsets.UTF_8.name())); | ||
resource.getContents().add(modelContent); | ||
try (Writer writer = new OutputStreamWriter(out, StandardCharsets.UTF_8)) { | ||
String formatted = serializer.serialize(modelContent, SaveOptions.newBuilder().format().getOptions()); | ||
writer.write(formatted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why you need a serializer now ? Wasn't the save working for sitemap ? If not, why ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatter is not called on save if there already is a model. That is the case for sitemaps loaded from a text file, but also for sitemap programmatically created from org.openhab.core.ui. And in the second case it results in a generated DSL with the full sitemap on one line.
<dependency> | ||
<groupId>org.openhab.core.bundles</groupId> | ||
<artifactId>org.openhab.core.model.sitemap</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.openhab.core.bom</groupId> | ||
<artifactId>org.openhab.core.bom.compile-model</artifactId> | ||
<type>pom</type> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to create a dependency between YAML and the old DSL stuff.
You did that I believe just for the classes SitemapFileGenerator and AbstractSitemapFileGenerator but these classes should be moved probably into bundle org.openhab.core.ui
No idea why it was necessary to add the dependency to org.openhab.core.bom.compile-model but this will certainly also avoid that.
* @author Mark Herwege - Initial contribution | ||
*/ | ||
@NonNullByDefault | ||
public abstract class AbstractSitemapFileGenerator implements SitemapFileGenerator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should be moved into org.openhab.core.ui so that it can be used by YAML without creating a dependency to the DSL stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
org.openhab.core.ui actually depends on org.core.model.core and core.model.sitemap. So as it is now, it doesn’t change much. In principle I do agree to move them. But it should be part of an effort to create a sitemap registry that does no depend on model.sitemap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be put in org.openhab.core ? I did not check if we have already something about sitemap in that bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, there is nothing about sitemap in org.openhab.core.
That's why I got stuck with this, see: #4945 (comment)
@lolodomo I very much appreciate your detailed review of this already. I just think it is not ready yet. I will take on your comments, but I believe I need to solve another aspect first. A central sitemap registry is missing.
Sitemaps currently don't have any other central registry but in the model package. At the moment, there are only 2 providers: from the model, and from the UI. The one from the UI actually creates an in memory model directly, so it is also not a real provider mechanism like for items, things or persistence. YAML will be another provider, making things even more complex. And further to this, to be able to parse YAML sitemaps and not do the same as with the UI sitemaps (create the model), this registry would be required as well.
Ultimately, my conclusion was a registry independent of the model is required, and a started working on a separate refactoring to get there. But I got stuck and didn't have the time to finish yet. One of the challenges I created for myself was trying to remove the reliance on emf as well, but removing EObject and EList makes it very complex, requires a lot of dependent changes, and is probably not worth the effort. So my intention is to restart this while keeping emf as a dependency (and eliminating the xtend dependencies). If this central registry should sit in org.openhab.core or org.openhab.core.ui is probably a secondary issue. So far UI related elements have been kept out of core except for the sitemap model. And in the feature definitions, org.openhab.core.ui is and a second group of dependencies, so not in the core. We could move that over there, ore create something in the core itself.
I would appreciate your feedback on my architectural reasoning. If you agree, should I first create another PR creating this sitemap registry and rewire everything to work with that? This one would then be rebased on that when that has been finished and review. Or should I try to do all in one big PR (this one)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @author Mark Herwege - Initial contribution | ||
*/ | ||
@NonNullByDefault | ||
public interface SitemapFileGenerator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should be moved into org.openhab.core.ui so that it can be used by YAML without creating a dependency to the DSL stuff.
<dependency> | ||
<groupId>org.openhab.core.bundles</groupId> | ||
<artifactId>org.openhab.core.model.core</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.openhab.core.bundles</groupId> | ||
<artifactId>org.openhab.core.model.sitemap</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.openhab.core.bom</groupId> | ||
<artifactId>org.openhab.core.bom.compile-model</artifactId> | ||
<type>pom</type> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the move of the 2 classes SitemapFileGenerator and AbstractSitemapFileGenerator in org.openhab.core.ui, just adding a dependency to org.openhab.core.ui should be sufficient.
private static final String DSL_SITEMAPS_EXAMPLE = """ | ||
sitemap MySitemap label="My Sitemap" { | ||
Frame { | ||
Input item=MyItem label="My Input" | ||
} | ||
} | ||
"""; | ||
|
||
private static final String YAML_SITEMAPS_EXAMPLE = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide the same example in both formats.
"""; | ||
|
||
private static final String YAML_SITEMAPS_EXAMPLE = """ | ||
version: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version should be 1.
I see it was kept to 2 for other examples in that file, that is to be fixed in a separate PR so that it can be easily backported to 5.0.x. I will create it.
return Response.status(Response.Status.NOT_FOUND) | ||
.entity("Sitemap with UID '" + sitemapUID + "' does not exist!").build(); | ||
} | ||
List<Sitemap> sitemaps = sitemapNames.stream().sorted().map(name -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid sorting the names so that that order provided as input is kept.
In case no input is provided, the sitemaps are already sorted by name at line 378.
@YamlElementName("sitemap") | ||
public class YamlSitemapDTO implements YamlElement, Cloneable { | ||
|
||
public String uid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"name" would be better, no ?
public String uid; | ||
public String label; | ||
public String icon; | ||
public List<Map.Entry<String, YamlWidgetDTO>> widgets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried to use map but in this case we have no identifier for each widget so th elist seems to appropriate.
|
||
@Override | ||
public boolean isValid(@Nullable List<@NonNull String> errors, @Nullable List<@NonNull String> warnings) { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is of course to be implemented.
return false; | ||
} | ||
YamlSitemapDTO other = (YamlSitemapDTO) obj; | ||
return Objects.equals(uid, other.uid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is of course to be enhanced to cover the different fields.
@YamlElementName("mapping") | ||
public class YamlMappingDTO implements YamlElement, Cloneable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to implement YamlElement and to use @YamlElementName
@YamlElementName("rule") | ||
public class YamlRuleDTO implements YamlElement, Cloneable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to implement YamlElement and to use @YamlElementName
@YamlElementName("widget") | ||
public class YamlWidgetDTO implements YamlElement, Cloneable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to implement YamlElement and to use @YamlElementName
This change hardly adds code to core, but I think it would be good, if this is converted to an Add-On. In fact I think YAML handling of Items and Things should also be Add-On, and Xtext reading of Items/Things/Sitemaps should be Add-On, too. The reason is that users will be able to install only what they need, keeping installation size, initialization after MainUI already depends on Add-Ons to be fully functional (Blockly/JavaScript), so MainUI could handle different formats for the same data only when the Add-Ons are installed: not an obstacle. |
In OH 5.0, DSL serialization for items and things was introduced.
This PR extends this to serialize sitemap definitions into DSL and YAML. This is a basis to be able to remove the specific sitemap parser from the UI in the future, and use a REST call instead.
To make this work for DSL, I had to do a change that has a broader impact. The current sitemap syntax contains a number of unnamed lists, such as:
These do not serialize well and a separate
labelcolor=[...]
entry is created for each definition. I tried getting around this by writing a custom serializer but did not succeed. I therefore modified the grammar to explicitely name these lists, like:and
This does not change the syntax from a user perspective, but it does introduce an extra named level in the created Java objects. All calls to retrieve an array in the code needed to be adapted. This PR already includes the required core changes. I have the code to also adjust BasicUI and CometVisu. With this grammar change, the sitemap grammar serializes without issues.
The grammar was too big with the above change. To resolve that, I moved the variable sequence of unique properties out of the syntax and built extra validation checks to cope with this. XText is apparantly very bad at these variable sequences (using the & symbol). It also had a lot of unexpected consequences in the formatting of the result.
I now also force the use of the serializer, so sitemaps created programatically and not from a file get formatted properly. save does not always call the formatter. This had impact on the items and things serialization as well, requiring an extra parameter.
@lolodomo What do you think?