Sitemap DSL serialization and parsing#5459
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Adds infrastructure to parse/serialize sitemap DSL to/from a new sitemap DTO format, plus a temporary translation layer to/from the UIComponent sitemap representation, and exposes sitemap conversion via the existing file-format REST resource.
Changes:
- Introduces sitemap DTOs + mappers and sitemap parser/serializer SPI (
SitemapParser/SitemapSerializer). - Implements DSL sitemap parsing/serialization via a new
DslSitemapConverterand updates the DSL provider behavior to support isolated parsing. - Extends the file-format REST API to create/parse
text/vnd.openhab.dsl.sitemapand wires UIComponent sitemaps as a managed provider.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java | Makes UIComponent sitemap provider managed (add/update/remove/get). |
| bundles/org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapMapper.java | Adds mapper between Sitemap model and UIComponent representation. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/fileconverter/SitemapSerializer.java | Adds sitemap serializer SPI. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/fileconverter/SitemapParser.java | Adds sitemap parser SPI. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/fileconverter/AbstractSitemapSerializer.java | Base class for sitemap serializers. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/AbstractSitemapDTO.java | Adds base sitemap DTO used across REST/file-format conversion. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTO.java | Adds sitemap definition DTO including widgets list. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/AbstractWidgetDTO.java | Adds shared widget DTO fields for reuse. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/WidgetDTO.java | Adds widget definition DTO including rules/buttons/children. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/RuleDTO.java | Adds rule DTO (conditions + argument). |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/ConditionDTO.java | Adds condition DTO. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/MappingDTO.java | Moves/defines mapping DTO in sitemap bundle. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/ButtonDefinitionDTO.java | Adds buttongrid button definition DTO. |
| bundles/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java | Adds bidirectional mapping between model Sitemap and DTOs. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java | Implements DSL sitemap parsing/serialization using ModelRepository. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/DslSitemapProvider.java | Refactors DSL sitemap provider caching/notifications for model-based + isolated parsing use. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/formatting/SitemapIndentationInformation.java | Sets DSL formatter indentation to 2 spaces. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/formatting/SitemapFormatter.xtend | Adds formatting rules for sitemap DSL output. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/SitemapStandaloneSetup.xtend | Caches Xtext injector to reuse setup. |
| bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/SitemapRuntimeModule.xtend | Binds formatter + indentation info in runtime module. |
| bundles/org.openhab.core.model.sitemap/bnd.bnd | Adds sitemap fileconverter package import. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/WidgetDTO.java | Reuses shared widget DTO fields by extending AbstractWidgetDTO. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapDTO.java | Reuses shared sitemap DTO fields by extending AbstractSitemapDTO. |
| bundles/org.openhab.core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java | Adjusts mapping handling to account for DTO field refactor. |
| bundles/org.openhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/fileformat/FileFormatResource.java | Adds sitemap DSL create/parse endpoints and registry-based sitemap serialization. |
| bundles/org.openhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/fileformat/FileFormatDTO.java | Extends file-format JSON DTO to include sitemaps. |
| bundles/org.openhab.core.io.rest.core/pom.xml | Adds dependency on org.openhab.core.sitemap for DTOs/mappers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...hab.core.sitemap/src/main/java/org/openhab/core/sitemap/fileconverter/SitemapSerializer.java
Outdated
Show resolved
Hide resolved
....core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapMapper.java
Outdated
Show resolved
Hide resolved
...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java
Outdated
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...hab.core.sitemap/src/main/java/org/openhab/core/sitemap/fileconverter/SitemapSerializer.java
Show resolved
Hide resolved
...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java
Show resolved
Hide resolved
....core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapMapper.java
Outdated
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/openhab/core/io/rest/core/internal/fileformat/FileFormatResource.java
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
bundles/org.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/DslSitemapProvider.java:403
modelChangednow parses and caches all.sitemapmodels, including isolated models created viaModelRepository.createIsolatedModel(...)(names start withModelCoreConstants.PREFIX_TMP_MODEL). BecausegetAll()/getSitemapNames()/getSitemap()read fromsitemapCache, isolated parses can temporarily leak into theSitemapRegistry, violatingSitemapParser's contract (“without impacting the sitemap registry”) and potentially shadowing real sitemaps with the same name. Exclude isolated models from registry-visible APIs and from listener notifications (or keep a separate cache for isolated models only used bygetAllFromModel).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Outdated
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/openhab/core/io/rest/core/internal/fileformat/FileFormatResource.java
Outdated
Show resolved
Hide resolved
...ore.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapProvider.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/openhab/core/io/rest/core/internal/fileformat/FileFormatResource.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/openhab/core/io/rest/core/internal/fileformat/FileFormatResource.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Outdated
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Outdated
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Outdated
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Outdated
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Show resolved
Hide resolved
...io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/SitemapResource.java
Outdated
Show resolved
Hide resolved
....core.io.rest.sitemap/src/main/java/org/openhab/core/io/rest/sitemap/internal/WidgetDTO.java
Show resolved
Hide resolved
...nhab.core.model.sitemap/src/org/openhab/core/model/sitemap/formatting/SitemapFormatter.xtend
Outdated
Show resolved
Hide resolved
...nhab.core.model.sitemap/src/org/openhab/core/model/sitemap/formatting/SitemapFormatter.xtend
Outdated
Show resolved
Hide resolved
...del.sitemap/src/org/openhab/core/model/sitemap/formatting/SitemapIndentationInformation.java
Outdated
Show resolved
Hide resolved
...g.openhab.core.model.sitemap/src/org/openhab/core/model/sitemap/SitemapStandaloneSetup.xtend
Outdated
Show resolved
Hide resolved
...enhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/DslSitemapProvider.java
Outdated
Show resolved
Hide resolved
...nhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/SitemapProviderImpl.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Outdated
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...enhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/DslSitemapProvider.java
Show resolved
Hide resolved
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>
|
@mherwege : I cannot mark comments as resolved, can you please do it for all those with my comment "OK" or "Done" ? |
@lolodomo Done. The code is now working as expected as far as I can see. The new sitemap endpoints allow adding, removing and updating managed sitemaps and update the UIComponentRegistry correctly, so are visible as managed sitemaps in the UI. Unmanaged sitemaps are still not visible, but that would be a next step in the UI code. I will go through the code one more time and have copilot do one or more reviews before I take this out of Draft. |
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Outdated
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
...l.sitemap/src/org/openhab/core/model/sitemap/internal/fileconverter/DslSitemapConverter.java
Show resolved
Hide resolved
lolodomo
left a comment
There was a problem hiding this comment.
Review of core.sitemap bundle
....sitemap/src/main/java/org/openhab/core/sitemap/fileconverter/AbstractSitemapSerializer.java
Outdated
Show resolved
Hide resolved
...org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/ButtonDefinitionDTO.java
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/internal/SitemapImpl.java
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Outdated
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Show resolved
Hide resolved
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>
|
@lolodomo I believe this is all good now. copilot is not able to come up with meaningful remarks anymore, so from my perspective this is ready. |
...enhab.core.model.sitemap/src/org/openhab/core/model/sitemap/internal/DslSitemapProvider.java
Outdated
Show resolved
Hide resolved
...es/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDTOMapper.java
Outdated
Show resolved
Hide resolved
....core.ui/src/main/java/org/openhab/core/ui/internal/components/UIComponentSitemapMapper.java
Show resolved
Hide resolved
Still few things to adjust. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@lolodomo I made a few changes, but also commented on some of your remarks where I believe there is no issue. Can you check again? Thanks for all your effort reviewing this. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
...rg.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDefinitionDTO.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
...s/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/AbstractWidgetDTO.java
Outdated
Show resolved
Hide resolved
...rg.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/SitemapDefinitionDTO.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
...s/org.openhab.core.sitemap/src/main/java/org/openhab/core/sitemap/dto/AbstractWidgetDTO.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@openhab/core-maintainers : could we have your attention please ? This is the second step (after the sitemap registry) and we are already working on next steps (Mark to update Main UI and finally expose new features to users and myself to add the new YAML format). |
This PR builds on openhab#5459 and partially replaces openhab#4945 (covering YAML). It also makes further steps towards openhab#5007 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
This PR creates a new YAML format for sitemaps. This PR builds on openhab#5459 and partially replaces openhab#4945 (covering YAML). It also makes further steps towards openhab#5007 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
|
@mherwege : can you please fix the signature of this method, @nullable is missing for the parameter (while present in the implementation) Edit: For all, it is not a requirement for a change in that PR, it can be fixed in a separate PR. I can even fix it in my own PR. What I mean is that it should not delay the merge. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@lolodomo I pushed the fix here. There is another thing I was thinking of. The validation checks for minValue < maxValue only work if both are set. The documentation says if these are not set, they will default to 0 and 100. This does not happen in the sitemap model or core sitemap code, so I assume this happens when rendering. |
This PR creates a new YAML format for sitemaps. This PR builds on openhab#5459 and partially replaces openhab#4945 (covering YAML). It also makes further steps towards openhab#5007 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
|
@lolodomo I have another question about |
This PR creates a new YAML format for sitemaps. This PR builds on openhab#5459 and partially replaces openhab#4945 (covering YAML). It also makes further steps towards openhab#5007 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
|
@holgerfriedrich @kaikreuzer : is it possible to merge that PR so that we can make progress ? |
This PR creates a new YAML format for sitemaps. This PR builds on openhab#5459 and partially replaces openhab#4945 (covering YAML). It also makes further steps towards openhab#5007 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
This PR creates a new YAML format for sitemaps. This PR builds on openhab#5459 and partially replaces openhab#4945 (covering YAML). It also makes further steps towards openhab#5007 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
This PR builds on #5004 and partially replaces #4945 (YAML is not covered).
It also makes further steps towards #5007
I see the following steps to take:
What this will give us is a full set of new REST endpoints to manage sitemaps in the new registry format without immediately impacting the UI.
As next steps (separate PR's) I see:
I have not decided on 5 yet, but I see 4 and 5 as next steps after this PR (and a YAML parsing/serialization PR).
@lolodomo This first commit in this PR is the parsing/serialization part. But as I pointed out in the other PR, without the extra work, it is still somewhat limited in its use. The REST endpoints work, but the DTO is not understood by the rest of the sitemap REST endpoints.