Fix FeatureLayer.refresh for moved point layers

patrickarlt commented 2 years ago

This fixes FeatureLayer.refresh() for layers where simplifyFactor is not set. Originally this was intended to update the geometry when a higher resolution geometry came in from the service but it also means that layers with edited geometries were missed by FeatureLayer.refresh()

Can be reproudced with the following HTML in the debug folder (originally from @gavinr):

<!-- https://esri.github.io/esri-leaflet/examples/simple-editing.html -->
<!-- Forked from https://codepen.io/jwasilgeo/pen/KKXVjvy?editors=1000 -->
<!DOCTYPE html>
<html>

<head>
  <meta charset="utf-8" />
  <title>Editing feature layers</title>
  <meta name="viewport" content="initial-scale=1,maximum-scale=1,user-scalable=no" />

  <!-- Load Leaflet from CDN -->
  <link rel="stylesheet" href="https://unpkg.com/leaflet@1.7.1/dist/leaflet.css" />
  <script src="https://unpkg.com/leaflet@1.7.1/dist/leaflet.js"></script>

  <!-- Load Esri Leaflet from source-->
  <script src="../dist/esri-leaflet-debug.js"></script>

  <style>
    body {
      margin: 0;
      padding: 0;
    }

    #map {
      position: absolute;
      top: 150px;
      bottom: 0;
      right: 0;
      left: 0;
    }
  </style>
</head>

<body>

  <!-- Leaflet Draw -->
  <style>
    #info-pane {
      z-index: 400;
      padding: 1em;
      background: white;
      text-align: right;
    }

    #form {
      display: none;
    }
  </style>

  <div id="map"></div>
  <div id="info-pane" class="leaflet-bar" style="font-size: 10px">
    These do not use the Leaflet or Esri Leaflet add/edit funcitonality. They call the REST endpoints using FETCH
    directly:<br />
    <button id="addFeatureButton">ADD a feature within the current extent</button><br />
    <button id="moveFeatureButton">MOVE one of the features within the current extent</button>
    <br /><br />
    This DOES call the leaflet layer.refresh() function:<br />
    <button id="refreshLayerButton">Refresh the layer</button>
  </div>

  <script>
    // create the map
    var map = L.map('map').setView([38.631, -90.199], 8);
    // basemap
    var OpenStreetMap_Mapnik = L.tileLayer('https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png', {
      maxZoom: 19,
      attribution: '&copy; <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors'
    });
    OpenStreetMap_Mapnik.addTo(map);
    // add our feature layer to the map
    const militaryOps = L.esri.featureLayer({
      url: 'https://sampleserver6.arcgisonline.com/arcgis/rest/services/Military/FeatureServer/3'
    }).addTo(map);

    const addFeatureButton = document.querySelector("#addFeatureButton");
    const moveFeatureButton = document.querySelector("#moveFeatureButton");
    const refreshLayerButton = document.querySelector("#refreshLayerButton");
    addFeatureButton.addEventListener("click", () => {
      const latlng = getRandomPointWithinCurrentExtent(map);
      const feat = L.marker(latlng).toGeoJSON();
      // set the attribute value that controls the feature service symbology
      feat.properties.symbolname = "Decision Point";
      fetch(`${militaryOps.options.url}/addFeatures`, {
        "headers": {
          "Content-Type": "application/x-www-form-urlencoded; charset=UTF-8",
        },
        "body": `features=${JSON.stringify([L.esri.Util.geojsonToArcGIS(feat)])}&f=json`,
        "method": "POST",
        "mode": "cors"
      })
        .then(res => res.json())
        .then(res => {
          console.log(res);
          // militaryOps.refresh();
        });
    });

    moveFeatureButton.addEventListener("click", () => {
      console.log('move', map.getPanes()[militaryOps.options.pane]);
      const latlng = getRandomPointWithinCurrentExtent(map);
      console.log("latlng", latlng);

      let feat = {};
      militaryOps.eachActiveFeature((feature) => {
        feat.attributes = {};
        feat.attributes.OBJECTID = feature.feature.id
        feat.geometry = { "spatialReference": { "wkid": 4326 } };

        feat.geometry.x = latlng.lng;
        feat.geometry.y = latlng.lat;
      });

      console.log('sending:', feat);
      // const latlng = getRandomPointWithinCurrentExtent(map);
      // const feat = L.marker(latlng).toGeoJSON();
      // // set the attribute value that controls the feature service symbology
      // feat.properties.symbolname = "Decision Point";
      fetch(`${militaryOps.options.url}/updateFeatures`, {
        "headers": {
          "Content-Type": "application/x-www-form-urlencoded; charset=UTF-8",
        },
        "body": `features=${JSON.stringify([feat])}&f=json`,
        "method": "POST",
        "mode": "cors"
      })
        .then(res => res.json())
        .then(res => {
          console.log(res);
          // militaryOps.refresh();
        });
    });

    refreshLayerButton.addEventListener("click", () => {
      militaryOps.refresh();
    });

    function randomIntFromInterval(min, max) { // min and max included 
      return Math.random() * (max - min) + min;
    }
    const getRandomPointWithinCurrentExtent = (map) => {
      const bounds = map.getBounds();
      var lng = randomIntFromInterval(bounds.getEast(), bounds.getWest()).toFixed(5);
      var lat = randomIntFromInterval(bounds.getSouth(), bounds.getNorth()).toFixed(5);
      return L.latLng(lat, lng);
    }

    // when a marker is clicked, remove the feature from the service, using its id
    militaryOps.on('click', function (e) {
      // militaryOps.deleteFeature(e.layer.feature.id, function(err, response) {
      //   if (err) {
      //     return;
      //   }
      //   console.log(response);
      // });
      fetch(`${militaryOps.options.url}/deleteFeatures`, {
        "headers": {
          "Content-Type": "application/x-www-form-urlencoded; charset=UTF-8",
        },
        "body": `objectIds=${e.layer.feature.id}&f=json`,
        "method": "POST",
        "mode": "cors"
      })
        .then(res => res.json())
        .then(res => {
          console.log(res);
          militaryOps.refresh();
        });
      // make sure map click event isn't fired.
      L.DomEvent.stopPropagation(e);
    });
  </script>

</body>

</html>

PedroVenancio commented 2 years ago

Dear @patrickarlt and @jwasilgeo

Thank you very much for this fix! I was testing it and these are my findings after apply the fix:

it works updating geometry position;
it also works updating attributes, both in popup and tooltip;

but it does not update the symbology. In my test case (https://codepen.io/PedroNGV/pen/rNzoQeE) I'm defining the icons symbol

var testIcons = {
  'em curso': L.icon({
    iconUrl: 'https://esri.github.io/esri-leaflet/img/red-triangle.png',
    iconSize: [20, 20],
    iconAnchor: [6, 6],
    popupAnchor: [-3, -5],
  }),
  'em resolução': L.icon({
    iconUrl: 'https://esri.github.io/esri-leaflet/img/orange-triangle.png',
    iconSize: [20, 20],
    iconAnchor: [6, 6],
    popupAnchor: [-3, -5],
  }),
  'em conclusão': L.icon({
    iconUrl: 'https://esri.github.io/esri-leaflet/img/yellow-triangle.png',
    iconSize: [20, 20],
    iconAnchor: [6, 6],
    popupAnchor: [-3, -5],
  }),
};

and then the icon is applied based on an attribute value:

function(geojson, latlng) {
    return L.marker(latlng, {
      icon: testIcons[geojson.properties.EstadoAgrupado.toLowerCase()]
    });
  }

The attribute is updated after this fix, but the symbol only changes if I refresh the browser. Sometimes I also see that the symbology changes randomly after some FeatureLayer.refresh() requests, just like it happened with attributes and geometries before the fix. So it seems to me that the refresh to symbology is somehow missing.

Can you check it in your test case?

Thank you very much! Best regards, Pedro

jwasilgeo commented 2 years ago

I can reproduce the invalid symbology after an update and refresh. Below is a slightly modified debug case (starting from the debug code at the top here) that switches between 2 different attributes and symbols during the update operation.

debug code

```html Editing feature layers

These do not use the Leaflet or Esri Leaflet add/edit funcitonality. They call the REST endpoints using FETCH directly:

This DOES call the leaflet layer.refresh() function:

```

jwasilgeo commented 2 years ago

@patrickarlt, your thoughts on commit ef8579b28e3a6470f97b634374a1417661a0b646, if that's the right spot--timing wise--to add a call to redraw()?

PedroVenancio commented 2 years ago

@jwasilgeo It seems to work also in my debug case! Thank you very much!

jwasilgeo commented 2 years ago

@PedroVenancio I'm glad to hear it is working for you in your case. We decided for now not to merge this PR as we need more time to consider the consequences of these changes (that in some cases this would lead to inefficient code and re-rendering on the map, especially with the added call to redraw()), and then plan how we will improve upon it when using a FeatureServer layer that has editing enabled.

For your situation I recommend that your options are either:

Use the full set of proposed changes in this PR yourself, or
Use the original proposed changes in commit 836caa241b9b61b8335ec77f80a5c389da86b5d3 and also manually call yourLayer.redraw(yourFeatureUniqueID) after the refresh, if you know the ID of the feature that gets updated.
```
yourLayer.refresh();
yourLayer.redraw(90210);
```

PedroVenancio commented 2 years ago

Hi @jwasilgeo

I've implemented your first proposal for now. If I see it causes some impact, I will change for the second proposal, but I hope you can check the impact and merge this (or a polished) PR in the meanwhile.

Thank you very much!

patrickarlt commented 2 years ago

@PedroVenancio @jwasilgeo @gavinr I think we should merge this as is with https://github.com/Esri/esri-leaflet/commit/ef8579b28e3a6470f97b634374a1417661a0b646. There might be some performance hit because we will be updating features all the time but I think that is actually the proper behavior.

PedroVenancio commented 2 years ago

@patrickarlt @jwasilgeo @gavinr Thank you very much for the fix.

I must say that I've implemented https://github.com/Esri/esri-leaflet/pull/1304/commits/836caa241b9b61b8335ec77f80a5c389da86b5d3 and https://github.com/Esri/esri-leaflet/commit/ef8579b28e3a6470f97b634374a1417661a0b646 in my applications and I'm not seeing any performance hit. Maybe the impact can be greater when editing features, but I also think this is the proper behaviour.

Thanks again for your help!

Esri / esri-leaflet

Fix FeatureLayer.refresh for moved point layers #1304