Fix GeoJSON Source processing in MockFeatureCollection (#1248)

If there were multiple features of the same type in a GeoJSON file and
the file did not include names for the components, one would receive
multiple components with the same name (e.g., LineString1). LineString
processing was also broken, resulting in a coordinate string without
values. Setting the Source to None... would remove the components but
not clear the features off the map.

This commit addresses three changes:

1. Refactors how naming of map features is handled during the loading
   of GeoJSON into a MockFeatureCollection so that each component has a
   unique name.
2. It corrects handling of LineString coordinates when the line string
   is not part of a MultiLineString construct.
3. It cleans up how features are removed when a feature collection is
   cleared.

Change-Id: Iccbcab65989169ab730239b8915b62ca6b6f636c

appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockFeatureCollection.java

@@ -104,6 +104,7 @@ public class MockFeatureCollection extends MockContainer implements MockMapFeatu
       List<MockComponent> children = new ArrayList<MockComponent>(getChildren());
       for (MockComponent component : children) {
         removeComponent(component, true);
+        component.onRemoved();
       }
       return;
     }
@@ -117,10 +118,8 @@ public class MockFeatureCollection extends MockContainer implements MockMapFeatu
       @Override
       public void onSuccess(String result) {
         setGeoJSONProperty(result);
-        for (MockMapFeature feature : features) {
-          MockFeatureCollection.this.addVisibleComponent((MockComponent) feature, -1);
-          feature.addToMap(map);
-        }
+        MockFeatureCollection.this.onSelectedChange(true);  // otherwise the last imported component
+                                                            // will be shown in the properties panel
       }
     });
   }

appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockLineString.java

@@ -32,7 +32,6 @@ public class MockLineString extends MockMapFeatureBase {
   static MockLineString fromGeoJSON(MockFeatureCollection parent, JSONObject properties, JavaScriptObject layer) {
     MockLineString line = new MockLineString(parent.editor);
     line.feature = layer;
-    line.setContainer(parent);
     String name = null;
     for (String key : properties.keySet()) {
       String value;
@@ -60,17 +59,7 @@ public class MockLineString extends MockMapFeatureBase {
         line.onPropertyChange(PROPERTY_NAME_VISIBLE, value);
       }
     }
-    if (name == null) {
-      name = line.getPropertyValue(PROPERTY_NAME_TITLE);
-    }
-    name = name.replaceAll("[ \t]+", "_");
-    if (name.equalsIgnoreCase("")) {
-      name = ComponentsTranslation.getComponentName(TYPE) + "1";
-    }
-    name = ensureUniqueName(name, parent.editor.getComponentNames());
-    line.changeProperty(PROPERTY_NAME_NAME, name);
-    line.onPropertyChange(PROPERTY_NAME_NAME, name);
-    line.getForm().fireComponentRenamed(line, ComponentsTranslation.getComponentName(TYPE));
+    processFeatureName(line, parent, name);
     line.preserveLayerData();
     return line;
   }
@@ -188,7 +177,11 @@ public class MockLineString extends MockMapFeatureBase {
     if (line) {
       var latlngs = line.getLatLngs();
       var resultJson = [];
-      if (latlngs[0][0] instanceof top.L.LatLng) {
+      if (latlngs[0] instanceof top.L.LatLng) {
+        for (var i = 0; i < latlngs.length; i++) {
+          resultJson.push([latlngs[i].lat, latlngs[i].lng]);
+        }
+      } else if (latlngs[0][0] instanceof top.L.LatLng) {
         for (var i = 0; i < latlngs[0].length; i++) {
           resultJson.push([latlngs[0][i].lat, latlngs[0][i].lng]);
         }

appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockMapFeatureBase.java

@@ -5,6 +5,7 @@
 
 package com.google.appinventor.client.editor.simple.components;
 
+import com.google.appinventor.client.ComponentsTranslation;
 import com.google.appinventor.client.editor.simple.SimpleEditor;
 import com.google.gwt.core.client.JavaScriptObject;
 import com.google.gwt.resources.client.ImageResource;
@@ -125,7 +126,7 @@ public abstract class MockMapFeatureBase extends MockVisibleComponent implements
     }
     int i = name.length() - 1;
     while (name.charAt(i) >= '0' && name.charAt(i) <= '9') i--;
-    String base = name.substring(0, i);
+    String base = name.substring(0, i + 1);
     int counter = 1;
     if (i < name.length() - 1) {
       counter = Integer.parseInt(name.substring(i + 1)) + 1;
@@ -134,6 +135,25 @@ public abstract class MockMapFeatureBase extends MockVisibleComponent implements
     return base + counter;
   }
 
+  protected static void processFeatureName(MockMapFeatureBase feature, MockFeatureCollection parent,
+      String name) {
+    if (name == null) {
+      name = feature.getPropertyValue(PROPERTY_NAME_TITLE);
+    }
+    name = name.replaceAll("[ \t]+", "_");
+    if (name.equalsIgnoreCase("")) {
+      name = ComponentsTranslation.getComponentName(feature.getType()) + "1";
+    }
+    name = ensureUniqueName(name, parent.editor.getComponentNames());
+    parent.addVisibleComponent(feature, -1);
+    final String oldName = feature.getPropertyValue(PROPERTY_NAME_NAME);
+    if (oldName == null || !oldName.equals(name)) {
+      feature.changeProperty(PROPERTY_NAME_NAME, name);
+      feature.onPropertyChange(PROPERTY_NAME_NAME, name);
+      feature.getForm().fireComponentRenamed(feature, oldName);
+    }
+  }
+
   // JSNI Methods
   public static native double distanceBetweenPoints(JavaScriptObject map, MockMap.LatLng point1, MockMap.LatLng point2)/*-{
     var pt1 = [point1.@com.google.appinventor.client.editor.simple.components.MockMap.LatLng::latitude,

appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockMarker.java

@@ -53,7 +53,6 @@ public class MockMarker extends MockMapFeatureBaseWithFill {
   static MockMarker fromGeoJSON(MockFeatureCollection parent, JSONObject properties, JavaScriptObject layer) {
     MockMarker marker = new MockMarker(parent.editor);
     marker.feature = layer;
-    marker.setContainer(parent);
     String name = null;
     boolean hadImageAsset = false;
     for (String key : properties.keySet()) {
@@ -107,17 +106,7 @@ public class MockMarker extends MockMapFeatureBaseWithFill {
         marker.onPropertyChange(PROPERTY_NAME_DESCRIPTION, value);
       }
     }
-    if (name == null) {
-      name = marker.getPropertyValue(PROPERTY_NAME_TITLE);
-    }
-    name = name.replaceAll("[ \t]+", "_");
-    if (name.equalsIgnoreCase("")) {
-      name = ComponentsTranslation.getComponentName("Marker") + "1";
-    }
-    name = ensureUniqueName(name, parent.editor.getComponentNames());
-    marker.changeProperty(PROPERTY_NAME_NAME, name);
-    marker.onPropertyChange(PROPERTY_NAME_NAME, name);
-    marker.getForm().fireComponentRenamed(marker, ComponentsTranslation.getComponentName("Marker"));
+    processFeatureName(marker, parent, name);
     if (!hadImageAsset) {
       marker.setImageAsset(null);
     }

appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockPolygon.java

@@ -34,6 +34,7 @@ public class MockPolygon extends MockPolygonBase {
   static MockPolygon fromGeoJSON(MockFeatureCollection parent, JSONObject properties, JavaScriptObject layer) {
     MockPolygon polygon = new MockPolygon(parent.editor);
     polygon.feature = layer;
+    String name = null;
     boolean hadFillColor = false, hadStrokeColor = false, hadStrokeWidth = false;
     for (String key : properties.keySet()) {
       if (key.equalsIgnoreCase(PROPERTY_NAME_STROKEWIDTH) || key.equalsIgnoreCase(CSS_PROPERTY_STROKEWIDTH)) {
@@ -46,9 +47,7 @@ public class MockPolygon extends MockPolygonBase {
         polygon.getProperties().changePropertyValue(PROPERTY_NAME_FILLCOLOR, properties.get(key).isString().stringValue());
         hadFillColor = true;
       } else if (key.equalsIgnoreCase(PROPERTY_NAME_NAME)) {
-        String name = properties.get(key).isString().stringValue();
-        name = name.replaceAll("[ \t]+", "_");
-        polygon.changeProperty(PROPERTY_NAME_NAME, name);
+        name = properties.get(key).isString().stringValue();
       } else if (key.equalsIgnoreCase(PROPERTY_NAME_VISIBLE)) {
         polygon.setVisibleProperty(properties.get(key).isString().stringValue());
       }
@@ -62,6 +61,7 @@ public class MockPolygon extends MockPolygonBase {
     if (!hadStrokeWidth) {
       polygon.getProperties().changePropertyValue(PROPERTY_NAME_STROKEWIDTH, "1");
     }
+    processFeatureName(polygon, parent, name);
     polygon.preserveLayerData();
     return polygon;
   }

appinventor/blocklyeditor/src/component_database.js

@@ -157,9 +157,12 @@ Blockly.ComponentDatabase.prototype.renameInstance = function(uid, oldName, newN
   if (!this.hasInstance(uid)) {
     return false;
   }
+  if (oldName === newName) {  // oldName is the same as newName... don't waste time
+    return false;
+  }
   this.instances_[uid].name = newName;
-  this.instanceNameUid_[newName] = uid;
   delete this.instanceNameUid_[oldName];
+  this.instanceNameUid_[newName] = uid;
   return true;
 };