diff --git a/umap/static/umap/js/modules/data/features.js b/umap/static/umap/js/modules/data/features.js index cfecb891..0866b967 100644 --- a/umap/static/umap/js/modules/data/features.js +++ b/umap/static/umap/js/modules/data/features.js @@ -75,6 +75,15 @@ class Feature { return this.ui.getBounds() } + get geometry() { + return this._geometry + } + + set geometry(value) { + this._geometry = value + this.geometryChanged() + } + getClassName() { return this.staticOptions.className } @@ -112,10 +121,6 @@ class Feature { this.sync.upsert(this.toGeoJSON()) } - getGeometry() { - return this.toGeoJSON().geometry - } - isReadOnly() { return this.datalayer?.isDataReadOnly() } @@ -333,7 +338,7 @@ class Feature { } populate(geojson) { - this.geometry = geojson.geometry + this._geometry = geojson.geometry this.properties = Object.fromEntries( Object.entries(geojson.properties || {}).map(this.cleanProperty) ) @@ -494,13 +499,13 @@ class Feature { } clone() { - const geoJSON = this.toGeoJSON() - delete geoJSON.id - delete geoJSON.properties.id - const layer = this.datalayer.geojsonToFeatures(geoJSON) - layer.isDirty = true - layer.edit() - return layer + const geojson = this.toGeoJSON() + delete geojson.id + delete geojson.properties.id + const feature = this.datalayer.makeFeature(geojson) + feature.isDirty = true + feature.edit() + return feature } extendedProperties() { @@ -552,6 +557,10 @@ export class Point extends Feature { this.geometry.coordinates = GeoJSON.latLngToCoords(latlng) } + geometryChanged() { + this.ui.setLatLng(this.coordinates) + } + makeUI() { return new LeafletMarker(this) } @@ -628,6 +637,10 @@ class Path extends Feature { this.geometry.type = type } + geometryChanged() { + this.ui.setLatLngs(this.coordinates) + } + connectToDataLayer(datalayer) { super.connectToDataLayer(datalayer) // We keep markers on their own layer on top of the paths. @@ -814,7 +827,7 @@ export class LineString extends Path { delete geojson.id // delete the copied id, a new one will be generated. - const polygon = this.datalayer.geojsonToFeatures(geojson) + const polygon = this.datalayer.makeFeature(geojson) polygon.edit() this.del() } @@ -957,7 +970,7 @@ export class Polygon extends Path { geojson.geometry.coordinates = Utils.flattenCoordinates( geojson.geometry.coordinates ) - const polyline = this.datalayer.geojsonToFeatures(geojson) + const polyline = this.datalayer.makeFeature(geojson) polyline.edit() this.del() } diff --git a/umap/static/umap/js/modules/data/layer.js b/umap/static/umap/js/modules/data/layer.js index 01e88451..c8b782e4 100644 --- a/umap/static/umap/js/modules/data/layer.js +++ b/umap/static/umap/js/modules/data/layer.js @@ -375,7 +375,7 @@ export class DataLayer { removeFeature(feature, sync) { const id = stamp(feature) - // if (sync !== false) feature.sync.delete() + if (sync !== false) feature.sync.delete() this.layer.removeLayer(feature.ui) delete this.map.features_index[feature.getSlug()] feature.disconnectFromDataLayer(this) @@ -414,135 +414,56 @@ export class DataLayer { try { // Do not fail if remote data is somehow invalid, // otherwise the layer becomes uneditable. - this.geojsonToFeatures(geojson, sync) + this.makeFeatures(geojson, sync) } catch (err) { console.log('Error with DataLayer', this.umap_id) console.error(err) } } - // The choice of the name is not ours, because it is required by Leaflet. - // It is misleading, as the returned objects are uMap objects, and not - // GeoJSON features. - geojsonToFeatures(geojson, sync) { - if (!geojson) return - const features = Array.isArray(geojson) ? geojson : geojson.features - let i - let len - - if (features) { - Utils.sortFeatures(features, this.map.getOption('sortKey'), L.lang) - for (i = 0, len = features.length; i < len; i++) { - this.geojsonToFeatures(features[i]) - } - return this // Why returning "this" ? + makeFeatures(geojson = {}, sync = true) { + if (geojson.type === 'Feature' || geojson.coordinates) { + geojson = [geojson] } - - const geometry = geojson.type === 'Feature' ? geojson.geometry : geojson - - const feature = this.geoJSONToLeaflet({ geometry, geojson }) - if (feature) { - this.addFeature(feature) - if (sync) feature.onCommit() - return feature + const collection = Array.isArray(geojson) + ? geojson + : geojson.features || geojson.geometries + Utils.sortFeatures(collection, this.map.getOption('sortKey'), L.lang) + for (const feature of collection) { + this.makeFeature(feature, sync) } } - /** - * Create or update Leaflet features from GeoJSON geometries. - * - * If no `feature` is provided, a new feature will be created. - * If `feature` is provided, it will be updated with the passed geometry. - * - * GeoJSON and Leaflet use incompatible formats to encode coordinates. - * This method takes care of the convertion. - * - * @param geometry GeoJSON geometry field - * @param geojson Enclosing GeoJSON. If none is provided, a new one will - * be created - * @param id Id of the feature - * @param feature Leaflet feature that should be updated with the new geometry - * @returns Leaflet feature. - */ - geoJSONToLeaflet({ geometry, geojson = null, id = null, feature = null } = {}) { - if (!geometry) return // null geometry is valid geojson. - const coords = geometry.coordinates - let latlng - let latlngs - - // Create a default geojson if none is provided - if (geojson === undefined) geojson = { type: 'Feature', geometry: geometry } + makeFeature(geojson = {}, sync = true, id = null) { + const geometry = geojson.geometry || geojson + let feature switch (geometry.type) { case 'Point': - try { - latlng = GeoJSON.coordsToLatLng(coords) - } catch (e) { - console.error('Invalid latlng object from', coords) - break - } - if (feature) { - feature.coordinates = latlng - return feature - } - return new Point(this, geojson) - + // FIXME: deal with MutliPoint + feature = new Point(this, geojson, id) + break case 'MultiLineString': case 'LineString': - latlngs = GeoJSON.coordsToLatLngs( - coords, - geometry.type === 'LineString' ? 0 : 1 - ) - if (!latlngs.length) break - if (feature) { - feature.coordinates = latlngs - return feature - } - return new LineString(this, geojson) - + feature = new LineString(this, geojson, id) + break case 'MultiPolygon': case 'Polygon': - latlngs = GeoJSON.coordsToLatLngs(coords, geometry.type === 'Polygon' ? 1 : 2) - if (feature) { - feature.coordinates = latlngs - return feature - } - return new Polygon(this, geojson) - case 'GeometryCollection': - return this.geojsonToFeatures(geometry.geometries) - + feature = new Polygon(this, geojson, id) + break default: + console.log(geojson) Alert.error( translate('Skipping unknown geometry.type: {type}', { type: geometry.type || 'undefined', }) ) } - } - - _pointToLayer(geojson, latlng, id) { - return new U.Marker(this.map, latlng, { geojson: geojson, datalayer: this }, id) - } - - _lineToLayer(geojson, latlngs, id) { - return new U.Polyline( - this.map, - latlngs, - { - geojson: geojson, - datalayer: this, - color: null, - }, - id - ) - } - - _polygonToLayer(geojson, latlngs, id) { - // Ensure no empty hole - // for (let i = latlngs.length - 1; i > 0; i--) { - // if (!latlngs.slice()[i].length) latlngs.splice(i, 1); - // } - return new U.Polygon(this.map, latlngs, { geojson: geojson, datalayer: this }, id) + if (feature) { + this.addFeature(feature) + if (sync) feature.onCommit() + return feature + } } async importRaw(raw, format) { @@ -554,7 +475,7 @@ export class DataLayer { } importFromFiles(files, type) { - for (let i = 0, f; (f = files[i]); i++) { + for (const f of files) { this.importFromFile(f, type) } } diff --git a/umap/static/umap/js/modules/rendering/ui.js b/umap/static/umap/js/modules/rendering/ui.js index 143d96de..ea669097 100644 --- a/umap/static/umap/js/modules/rendering/ui.js +++ b/umap/static/umap/js/modules/rendering/ui.js @@ -126,6 +126,7 @@ export const LeafletMarker = Marker.extend({ geometryChanged: function() { this.feature.coordinates = this._latlng + this.feature.sync.update('geometry', this.feature.geometry) }, addInteractions() { @@ -133,7 +134,7 @@ export const LeafletMarker = Marker.extend({ this.on('dragend', (event) => { this.isDirty = true this.feature.edit(event) - this.feature.sync.update('geometry', this.feature.getGeometry()) + this.geometryChanged() }) this.on('editable:drawing:commit', this.onCommit) if (!this.feature.isReadOnly()) this.on('mouseover', this._enableDragging) @@ -251,7 +252,6 @@ const PathMixin = { }, _onDrag: function () { - this.geometryChanged() if (this._tooltip) this._tooltip.setLatLng(this.getCenter()) }, diff --git a/umap/static/umap/js/modules/sync/updaters.js b/umap/static/umap/js/modules/sync/updaters.js index ce6ef5cc..ca72ba0e 100644 --- a/umap/static/umap/js/modules/sync/updaters.js +++ b/umap/static/umap/js/modules/sync/updaters.js @@ -71,15 +71,13 @@ export class FeatureUpdater extends BaseUpdater { upsert({ metadata, value }) { const { id, layerId } = metadata const datalayer = this.getDataLayerFromID(layerId) - let feature = this.getFeatureFromMetadata(metadata, value) + const feature = this.getFeatureFromMetadata(metadata, value) - feature = datalayer.geoJSONToLeaflet({ - geometry: value.geometry, - geojson: value, - id, - feature, - }) - datalayer.addFeature(feature) + if (feature) { + feature.geometry = value.geometry + } else { + datalayer.makeFeature(value) + } } // Update a property of an object @@ -90,7 +88,8 @@ export class FeatureUpdater extends BaseUpdater { } if (key === 'geometry') { const datalayer = this.getDataLayerFromID(metadata.layerId) - datalayer.geoJSONToLeaflet({ geometry: value, id: metadata.id, feature }) + const feature = this.getFeatureFromMetadata(metadata, value) + feature.geometry = value } else { this.updateObjectValue(feature, key, value) feature.datalayer.indexProperties(feature) diff --git a/umap/static/umap/js/umap.controls.js b/umap/static/umap/js/umap.controls.js index 2321e9fe..a4fd58b4 100644 --- a/umap/static/umap/js/umap.controls.js +++ b/umap/static/umap/js/umap.controls.js @@ -1022,7 +1022,7 @@ U.Search = L.PhotonSearch.extend({ L.DomEvent.on(edit, 'mousedown', (e) => { L.DomEvent.stop(e) const datalayer = this.map.defaultEditDataLayer() - const layer = datalayer.geojsonToFeatures(feature) + const layer = datalayer.makeFeature(feature) layer.isDirty = true layer.edit() }) diff --git a/umap/tests/integration/conftest.py b/umap/tests/integration/conftest.py index 504ff5f3..079f6b78 100644 --- a/umap/tests/integration/conftest.py +++ b/umap/tests/integration/conftest.py @@ -27,10 +27,24 @@ def mock_osm_tiles(page): @pytest.fixture -def page(context): - page = context.new_page() - page.on("console", lambda msg: print(msg.text) if msg.type != "warning" else None) - return page +def new_page(context): + def make_page(prefix="console"): + page = context.new_page() + page.on( + "console", + lambda msg: print(f"{prefix}: {msg.text}") + if msg.type != "warning" + else None, + ) + page.on("pageerror", lambda exc: print(f"{prefix} uncaught exception: {exc}")) + return page + + yield make_page + + +@pytest.fixture +def page(new_page): + return new_page() @pytest.fixture diff --git a/umap/tests/integration/test_draw_polyline.py b/umap/tests/integration/test_draw_polyline.py index 17f40865..6759cf93 100644 --- a/umap/tests/integration/test_draw_polyline.py +++ b/umap/tests/integration/test_draw_polyline.py @@ -243,7 +243,9 @@ def test_can_transfer_shape_from_multi(live_server, page, tilelayer, settings): data = save_and_get_json(page) assert data["features"][0]["geometry"] == { "coordinates": [ - [-6.569824, 52.49616], [-7.668457, 52.49616], [-7.668457, 53.159947] + [-6.569824, 52.49616], + [-7.668457, 52.49616], + [-7.668457, 53.159947], ], "type": "LineString", } diff --git a/umap/tests/integration/test_websocket_sync.py b/umap/tests/integration/test_websocket_sync.py index a038f784..dfa86c3a 100644 --- a/umap/tests/integration/test_websocket_sync.py +++ b/umap/tests/integration/test_websocket_sync.py @@ -12,7 +12,7 @@ DATALAYER_UPDATE = re.compile(r".*/datalayer/update/.*") @pytest.mark.xdist_group(name="websockets") def test_websocket_connection_can_sync_markers( - context, live_server, websocket_server, tilelayer + new_page, live_server, websocket_server, tilelayer ): map = MapFactory(name="sync", edit_status=Map.ANONYMOUS) map.settings["properties"]["syncEnabled"] = True @@ -20,9 +20,9 @@ def test_websocket_connection_can_sync_markers( DataLayerFactory(map=map, data={}) # Create two tabs - peerA = context.new_page() + peerA = new_page("Page A") peerA.goto(f"{live_server.url}{map.get_absolute_url()}?edit") - peerB = context.new_page() + peerB = new_page("Page B") peerB.goto(f"{live_server.url}{map.get_absolute_url()}?edit") a_marker_pane = peerA.locator(".leaflet-marker-pane > div") @@ -60,12 +60,12 @@ def test_websocket_connection_can_sync_markers( expect(b_marker_pane).to_have_count(2) # Drag a marker on peer B and check that it moved on peer A - a_first_marker.bounding_box() == b_first_marker.bounding_box() + assert a_first_marker.bounding_box() == b_first_marker.bounding_box() b_old_bbox = b_first_marker.bounding_box() b_first_marker.drag_to(b_map_el, target_position={"x": 250, "y": 250}) assert b_old_bbox is not b_first_marker.bounding_box() - a_first_marker.bounding_box() == b_first_marker.bounding_box() + assert a_first_marker.bounding_box() == b_first_marker.bounding_box() # Delete a marker from peer A and check it's been deleted on peer B a_first_marker.click(button="right")