Conversation
giswqs
commented
Nov 20, 2025
|
🚀 Deployed on https://691e9937ab21a6cdc944cc72--opengeos.netlify.app |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds custom styling capabilities for Geoman-managed features by introducing a mirrored read-only GeoJSON layer that visualizes geoman_data with user-defined paint properties.
- Adds
paintandpaint_above_geomanparameters toadd_geoman_control()method - Implements JavaScript infrastructure to create and maintain synchronized style layers
- Enables data-driven styling expressions for geoman features (e.g., styling by feature properties)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| anymap/maplibre.py | Adds paint and paint_above_geoman parameters to the add_geoman_control method with documentation |
| anymap/static/maplibre_widget.js | Implements mirrored style layer system that syncs with geoman_data, including layer creation, data updates, and placement logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const layers = (map.getStyle() && map.getStyle().layers) || []; | ||
| for (const ly of layers) { | ||
| const id = (ly && ly.id) || ''; | ||
| if (id && (id.startsWith('gm-') || id.includes('geoman') || id.includes('gm'))) { |
There was a problem hiding this comment.
The layer detection logic for finding Geoman layers uses a broad heuristic (id.includes('geoman') || id.includes('gm')). This could potentially match non-Geoman layers that happen to contain 'gm' or 'geoman' in their IDs.
Consider making the detection more specific by:
- Checking for an exact prefix match only (e.g.,
id.startsWith('gm-')orid.startsWith('geoman-')) - Maintaining a registry of actual Geoman layer IDs when they're created
- Checking for a specific attribute that marks Geoman layers
Example improvement:
// More specific prefix check
if (id && (id.startsWith('gm-') || id.startsWith('geoman-'))) {
beforeId = id;
break;
}| if (id && (id.startsWith('gm-') || id.includes('geoman') || id.includes('gm'))) { | |
| if (id && (id.startsWith('gm-') || id.startsWith('geoman-'))) { |
| try { | ||
| if (el._geomanStyle && map.getSource(el._geomanStyle.srcId)) { | ||
| map.getSource(el._geomanStyle.srcId).setData(collection || { type: 'FeatureCollection', features: [] }); | ||
| } | ||
| } catch (_e) {} |
There was a problem hiding this comment.
This is a duplicate of the code at lines 3924-3929. The mirrored style source update logic should be refactored to use the refreshGeomanStyleLayers function instead of being duplicated. See comment on lines 3924-3929 for the refactoring suggestion.
| try { | |
| if (el._geomanStyle && map.getSource(el._geomanStyle.srcId)) { | |
| map.getSource(el._geomanStyle.srcId).setData(collection || { type: 'FeatureCollection', features: [] }); | |
| } | |
| } catch (_e) {} | |
| refreshGeomanStyleLayers(map, el, collection); |
| const ensureGeomanStyleLayers = (paintConfig) => { | ||
| if (!paintConfig) return; | ||
| const srcId = 'geoman-style-src'; | ||
| const fillId = 'geoman-style-fill'; | ||
| const lineId = 'geoman-style-line'; | ||
| const pointId = 'geoman-style-point'; | ||
| try { | ||
| if (!map.getSource(srcId)) { | ||
| map.addSource(srcId, { type: 'geojson', data: { type: 'FeatureCollection', features: [] } }); | ||
| } | ||
| } catch (_e) {} | ||
| // Placement: default above Geoman; if paintAbove === false, push below first geoman layer | ||
| let beforeId = undefined; | ||
| if (!paintAbove) { | ||
| try { | ||
| const layers = (map.getStyle() && map.getStyle().layers) || []; | ||
| for (const ly of layers) { | ||
| const id = (ly && ly.id) || ''; | ||
| if (id && (id.startsWith('gm-') || id.includes('geoman') || id.includes('gm'))) { | ||
| beforeId = id; | ||
| break; | ||
| } | ||
| } | ||
| } catch (_e2) {} | ||
| } | ||
| if (!map.getLayer(fillId)) { | ||
| try { | ||
| const layer = { | ||
| id: fillId, | ||
| type: 'fill', | ||
| source: srcId, | ||
| filter: ['any', | ||
| ['==', ['geometry-type'], 'Polygon'], | ||
| ['==', ['geometry-type'], 'MultiPolygon'] | ||
| ], | ||
| paint: Object.assign({ | ||
| 'fill-color': '#90caf9', | ||
| 'fill-opacity': 0.2 | ||
| }, (paintConfig.fill || {})) | ||
| }; | ||
| if (beforeId) map.addLayer(layer, beforeId); else map.addLayer(layer); | ||
| } catch (_e3) {} | ||
| } | ||
| if (!map.getLayer(lineId)) { | ||
| try { | ||
| const layer = { | ||
| id: lineId, | ||
| type: 'line', | ||
| source: srcId, | ||
| filter: ['any', | ||
| ['==', ['geometry-type'], 'LineString'], | ||
| ['==', ['geometry-type'], 'MultiLineString'] | ||
| ], | ||
| paint: Object.assign({ | ||
| 'line-color': '#42a5f5', | ||
| 'line-width': 2.5 | ||
| }, (paintConfig.line || {})) | ||
| }; | ||
| if (beforeId) map.addLayer(layer, beforeId); else map.addLayer(layer); | ||
| } catch (_e4) {} | ||
| } | ||
| if (!map.getLayer(pointId)) { | ||
| try { | ||
| const layer = { | ||
| id: pointId, | ||
| type: 'circle', | ||
| source: srcId, | ||
| filter: ['any', | ||
| ['==', ['geometry-type'], 'Point'], | ||
| ['==', ['geometry-type'], 'MultiPoint'] | ||
| ], | ||
| paint: Object.assign({ | ||
| 'circle-radius': 5, | ||
| 'circle-color': '#1976d2', | ||
| 'circle-stroke-color': '#0d47a1', | ||
| 'circle-stroke-width': 1.5 | ||
| }, (paintConfig.point || {})) | ||
| }; | ||
| if (beforeId) map.addLayer(layer, beforeId); else map.addLayer(layer); | ||
| } catch (_e5) {} | ||
| } | ||
| el._geomanStyle = { srcId, fillId, lineId, pointId }; | ||
| }; |
There was a problem hiding this comment.
[nitpick] The ensureGeomanStyleLayers function could be called multiple times, but it only checks if layers exist before creating them. Consider adding a guard to prevent multiple initialization attempts or track the initialization state more explicitly.
For example:
const ensureGeomanStyleLayers = (paintConfig) => {
if (!paintConfig || el._geomanStyleInitialized) return;
// ... rest of initialization
el._geomanStyleInitialized = true;
};This would make the initialization behavior more explicit and prevent any potential race conditions if the function is called concurrently.
| // Placement: default above Geoman; if paintAbove === false, push below first geoman layer | ||
| let beforeId = undefined; | ||
| if (!paintAbove) { | ||
| try { | ||
| const layers = (map.getStyle() && map.getStyle().layers) || []; | ||
| for (const ly of layers) { | ||
| const id = (ly && ly.id) || ''; | ||
| if (id && (id.startsWith('gm-') || id.includes('geoman') || id.includes('gm'))) { | ||
| beforeId = id; | ||
| break; | ||
| } | ||
| } | ||
| } catch (_e2) {} | ||
| } |
There was a problem hiding this comment.
The comment states "default above Geoman; if paintAbove === false, push below first geoman layer", but the logic is inverted. When paintAbove is false, the code searches for a Geoman layer to use as beforeId, which would place the style layers below (rendered first). However, when paintAbove is true (default), beforeId remains undefined, which adds layers on top (rendered last).
This appears to be working as intended based on the parameter documentation, but the comment is misleading. Consider updating the comment to:
// Placement: if paintAbove === false, insert below first geoman layer; otherwise add on top (default)This would better reflect the actual behavior.
| // Ensure mirrored style reflects current data | ||
| try { | ||
| if (stylePaint) { | ||
| ensureGeomanStyleLayers(stylePaint); | ||
| const currentData = model.get('geoman_data') || { type: 'FeatureCollection', features: [] }; | ||
| refreshGeomanStyleLayers(currentData); | ||
| } | ||
| } catch (_e) {} |
There was a problem hiding this comment.
The initialization code calls ensureGeomanStyleLayers(stylePaint) twice in quick succession (line 4859 and originally at line 4414), which is redundant. The first call at line 4414 already ensures the layers exist and are initialized. The second call will exit early due to the !map.getLayer(fillId) checks, but this still represents unnecessary work.
Consider either:
- Removing the second
ensureGeomanStyleLayerscall and only callingrefreshGeomanStyleLayers - Adding a flag to track if initialization has already happened
Suggested fix:
// Ensure mirrored style reflects current data
try {
if (stylePaint && el._geomanStyle) {
// Layers already created at line 4414, just refresh data
const currentData = model.get('geoman_data') || { type: 'FeatureCollection', features: [] };
refreshGeomanStyleLayers(currentData);
}
} catch (_e) {}Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
for more information, see https://pre-commit.ci