From f3bd8b72270636cbfb56b96cbac8af8ca137349b Mon Sep 17 00:00:00 2001
From: Bryan Housel <bryan@7thposition.com>
Date: Tue, 10 Mar 2015 00:06:29 -0400
Subject: [PATCH] Feature Filtering: don't match multipolygon lines as 'others'
 (fixes #2548)

If the entity is a way that is a member of a parent relation,
use that parent relation's matches instead of matching 'other'
---
 js/id/renderer/features.js     | 48 +++++++++++++++++++++-------------
 test/spec/renderer/features.js | 13 +++++++--
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/js/id/renderer/features.js b/js/id/renderer/features.js
index 22f9ea47539..9793025d48f 100644
--- a/js/id/renderer/features.js
+++ b/js/id/renderer/features.js
@@ -154,20 +154,11 @@ iD.Features = function(context) {
         return false;
     });
 
-    // lines or areas that don't match another feature filter.
+    // Lines or areas that don't match another feature filter.
+    // IMPORTANT: The 'others' feature must be the last one defined,
+    //   so that code in getMatches can skip this test if `hasMatch = true`
     defineFeature('others', function isOther(entity, resolver, geometry) {
-        return (geometry === 'line' || geometry === 'area') && !(
-            _features.major_roads.filter(entity, resolver, geometry) ||
-            _features.minor_roads.filter(entity, resolver, geometry) ||
-            _features.paths.filter(entity, resolver, geometry) ||
-            _features.buildings.filter(entity, resolver, geometry) ||
-            _features.landuse.filter(entity, resolver, geometry) ||
-            _features.boundaries.filter(entity, resolver, geometry) ||
-            _features.water.filter(entity, resolver, geometry) ||
-            _features.rail.filter(entity, resolver, geometry) ||
-            _features.power.filter(entity, resolver, geometry) ||
-            _features.past_future.filter(entity, resolver, geometry)
-        );
+        return (geometry === 'line' || geometry === 'area');
     });
 
 
@@ -237,6 +228,8 @@ iD.Features = function(context) {
 
     features.gatherStats = function(d, resolver, dimensions) {
         var needsRedraw = false,
+            type = _.groupBy(d, function(ent) { return ent.type; }),
+            entities = [].concat(type.relation || [], type.way || [], type.node || []),
             currHidden, geometry, matches;
 
         _.each(_features, function(f) { f.count = 0; });
@@ -245,10 +238,10 @@ iD.Features = function(context) {
         // a _cullFactor of 1 corresponds to a 1000x1000px viewport..
         _cullFactor = dimensions[0] * dimensions[1] / 1000000;
 
-        for (var i = 0; i < d.length; i++) {
-            geometry = d[i].geometry(resolver);
+        for (var i = 0; i < entities.length; i++) {
+            geometry = entities[i].geometry(resolver);
             if (!(geometry === 'vertex' || geometry === 'relation')) {
-                matches = Object.keys(features.getMatches(d[i], resolver, geometry));
+                matches = Object.keys(features.getMatches(entities[i], resolver, geometry));
                 for (var j = 0; j < matches.length; j++) {
                     _features[matches[j]].count++;
                 }
@@ -296,9 +289,28 @@ iD.Features = function(context) {
 
             if (!(geometry === 'vertex' || geometry === 'relation')) {
                 for (var i = 0; i < _keys.length; i++) {
-                    if (hasMatch && _keys[i] === 'others') {
-                        continue;
+
+                    if (_keys[i] === 'others') {
+                        if (hasMatch) continue;
+
+                        // If the entity is a way that has not matched any other
+                        // feature type, see if it has a parent relation, and if so,
+                        // match whatever feature types the parent has matched.
+                        // (The way is a member of a multipolygon.)
+                        //
+                        // IMPORTANT:
+                        // For this to work, getMatches must be called on relations before ways.
+                        //
+                        if (entity.type === 'way') {
+                            var parents = features.getParents(entity, resolver, geometry);
+                            if (parents.length === 1) {
+                                var pkey = iD.Entity.key(parents[0]);
+                                matches = _.clone(_cache[pkey].matches);
+                                continue;
+                            }
+                        }
                     }
+
                     if (_features[_keys[i]].filter(entity, resolver, geometry)) {
                         matches[_keys[i]] = hasMatch = true;
                     }
diff --git a/test/spec/renderer/features.js b/test/spec/renderer/features.js
index 3b82a070fa9..35a615f4eaf 100644
--- a/test/spec/renderer/features.js
+++ b/test/spec/renderer/features.js
@@ -135,6 +135,15 @@ describe('iD.Features', function() {
             iD.Way({id: 'scrub', tags: {area: 'yes', natural: 'scrub'}, version: 1}),
             iD.Way({id: 'industrial', tags: {area: 'yes', landuse: 'industrial'}, version: 1}),
             iD.Way({id: 'parkinglot', tags: {area: 'yes', amenity: 'parking', parking: 'surface'}, version: 1}),
+            iD.Way({id: 'inner', version: 1}),
+            iD.Way({id: 'outer', version: 1}),
+            iD.Relation({id: 'retail', tags: {landuse: 'retail', type: 'multipolygon'},
+                    members: [
+                        {id: 'outer', role: 'outer', type: 'way'},
+                        {id: 'inner', role: 'inner', type: 'way'}
+                    ],
+                    version: 1
+                }),
 
             // Boundaries
             iD.Way({id: 'boundary', tags: {boundary: 'administrative'}, version: 1}),
@@ -279,7 +288,7 @@ describe('iD.Features', function() {
 
             doMatch([
                 'forest', 'scrub', 'industrial', 'parkinglot', 'building_no',
-                'rail_landuse', 'landuse_construction'
+                'rail_landuse', 'landuse_construction', 'retail', 'inner', 'outer'
             ]);
 
             dontMatch([
@@ -385,7 +394,7 @@ describe('iD.Features', function() {
             dontMatch([
                 'point_bar', 'motorway', 'service', 'path', 'building_yes',
                 'forest', 'boundary', 'water', 'railway', 'power_line',
-                'motorway_construction',
+                'motorway_construction', 'retail', 'inner', 'outer'
             ]);
         });
     });