wip: refactor DataLayer.geojsonToFeatures and fix sync tests

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()
   }

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)
     }
   }

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())
   },
 

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)

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()
     })

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

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",
     }

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")