Deselect deleted features; closes mapbox#357

It involved some refactoring to keep the selection in the store, accessible outside of simple_select. And other refactoring to improve testability and clarify the API of the store.
bensleveritt · Jun 15, 2016 · c15a440 · c15a440
1 parent 57fd3d2
commit c15a440
Show file tree

Hide file tree

Showing 13 changed files with 431 additions and 101 deletions.
diff --git a/package.json b/package.json
@@ -12,8 +12,9 @@
     ]
   },
   "scripts": {
-    "test": "npm run lint && tape -r ./test/mock-browser.js -r babel-register test/*.test.js",
+    "test": "npm run lint && npm run tape",
     "lint": "eslint --no-eslintrc -c .eslintrc index.js src",
+    "tape": "tape -r ./test/mock-browser.js -r babel-register test/*.test.js",
     "build": "NODE_ENV=production browserify index.js > dist/mapbox-gl-draw.js",
     "prepublish": "NODE_ENV=production browserify index.js | uglifyjs -c -m > dist/mapbox-gl-draw.js",
     "start": "node server.js"

diff --git a/src/lib/mode_handler.js b/src/lib/mode_handler.js
@@ -53,7 +53,10 @@ var ModeHandler = function(mode, DrawContext) {
 
   return {
     render: mode.render || function(geojson) {return geojson; },
-    stop: mode.stop || function() {},
+    stop: function() {
+      DrawContext.store.clearSelected();
+      if (mode.stop) mode.stop();
+    },
     drag: function(event) {
       delegate('drag', event);
     },

diff --git a/src/lib/simple_set.js b/src/lib/simple_set.js
@@ -0,0 +1,31 @@
+function SimpleSet(items) {
+  this._items = items || [];
+}
+
+SimpleSet.prototype.add = function(item) {
+  if (this._items.indexOf(item) !== -1) return;
+  this._items.push(item);
+  return this;
+};
+
+SimpleSet.prototype.delete = function(item) {
+  var itemIndex = this._items.indexOf(item);
+  if (itemIndex === -1) return;
+  this._items.splice(itemIndex, 1);
+  return this;
+};
+
+SimpleSet.prototype.has = function(item) {
+  return this._items.indexOf(item) !== -1;
+};
+
+SimpleSet.prototype.values = function() {
+  return this._items;
+};
+
+SimpleSet.prototype.clear = function() {
+  this._items = [];
+  return this;
+};
+
+module.exports = SimpleSet;
diff --git a/src/modes/direct_select.js b/src/modes/direct_select.js
@@ -45,6 +45,7 @@ module.exports = function(ctx, opts) {
 
   return {
     start: function() {
+      ctx.store.setSelected(featureId);
       ctx.map.doubleClickZoom.disable();
       this.on('mousedown', isOfMetaType('vertex'), onVertex);
       this.on('mousedown', isOfMetaType('midpoint'), onMidpoint);

diff --git a/src/modes/draw_line_string.js b/src/modes/draw_line_string.js
@@ -52,6 +52,7 @@ module.exports = function(ctx) {
 
   return {
     start: function() {
+      ctx.store.setSelected(feature.id);
       setTimeout(() => {
         if (ctx.map && ctx.map.doubleClickZoom) {
           ctx.map.doubleClickZoom.disable();

diff --git a/src/modes/draw_point.js b/src/modes/draw_point.js
@@ -30,6 +30,7 @@ module.exports = function(ctx) {
 
   return {
     start: function() {
+      ctx.store.setSelected(feature.id);
       ctx.ui.setClass({mouse:'add'});
       ctx.ui.setButtonActive(types.POINT);
       this.on('click', () => true, onClick);

diff --git a/src/modes/draw_polygon.js b/src/modes/draw_polygon.js
@@ -53,6 +53,7 @@ module.exports = function(ctx) {
 
   return {
     start: function() {
+      ctx.store.setSelected(feature.id);
       setTimeout(() => {
         if (ctx.map && ctx.map.doubleClickZoom) {
           ctx.map.doubleClickZoom.disable();

diff --git a/src/modes/simple_select.js b/src/modes/simple_select.js
@@ -2,13 +2,7 @@ var {noFeature, isShiftDown, isFeature, isOfMetaType, isBoxSelecting, isActiveFe
 var { DOM } = require('../lib/util');
 var featuresAt = require('../lib/features_at');
 var addCoords = require('../lib/add_coords');
-module.exports = function(ctx, startingSelectedFeatureIds) {
-
-  var selectedFeaturesById = {};
-  (startingSelectedFeatureIds || []).forEach(id => {
-    selectedFeaturesById[id] = ctx.store.get(id);
-  });
-
+module.exports = function(ctx, startingSelectedIds) {
   var startPos = null;
   var dragging = null;
   var featureCoords = null;
@@ -36,11 +30,11 @@ module.exports = function(ctx, startingSelectedFeatureIds) {
       var featuresInBox = featuresAt(null, bbox, ctx);
       if (featuresInBox.length >= 1000) return ctx.map.dragPan.enable();
       var ids = getUniqueIds(featuresInBox)
-        .filter(id => !isSelected(id));
+        .filter(id => !ctx.store.isSelected(id));
 
       if (ids.length) {
         ids.forEach(id => {
-          selectedFeaturesById[id] = ctx.store.get(id);
+          ctx.store.select(id);
           context.render(id);
         });
         context.fire('selected.start', {featureIds: ids});
@@ -56,15 +50,16 @@ module.exports = function(ctx, startingSelectedFeatureIds) {
   var readyForDirectSelect = function(e) {
     if (isFeature(e)) {
       var about = e.featureTarget.properties;
-      return selectedFeaturesById[about.id] !== undefined && selectedFeaturesById[about.id].type !== 'Point';
+      return ctx.store.isSelected(about.id)
+        && ctx.store.get(about.id).type !== 'Point';
     }
     return false;
   };
 
   var buildFeatureCoords = function() {
-    var featureIds = Object.keys(selectedFeaturesById);
-    featureCoords = featureIds.map(id => selectedFeaturesById[id].coordinates);
-    features = featureIds.map(id => selectedFeaturesById[id]);
+    var featureIds = ctx.store.getSelectedIds();
+    featureCoords = featureIds.map(id => ctx.store.get(id).coordinates);
+    features = featureIds.map(id => ctx.store.get(id));
     numFeatures = featureIds.length;
   };
 
@@ -74,19 +69,19 @@ module.exports = function(ctx, startingSelectedFeatureIds) {
     });
   };
 
-  var isSelected = function(id) {
-    return selectedFeaturesById[id] !== undefined;
-  };
-
   return {
     stop: function() {
       ctx.map.doubleClickZoom.enable();
     },
     start: function() {
+      if (ctx.store) {
+        ctx.store.setSelected(startingSelectedIds);
+      }
+
       dragging = false;
       this.on('click', noFeature, function() {
-        var wasSelected = Object.keys(selectedFeaturesById);
-        selectedFeaturesById = {};
+        var wasSelected = ctx.store.getSelectedIds();
+        ctx.store.clearSelected();
         this.fire('selected.end', {featureIds: wasSelected});
         wasSelected.forEach(id => this.render(id));
         ctx.map.doubleClickZoom.enable();
@@ -118,35 +113,35 @@ module.exports = function(ctx, startingSelectedFeatureIds) {
       this.on('click', isFeature, function(e) {
         ctx.map.doubleClickZoom.disable();
         var id = e.featureTarget.properties.id;
-        var featureIds = Object.keys(selectedFeaturesById);
-        if (isSelected(id) && !isShiftDown(e)) {
+        var featureIds = ctx.store.getSelectedIds();
+        if (ctx.store.isSelected(id) && !isShiftDown(e)) {
           if (featureIds.length > 1) {
             this.fire('selected.end', {featureIds: featureIds.filter(f => f !== id)});
           }
           this.on('click', readyForDirectSelect, directSelect);
           ctx.ui.setClass({mouse:'pointer'});
         }
-        else if (isSelected(id) && isShiftDown(e)) {
-          delete selectedFeaturesById[id];
+        else if (ctx.store.isSelected(id) && isShiftDown(e)) {
+          ctx.store.deselect(id);
           this.fire('selected.end', {featureIds: [id]});
           ctx.ui.setClass({mouse:'pointer'});
           this.render(id);
           if (featureIds.length === 1 ) {
             ctx.map.doubleClickZoom.enable();
           }
         }
-        else if (!isSelected(id) && isShiftDown(e)) {
+        else if (!ctx.store.isSelected(id) && isShiftDown(e)) {
           // add to selected
-          selectedFeaturesById[id] = ctx.store.get(id);
+          ctx.store.select(id);
           this.fire('selected.start', {featureIds: [id]});
           ctx.ui.setClass({mouse:'move'});
           this.render(id);
         }
-        else if (!isSelected(id) && !isShiftDown(e)) {
+        else if (!ctx.store.isSelected(id) && !isShiftDown(e)) {
           // make selected
           featureIds.forEach(formerId => this.render(formerId));
-          selectedFeaturesById = {};
-          selectedFeaturesById[id] = ctx.store.get(id);
+          ctx.store.clearSelected();
+          ctx.store.select(id);
           ctx.ui.setClass({mouse:'move'});
           this.fire('selected.end', {featureIds: featureIds});
           this.fire('selected.start', {featureIds: [id]});
@@ -225,12 +220,11 @@ module.exports = function(ctx, startingSelectedFeatureIds) {
         featureCoords = null;
         features = null;
         numFeatures = null;
-        ctx.store.delete(Object.keys(selectedFeaturesById));
-        selectedFeaturesById = {};
+        ctx.store.delete(ctx.store.getSelectedIds());
       });
     },
     render: function(geojson, push) {
-      geojson.properties.active = selectedFeaturesById[geojson.properties.id] ? 'true' : 'false';
+      geojson.properties.active = ctx.store.isSelected(geojson.properties.id) ? 'true' : 'false';
       if (geojson.properties.active === 'true' && geojson.geometry.type !== 'Point') {
         addCoords(geojson, false, push, ctx.map, []);
       }

diff --git a/src/render.js b/src/render.js
@@ -10,12 +10,12 @@ module.exports = function render() {
     var newColdIds = [];
 
     if (this.isDirty) {
-      newColdIds = this.featureIds;
+      newColdIds = this.getAllIds();
     }
     else {
-      newHotIds = this.changedIds.filter(id => this.features[id] !== undefined);
+      newHotIds = this.getChangedIds().filter(id => this.get(id) !== undefined);
       newColdIds = this.sources.hot.filter(function getColdIds(geojson) {
-        return geojson.properties.id && newHotIds.indexOf(geojson.properties.id) === -1 && this.features[geojson.properties.id] !== undefined;
+        return geojson.properties.id && newHotIds.indexOf(geojson.properties.id) === -1 && this.get(geojson.properties.id) !== undefined;
       }.bind(this)).map(geojson => geojson.properties.id);
     }
 
@@ -37,7 +37,7 @@ module.exports = function render() {
       }
     }).forEach(function calculateViewUpdate(change) {
       let {id, source} = change;
-      let feature = this.features[id];
+      let feature = this.get(id);
       let featureInternal = feature.internal(mode);
 
       this.ctx.events.currentModeRender(featureInternal, function addGeoJsonToView(geojson) {
@@ -57,7 +57,7 @@ module.exports = function render() {
       features: this.sources.hot
     });
 
-    let changed = this.changedIds.map(id => this.features[id])
+    let changed = this.getChangedIds().map(id => this.get(id))
       .filter(feature => feature !== undefined)
       .filter(feature => feature.isValid())
       .map(feature => feature.toGeoJSON());
@@ -68,5 +68,5 @@ module.exports = function render() {
 
   }
   this.isDirty = false;
-  this.changedIds = [];
+  this.clearChangedIds();
 };