Conversation
|
🚀 Deployed on https://690e1e979b5fa997d7da26d3--opengeos.netlify.app |
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the Geoman drawing control integration by improving feature synchronization between JavaScript and Python, and adding example usage documentation. The changes focus on preventing circular update loops and supporting multiple Geoman API versions.
Key changes:
- Improved bidirectional sync between JavaScript and Python for Geoman features with change detection to prevent feedback loops
- Added comprehensive notebook examples demonstrating how to get, set, and clear drawn features
- Enhanced feature import logic with support for multiple Geoman API versions and fallback mechanisms
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| docs/examples/maplibre_geoman.ipynb | Added documentation and code examples for managing drawn features (get, set, clear operations) |
| anymap/static/maplibre_widget.js | Enhanced Geoman data synchronization with change detection, multi-source support, and improved import/export logic to prevent circular updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // Data changed from Python even though we just exported - process it | ||
| el._geomanSyncFromJs = false; | ||
| } |
There was a problem hiding this comment.
The logic in the else block (lines 4096-4099) sets el._geomanSyncFromJs = false but then proceeds to import the data. However, this branch is only reached when data has changed from Python even though we just exported. The comment on line 4097 correctly describes this, but the implementation doesn't prevent potential race conditions or feedback loops if Python modifies the data immediately after receiving it. Consider adding additional safeguards or logging to track such scenarios.
| const newDataStr = JSON.stringify(newData); | ||
| const lastExportedStr = JSON.stringify(lastExported); |
There was a problem hiding this comment.
Using JSON.stringify() for deep comparison of potentially large GeoJSON objects can be inefficient and may cause performance issues with many features or complex geometries. The order of properties in the JSON string may also differ even when the data is functionally identical. Consider using a more efficient comparison method, such as comparing feature counts and IDs, or using a library designed for deep object comparison.
| "id": "7", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Clear all drawn features from the map:" |
There was a problem hiding this comment.
The markdown cell describing "Clear all drawn features from the map:" appears after code cells that already demonstrate clearing features (lines 55 and 107). This documentation should be placed before the relevant code examples, specifically before line 55 where m.clear_geoman_data() is first called.
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "6", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "m.set_geoman_data({\"type\": \"FeatureCollection\", \"features\": []})" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
The code cell on line 107 duplicates the functionality already demonstrated on line 55. Both cells clear geoman data using similar approaches. Consider removing this duplicate example or documenting why both approaches are shown.
| "cell_type": "code", | |
| "execution_count": null, | |
| "id": "6", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "m.set_geoman_data({\"type\": \"FeatureCollection\", \"features\": []})" | |
| ] | |
| }, |
No description provided.