Implement a core sitemap registry#5004
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a core sitemap registry that decouples sitemap handling from the model packages. It creates a central registry for sitemaps with providers for different sources (DSL sitemaps and UI-created sitemaps) and introduces a new org.openhab.core.sitemap module with a factory-based approach for sitemap creation.
- Creates a central
SitemapRegistryto manage sitemaps from different providers - Introduces a new
org.openhab.core.sitemapmodule with interfaces and implementations - Refactors existing UI sitemap provider to use the new registry architecture
Reviewed Changes
Copilot reviewed 91 out of 91 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Feature/Karaf configurations | Updates feature definitions to include new sitemap module and remove model dependencies |
| Test configurations | Adds sitemap module dependency to integration test bundles |
| UI module refactoring | Updates UI components to use new sitemap interfaces instead of model classes |
| New sitemap module | Introduces core sitemap interfaces, registry, factory, and implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
There is a new package |
mherwege
force-pushed
the
sitemap_registry
branch
from
aa5094a to
af8145b
Compare
|
@mherwege : this message just to inform you that I am not ignoring your PR at all, I will review it. |
|
I started by reviewing org.openhab.core.sitemap. The only question I have is if you did not forget that it is a possible to have a default rule condition to set a particular value when none of the other conditions are valid. |
The behaviour should not have changed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 91 out of 91 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public void addModelChangeListener(ModelRepositoryChangeListener listener) { | ||
| modelChangeListeners.add(listener); | ||
| public void addProviderChangeListener(ProviderChangeListener<Sitemap> listener) { | ||
| super.addProviderChangeListener(listener); | ||
| getAll().forEach(sitemap -> { | ||
| notifyListenersAboutAddedElement(sitemap); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Overriding addProviderChangeListener and then calling notifyListenersAboutAddedElement for every existing sitemap will notify all registered listeners (not just the newly added one), causing duplicate/incorrect ADDED events. Consider removing this override, or explicitly backfilling only the newly added listener without broadcasting to others.
There was a problem hiding this comment.
Indeed not needed.
| getAll().forEach(sitemap -> { | ||
| notifyListenersAboutRemovedElement(sitemap); | ||
| }); |
There was a problem hiding this comment.
Similarly, removeProviderChangeListener currently iterates all cached sitemaps and calls notifyListenersAboutRemovedElement, which will broadcast REMOVED events to the remaining listeners whenever any listener is removed. This is incorrect provider semantics and can cause consumers (e.g., registries) to drop all sitemaps unexpectedly. Remove this override or ensure notifications are not broadcast on listener removal.
| getAll().forEach(sitemap -> { | |
| notifyListenersAboutRemovedElement(sitemap); | |
| }); |
There was a problem hiding this comment.
Indeed should not be there.
| private void refreshSitemapModels() { | ||
| sitemapCache.clear(); | ||
| Iterable<String> sitemapFilenames = modelRepo.getAllModelNamesOfType(SITEMAP_MODEL_NAME); | ||
| for (String filename : sitemapFilenames) { | ||
| ModelSitemap modelSitemap = (ModelSitemap) modelRepo.getModel(filename); | ||
| if (modelSitemap != null) { | ||
| String sitemapName = filename.substring(0, filename.length() - SITEMAP_FILEEXT.length()); | ||
| if (!modelSitemap.getName().equals(sitemapName)) { | ||
| logger.warn( | ||
| "Filename `{}` does not match the name `{}` of the sitemap - please fix this as you might see unexpected behavior otherwise.", | ||
| filename, modelSitemap.getName()); | ||
| } | ||
| Sitemap sitemap = parseModelSitemap(modelSitemap); | ||
| sitemapCache.put(sitemapName, sitemap); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private Sitemap parseModelSitemap(ModelSitemap modelSitemap) { | ||
| Sitemap sitemap = sitemapFactory.createSitemap(modelSitemap.getName()); | ||
| sitemap.setLabel(modelSitemap.getLabel()); | ||
| sitemap.setIcon(modelSitemap.getIcon()); | ||
| List<Widget> widgets = sitemap.getWidgets(); | ||
| modelSitemap.getChildren().forEach(child -> addWidget(widgets, child, sitemap)); | ||
| return sitemap; |
There was a problem hiding this comment.
refreshSitemapModels caches sitemaps under a key derived from the filename, but parseModelSitemap creates the Sitemap using modelSitemap.getName() as its UID/name. The registry indexes by Sitemap.getUID()/name, so a mismatch between filename and sitemap name will make sitemapRegistry.get(<filename>) fail and also makes oldSitemap = sitemapRegistry.get(sitemapName) unreliable. Consider normalizing so the cached key and the created sitemap UID match (e.g., create the sitemap with sitemapName, or cache/index by modelSitemap.getName() and treat mismatch as an error/ignore).
There was a problem hiding this comment.
I will reject if filename and sitemap name do not match. It creates potential problems downstream anyway when modifying the sitemap. This is not new though.
There was a problem hiding this comment.
I will reject if filename and sitemap name do not match. It creates potential problems downstream anyway when modifying the sitemap. This is not new though.
There was a problem hiding this comment.
I never understood why we have this constraint. That means we can have only one sitemap per file.
Probably something to improve in a next PR.
| def void checkDuplicates(ModelWidget w) { | ||
| val seen = new HashSet<String> | ||
| val duplicates = new HashSet<String> | ||
| val node = NodeModelUtils.getNode(w) | ||
| val line = node.startLine | ||
|
|
||
| for (leaf : node.leafNodes) { | ||
| val text = leaf.text.trim | ||
| if (KEYWORDS.contains(text) && !seen.add(text)) { | ||
| duplicates.add(text) | ||
| } | ||
| } | ||
|
|
||
| duplicates.forEach[attr | | ||
| val cleanAttr = attr.replaceAll("=$", "") | ||
| warning(errorString(getWidgetType(w) + " widget, attribute '" + cleanAttr + "' must not appear more than once", line), null) | ||
| ] | ||
| } |
There was a problem hiding this comment.
checkDuplicates computes duplicates but is neither annotated with @Check nor called from any @Check method, so it will never run. Either add @Check (and an appropriate parameter type) or invoke it from other checks so duplicate-attribute warnings are actually emitted.
| @Check | ||
| def void checkFramesInFrame(Frame frame) { | ||
| for (Widget w : frame.children) { | ||
| if (w instanceof Frame) { | ||
| warning("Frames must not contain other frames", | ||
| SitemapPackage.Literals.FRAME.getEStructuralFeature(SitemapPackage.FRAME__CHILDREN)); | ||
| return; | ||
| def void checkWidgetHasItem(ModelWidget w) { | ||
| if (!(w instanceof ModelFrame || w instanceof ModelText || w instanceof ModelImage || w instanceof ModelVideo || w instanceof ModelWebview || w instanceof ModelButtongrid) && w.item === null) { | ||
| val node = NodeModelUtils.getNode(w) | ||
| val line = node.startLine | ||
| error(errorString(getWidgetType(w) + " widget doesn't have item defined", line), | ||
| SitemapPackage.Literals.MODEL_WIDGET.getEStructuralFeature(SitemapPackage.MODEL_WIDGET__ITEM)) | ||
| } |
There was a problem hiding this comment.
checkWidgetHasItem currently enforces item for most widgets but does not exempt ModelButton. In the grammar, ModelButton can omit item=, and buttons are typically used inside a Buttongrid that already carries the item. As-is, valid Buttongrid/Button usages without per-button items will become validation errors. Consider exempting ModelButton here (and relying on Buttongrid-level validation), or otherwise clarifying/enforcing the intended item semantics for buttons.
There was a problem hiding this comment.
This is not correct. If the Button Widget is used, it does require a item defined on the Button. Button Widgets are always part of a Buttongrid. When Button Widgets are used the Buttongrid does not need to define an item (it will be ignored). The item on the Buttongrid is only relevant if the buttons attribute on the Buttongrid is used.
There was a problem hiding this comment.
I have a doubt, I will check that.
There was a problem hiding this comment.
I have a doubt, I will check that.
In the current sitemap syntax, it is required:
You could argue it could default to the Buttongrid item, but I don't think that is what happens today.
| warning(errorString(className + "widget has icon '" + w.icon + "' and staticIcon '" + w.staticIcon + "' defined at the same time", line), null) | ||
| } | ||
| if (w.iconRules !== null && w.staticIcon !== null) { | ||
| val node = NodeModelUtils.getNode(w) | ||
| val line = node.startLine | ||
| val iconRules = NodeModelUtils.getTokenText(NodeModelUtils.getNode(w.iconRules)) | ||
| warning(errorString(className + "widget has icon rules '" + iconRules + "' and staticIcon '" + w.staticIcon + "' defined at the same time", line), null) |
There was a problem hiding this comment.
The warning messages in checkWidgetIcon concatenate className + "widget", producing messages like Textwidget ... (missing a space). Adjust the string to include a space for readability.
| warning(errorString(className + "widget has icon '" + w.icon + "' and staticIcon '" + w.staticIcon + "' defined at the same time", line), null) | |
| } | |
| if (w.iconRules !== null && w.staticIcon !== null) { | |
| val node = NodeModelUtils.getNode(w) | |
| val line = node.startLine | |
| val iconRules = NodeModelUtils.getTokenText(NodeModelUtils.getNode(w.iconRules)) | |
| warning(errorString(className + "widget has icon rules '" + iconRules + "' and staticIcon '" + w.staticIcon + "' defined at the same time", line), null) | |
| warning(errorString(className + " widget has icon '" + w.icon + "' and staticIcon '" + w.staticIcon + "' defined at the same time", line), null) | |
| } | |
| if (w.iconRules !== null && w.staticIcon !== null) { | |
| val node = NodeModelUtils.getNode(w) | |
| val line = node.startLine | |
| val iconRules = NodeModelUtils.getTokenText(NodeModelUtils.getNode(w.iconRules)) | |
| warning(errorString(className + " widget has icon rules '" + iconRules + "' and staticIcon '" + w.staticIcon + "' defined at the same time", line), null) |
|
Huge thanks @mherwege for this extensive PR and many thanks @lolodomo for your detailed review since the start! I had a glance over it and the code looks really good. I just ran copilot for a last time and I think it has some relevant findings - would be great if you could look at them. We should then be fine to merge after the next milestone build. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 91 out of 91 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Sitemap sitemap = null; | ||
| String sitemapFileName = modelName.substring(0, modelName.length() - SITEMAP_FILEEXT.length()); | ||
| Sitemap oldSitemap = sitemapRegistry.get(sitemapFileName); | ||
|
|
||
| if (type == EventType.REMOVED) { | ||
| sitemapCache.remove(sitemapFileName); | ||
| } else { | ||
| EObject modelSitemapObject = modelRepo.getModel(modelName); | ||
| // if the sitemap file is empty it will not be in the repo and thus there is no need to cache it here | ||
| if (modelSitemapObject instanceof ModelSitemap modelSitemap) { | ||
| String sitemapName = modelSitemap.getName(); | ||
| if (!sitemapFileName.equals(sitemapName)) { | ||
| logger.warn("Filename '{}' does not match the name '{}' of the sitemap - ignoring sitemap.", | ||
| sitemapFileName, sitemapName); | ||
| sitemapCache.remove(sitemapFileName); | ||
| } else { | ||
| sitemap = parseModelSitemap(modelSitemap); | ||
| sitemapCache.put(sitemapName, sitemap); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| switch (type) { | ||
| case EventType.ADDED: | ||
| if (sitemap != null) { | ||
| notifyListenersAboutAddedElement(sitemap); | ||
| } | ||
| break; | ||
| case EventType.REMOVED: | ||
| if (oldSitemap != null) { | ||
| notifyListenersAboutRemovedElement(oldSitemap); | ||
| } | ||
| break; | ||
| case EventType.MODIFIED: | ||
| if (sitemap != null && oldSitemap != null) { | ||
| notifyListenersAboutUpdatedElement(oldSitemap, sitemap); | ||
| } | ||
| break; |
There was a problem hiding this comment.
modelChanged only emits UPDATED events when both oldSitemap and the newly parsed sitemap are non-null. If a previously valid sitemap becomes invalid/empty or its internal name no longer matches the filename, the provider removes it from sitemapCache but does not emit a REMOVED event (and the registry will keep serving the stale sitemap). Similarly, if a previously invalid sitemap becomes valid again, no ADDED event is sent on MODIFIED. Consider deriving oldSitemap from the provider cache, and in the MODIFIED case emit REMOVED when oldSitemap != null && sitemap == null, emit ADDED when oldSitemap == null && sitemap != null, otherwise UPDATED when both are non-null.
There was a problem hiding this comment.
This can indeed be improved further.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 91 out of 91 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| org.openhab.core.semantics;version='[5.2.0,5.2.1)', \ | ||
| org.openhab.core.sitemap;version='[5.2.0,5.2.1)' |
There was a problem hiding this comment.
In this bndrun snippet the new entry for org.openhab.core.sitemap is indented with spaces instead of the tab-indentation used for the other -runbundles entries. Keeping the same indentation style improves readability and helps avoid subtle formatting/parsing issues in long continued properties.
| Sitemap oldSitemap = sitemapCache.put(sitemapName, sitemap); | ||
| if (oldSitemap != null) { | ||
| notifyListenersAboutUpdatedElement(oldSitemap, sitemap); | ||
| } else { | ||
| notifyListenersAboutAddedElement(sitemap); | ||
| } | ||
| } |
There was a problem hiding this comment.
ModelRepository#getModel returns null when the DSL file is empty or cannot be parsed. In the MODIFIED/ADDED paths, if getModel(modelName) returns null (or not a ModelSitemap), the old sitemap remains in sitemapCache and listeners are not notified, so the registry/REST can continue serving a stale sitemap. Consider removing any cached entry for that filename and emitting a REMOVED notification when the refreshed model cannot be obtained/parsed.
| Sitemap oldSitemap = sitemapCache.put(sitemapName, sitemap); | |
| if (oldSitemap != null) { | |
| notifyListenersAboutUpdatedElement(oldSitemap, sitemap); | |
| } else { | |
| notifyListenersAboutAddedElement(sitemap); | |
| } | |
| } | |
| if (sitemap != null) { | |
| Sitemap oldSitemap = sitemapCache.put(sitemapName, sitemap); | |
| if (oldSitemap != null) { | |
| notifyListenersAboutUpdatedElement(oldSitemap, sitemap); | |
| } else { | |
| notifyListenersAboutAddedElement(sitemap); | |
| } | |
| } else { | |
| // Parsing failed, remove any previously cached sitemap and notify listeners | |
| Sitemap oldSitemap = sitemapCache.remove(sitemapFileName); | |
| if (oldSitemap != null) { | |
| notifyListenersAboutRemovedElement(oldSitemap); | |
| } | |
| } | |
| } | |
| } else { | |
| // Model could not be obtained or is not a ModelSitemap, remove any cached sitemap | |
| Sitemap oldSitemap = sitemapCache.remove(sitemapFileName); | |
| if (oldSitemap != null) { | |
| notifyListenersAboutRemovedElement(oldSitemap); | |
| } |
| break; | ||
| case Video videoWidget: | ||
| ModelVideo modelVideo = (ModelVideo) modelWidget; | ||
| videoWidget.setUrl(modelVideo.getUrl()); |
There was a problem hiding this comment.
ModelVideo#url is optional in the DSL grammar, but this code passes it directly into Video#setUrl(String). If the model URL is absent, this can assign null into a field that is treated as non-null by the core sitemap API and downstream code. Consider guarding against null (e.g., keep the default/empty value) or only calling setUrl when a URL is actually present.
| videoWidget.setUrl(modelVideo.getUrl()); | |
| String videoUrl = modelVideo.getUrl(); | |
| if (videoUrl != null) { | |
| videoWidget.setUrl(videoUrl); | |
| } |
There was a problem hiding this comment.
Video url is not optional. It is checked in the sitemap validation, although it is only creating a warning. setUrl does accept null values and treats them properly.
| case Chart chartWidget: | ||
| ModelChart modelChart = (ModelChart) modelWidget; | ||
| chartWidget.setService(modelChart.getService()); | ||
| chartWidget.setRefresh(modelChart.getRefresh()); | ||
| chartWidget.setPeriod(modelChart.getPeriod()); | ||
| chartWidget.setLegend(modelChart.getLegend()); | ||
| chartWidget.setForceAsItem(modelChart.getForceAsItem()); | ||
| chartWidget.setYAxisDecimalPattern(modelChart.getYAxisDecimalPattern()); | ||
| chartWidget.setInterpolation(modelChart.getInterpolation()); | ||
| break; |
There was a problem hiding this comment.
ModelChart#period is optional in the DSL grammar, but the core sitemap Chart API treats period as a non-null string. Passing a missing period through here can result in null/invalid chart widgets later. Consider applying a default period (if the DSL has a default) or skipping/flagging the widget when period is absent.
There was a problem hiding this comment.
Webview period is not optional. It is checked in the sitemap validation. See above.
| case Webview webviewWidget: | ||
| ModelWebview modelWebview = (ModelWebview) modelWidget; | ||
| webviewWidget.setHeight(modelWebview.getHeight()); | ||
| webviewWidget.setUrl(modelWebview.getUrl()); |
There was a problem hiding this comment.
ModelWebview#url is optional in the DSL grammar, but this code passes it into Webview#setUrl(String). If missing, this can propagate a null/invalid URL into the core sitemap representation and later REST/UI code. Consider guarding against null (e.g., keep default/empty) or rejecting/skipping invalid widgets.
| webviewWidget.setUrl(modelWebview.getUrl()); | |
| String url = modelWebview.getUrl(); | |
| if (url != null && !url.isEmpty()) { | |
| webviewWidget.setUrl(url); | |
| } |
There was a problem hiding this comment.
Webview url is not optional. It is checked in the sitemap validation. See above.
| EList<ModelIconRule> modelIconRules = modelIconRuleList.getElements(); | ||
| modelIconRules.forEach(modelIconRule -> { | ||
| Rule iconRule = sitemapFactory.createRule(); | ||
| iconRule.setArgument(modelIconRule.getArg()); |
There was a problem hiding this comment.
addWidgetIconRules sets the rule argument twice (setArgument(modelIconRule.getArg()) before and after copying conditions). This is redundant and makes the method harder to read/maintain; keep a single assignment.
| iconRule.setArgument(modelIconRule.getArg()); |
| public void addModelChangeListener(ModelRepositoryChangeListener listener) { | ||
| modelChangeListeners.add(listener); | ||
| public Set<String> getSitemapNames() { | ||
| return sitemapCache.keySet(); |
There was a problem hiding this comment.
getSitemapNames() returns sitemapCache.keySet(), which is a live view backed by the underlying ConcurrentHashMap and supports removal. Exposing this allows external callers to mutate the provider cache without going through provider notifications. Consider returning an immutable copy (e.g., Set.copyOf(...)).
| return sitemapCache.keySet(); | |
| return Set.copyOf(sitemapCache.keySet()); |
There was a problem hiding this comment.
Sure, but not sure it is worth the copy overhead.
| public void removeModelChangeListener(ModelRepositoryChangeListener listener) { | ||
| modelChangeListeners.remove(listener); | ||
| public Collection<Sitemap> getAll() { | ||
| return sitemapCache.values(); |
There was a problem hiding this comment.
getAll() returns sitemapCache.values(), which is a live, mutable view of the underlying map. Since registries and other consumers may assume Provider#getAll() returns a stable snapshot, consider returning an immutable copy to prevent accidental external mutation and avoid concurrent-modification surprises.
| return sitemapCache.values(); | |
| return List.copyOf(sitemapCache.values()); |
There was a problem hiding this comment.
Sure, but not sure it is worth the copy overhead.
| @Override | ||
| public String[] getSupportedWidgetTypes() { | ||
| return WIDGET_TYPES; | ||
| } |
There was a problem hiding this comment.
getSupportedWidgetTypes() returns the internal WIDGET_TYPES array directly. Since arrays are mutable, callers can accidentally modify the factory’s internal list of supported types. Consider returning a defensive copy (e.g., WIDGET_TYPES.clone()).
There was a problem hiding this comment.
Sure, but not sure it is worth the copy overhead.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 91 out of 91 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@kaikreuzer @lolodomo There were indeed some relevant copilot remarks. I made adjustmets accordingly (apart from the last 3 remarks, tell me if I should adjust that, I don't think this is different from before). |
kaikreuzer
left a comment
kaikreuzer
left a comment
There was a problem hiding this comment.
Thanks you so much, @mherwege!
Since we released milestone 2 today, we can now merge this PR, start testing and building on it!
Depends on openhab/openhab-core#5004 This is required if the core PR would be merged, or BasicUI and CometVisu will not work anymore. I have tested with BasicUI. I don't use CometVisu but have done similar changes as for BasicUI. --------- Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@mherwege : I am just finishing to read the last Copilot suggestions just before the merge and I believe the suggestion that modelVideo.getUrl() and modelWebview.getUrl() should be checked against null was a good suggestion , because the validation check is just a warning, not an error, so videoWidget.setUrl(url); and webviewWidget.setUrl(url); will be called with null and this is not what is expected. |
@lolodomo You are right. I can easily make the validation check an error, but that will make the whole sitemap fail. It would have failed before as well. Shall I include that in the Button validation PR, extending it to validation improvements? |
|
Before your change, the sitemap will not have loaded as the field was mandatory in the syntax. |
With this PR, a central sitemap registry is created. This registry has providers that offer sitemaps provide sitemaps from different providers. The current ones are DSL sitemaps and UI created sitemaps. The code for these has been refactored to implement such providers.
The reasons for doing this are:
org.openhab.core.model.sitemap, soorg.openhab.core.uidepends on this model package.With the current structure, whatever we do when creating a new sitemap provider forces pulling the
org.openhab.core.model.sitemappackage directly or indirectly.It therefore seems more appropriate to decouple the internal sitemap representation from the DSL model and xtext code generation.
See for the background: #4945 (comment) and #4945 (comment)
This PR prepares for:
This PR is accompanied by the webui PR (openhab/openhab-webui#3349) that adapts the dependencies for BasicUI and CometVisu. Other UI's should not be concerned as they all work purely through the REST API and SSE events.
This is of course a major refactoring. Therefore there is a high risk and probability of some regressions and needs to be tested extensively. Code tests run through and my tests have not revealed issues so far. But I do expect there may be some.
@lolodomo I believe this is a better basis to continue work on YAML and DSL conversion afterwards.