Recategorize tags_suggests_area validation rule into mismatched_geometry rule

Warn about points tagged as vertices and vertices tagged as points (close #6319)

data/core.yaml

@@ -1436,6 +1436,9 @@ en:
         message: '{feature} has an invalid website.'
         message_multi: '{feature} has multiple invalid websites.'
         reference: 'Websites should start with "http" or "https".'
+    mismatched_geometry:
+      title: Mismatched Geometry
+      tip: "Find features with conflicting tags and geometry"
     missing_role:
       title: Missing Roles
       message: "{member} has no role within {relation}"
@@ -1466,6 +1469,9 @@ en:
       noncanonical_brand:
         message: "{feature} looks like a brand with nonstandard tags"
         reference: "All features of the same brand should be tagged the same way."
+    point_as_vertex:
+      message: '{feature} should be a standalone point based on its tags'
+      reference: "Some features shouldn't be part of lines or areas."
     private_data:
       title: Private Information
       tip: "Find features that may contain private information"
@@ -1473,9 +1479,7 @@ en:
       contact:
         message: '{feature} might be tagged with private contact information'
     tag_suggests_area:
-      title: Lines Tagged as Areas
       message: '{feature} should be a closed area based on the tag "{tag}"'
-      tip: "Find features that are tagged as lines and should possibly be tagged as areas"
       reference: "Areas must have connected endpoints."
     unknown_road:
       message: "{feature} has no classification"
@@ -1503,6 +1507,9 @@ en:
       tip: "Find features with unsquare corners that can be drawn better"
       buildings:
         reference: "Buildings with unsquare corners can often be drawn more accurately."
+    vertex_as_point:
+      message: '{feature} should be part of a line or area based on its tags'
+      reference: "Some features shouldn't be standalone points."
     fix:
       connect_almost_junction:
         annotation: Connected very close features.

dist/locales/en.json

@@ -1777,6 +1777,10 @@
                     "reference": "Websites should start with \"http\" or \"https\"."
                 }
             },
+            "mismatched_geometry": {
+                "title": "Mismatched Geometry",
+                "tip": "Find features with conflicting tags and geometry"
+            },
             "missing_role": {
                 "title": "Missing Roles",
                 "message": "{member} has no role within {relation}",
@@ -1817,6 +1821,10 @@
                     "reference": "All features of the same brand should be tagged the same way."
                 }
             },
+            "point_as_vertex": {
+                "message": "{feature} should be a standalone point based on its tags",
+                "reference": "Some features shouldn't be part of lines or areas."
+            },
             "private_data": {
                 "title": "Private Information",
                 "tip": "Find features that may contain private information",
@@ -1826,9 +1834,7 @@
                 }
             },
             "tag_suggests_area": {
-                "title": "Lines Tagged as Areas",
                 "message": "{feature} should be a closed area based on the tag \"{tag}\"",
-                "tip": "Find features that are tagged as lines and should possibly be tagged as areas",
                 "reference": "Areas must have connected endpoints."
             },
             "unknown_road": {
@@ -1868,6 +1874,10 @@
                     "reference": "Buildings with unsquare corners can often be drawn more accurately."
                 }
             },
+            "vertex_as_point": {
+                "message": "{feature} should be part of a line or area based on its tags",
+                "reference": "Some features shouldn't be standalone points."
+            },
             "fix": {
                 "connect_almost_junction": {
                     "annotation": "Connected very close features."

modules/core/validator.js

@@ -349,7 +349,7 @@ export function coreValidator(context) {
             ran.impossible_oneway = true;
         }
 
-        runValidation('tag_suggests_area');
+        runValidation('mismatched_geometry');
 
         // run all rules not yet run
         Object.keys(_rules).forEach(runValidation);

modules/validations/index.js

@@ -8,9 +8,9 @@ export { validationImpossibleOneway } from './impossible_oneway';
 export { validationIncompatibleSource } from './incompatible_source';
 export { validationFormatting } from './invalid_format';
 export { validationMaprules } from './maprules';
+export { validationMismatchedGeometry } from './mismatched_geometry';
 export { validationMissingRole } from './missing_role';
 export { validationMissingTag } from './missing_tag';
 export { validationOutdatedTags } from './outdated_tags';
 export { validationPrivateData } from './private_data';
-export { validationTagSuggestsArea } from './tag_suggests_area';
-export { validationUnsquareWay } from './unsquare_way';
+export { validationUnsquareWay } from './unsquare_way';

modules/validations/mismatched_geometry.js

@@ -0,0 +1,208 @@
+import { actionAddVertex } from '../actions/add_vertex';
+import { actionChangeTags } from '../actions/change_tags';
+import { actionMergeNodes } from '../actions/merge_nodes';
+import { osmNodeGeometriesForTags } from '../osm/tags';
+import { geoHasSelfIntersections, geoSphericalDistance } from '../geo';
+import { t } from '../util/locale';
+import { utilDisplayLabel, utilTagText } from '../util';
+import { validationIssue, validationIssueFix } from '../core/validation';
+
+
+export function validationMismatchedGeometry(context) {
+    var type = 'mismatched_geometry';
+
+    function tagSuggestingLineIsArea(entity) {
+        if (entity.type !== 'way' || entity.isClosed()) return null;
+
+        var tagSuggestingArea = entity.tagSuggestingArea();
+        if (!tagSuggestingArea) {
+            return null;
+        }
+
+        if (context.presets().matchTags(tagSuggestingArea, 'line') ===
+            context.presets().matchTags(tagSuggestingArea, 'area')) {
+            // these tags also allow lines and making this an area wouldn't matter
+            return null;
+        }
+
+        return tagSuggestingArea;
+    }
+
+    function makeConnectEndpointsFixOnClick(way, graph) {
+        // must have at least three nodes to close this automatically
+        if (way.nodes.length < 3) return null;
+
+        var nodes = graph.childNodes(way), testNodes;
+        var firstToLastDistanceMeters = geoSphericalDistance(nodes[0].loc, nodes[nodes.length-1].loc);
+
+        // if the distance is very small, attempt to merge the endpoints
+        if (firstToLastDistanceMeters < 0.75) {
+            testNodes = nodes.slice();   // shallow copy
+            testNodes.pop();
+            testNodes.push(testNodes[0]);
+            // make sure this will not create a self-intersection
+            if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) {
+                return function(context) {
+                    var way = context.entity(this.issue.entityIds[0]);
+                    context.perform(
+                        actionMergeNodes([way.nodes[0], way.nodes[way.nodes.length-1]], nodes[0].loc),
+                        t('issues.fix.connect_endpoints.annotation')
+                    );
+                };
+            }
+        }
+
+        // if the points were not merged, attempt to close the way
+        testNodes = nodes.slice();   // shallow copy
+        testNodes.push(testNodes[0]);
+        // make sure this will not create a self-intersection
+        if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) {
+            return function(context) {
+                var wayId = this.issue.entityIds[0];
+                var way = context.entity(wayId);
+                var nodeId = way.nodes[0];
+                var index = way.nodes.length;
+                context.perform(
+                    actionAddVertex(wayId, nodeId, index),
+                    t('issues.fix.connect_endpoints.annotation')
+                );
+            };
+        }
+    }
+
+    function lineTaggedAsAreaIssue(entity, graph) {
+
+        var tagSuggestingArea = tagSuggestingLineIsArea(entity);
+        if (!tagSuggestingArea) return null;
+
+        var fixes = [];
+
+        var connectEndsOnClick = makeConnectEndpointsFixOnClick(entity, graph);
+
+        fixes.push(new validationIssueFix({
+            title: t('issues.fix.connect_endpoints.title'),
+            onClick: connectEndsOnClick
+        }));
+
+        fixes.push(new validationIssueFix({
+            icon: 'iD-operation-delete',
+            title: t('issues.fix.remove_tag.title'),
+            onClick: function(context) {
+                var entityId = this.issue.entityIds[0];
+                var entity = context.entity(entityId);
+                var tags = Object.assign({}, entity.tags);  // shallow copy
+                for (var key in tagSuggestingArea) {
+                    delete tags[key];
+                }
+                context.perform(
+                    actionChangeTags(entityId, tags),
+                    t('issues.fix.remove_tag.annotation')
+                );
+            }
+        }));
+
+        return new validationIssue({
+            type: type,
+            subtype: 'area_as_line',
+            severity: 'warning',
+            message: function(context) {
+                var entity = context.hasEntity(this.entityIds[0]);
+                return entity ? t('issues.tag_suggests_area.message', {
+                    feature: utilDisplayLabel(entity, context),
+                    tag: utilTagText({ tags: tagSuggestingArea })
+                }) : '';
+            },
+            reference: showReference,
+            entityIds: [entity.id],
+            hash: JSON.stringify(tagSuggestingArea) +
+                // avoid stale "connect endpoints" fix
+                (typeof connectEndsOnClick),
+            fixes: fixes
+        });
+
+
+        function showReference(selection) {
+            selection.selectAll('.issue-reference')
+                .data([0])
+                .enter()
+                .append('div')
+                .attr('class', 'issue-reference')
+                .text(t('issues.tag_suggests_area.reference'));
+        }
+    }
+
+    function vertexTaggedAsPointIssue(entity, graph) {
+        // we only care about nodes
+        if (entity.type !== 'node') return null;
+
+        // ignore tagless points
+        if (Object.keys(entity.tags).length === 0) return null;
+
+        // address lines are special so just ignore them
+        if (entity.isOnAddressLine(graph)) return null;
+
+        var geometry = entity.geometry(graph);
+        var allowedGeometries = osmNodeGeometriesForTags(entity.tags);
+
+        if (geometry === 'point' && !allowedGeometries.point && allowedGeometries.vertex) {
+
+            return new validationIssue({
+                type: type,
+                subtype: 'vertex_as_point',
+                severity: 'warning',
+                message: function(context) {
+                    var entity = context.hasEntity(this.entityIds[0]);
+                    return entity ? t('issues.vertex_as_point.message', {
+                        feature: utilDisplayLabel(entity, context)
+                    }) : '';
+                },
+                reference: function showReference(selection) {
+                    selection.selectAll('.issue-reference')
+                        .data([0])
+                        .enter()
+                        .append('div')
+                        .attr('class', 'issue-reference')
+                        .text(t('issues.vertex_as_point.reference'));
+                },
+                entityIds: [entity.id]
+            });
+
+        } else if (geometry === 'vertex' && !allowedGeometries.vertex && allowedGeometries.point) {
+
+            return new validationIssue({
+                type: type,
+                subtype: 'point_as_vertex',
+                severity: 'warning',
+                message: function(context) {
+                    var entity = context.hasEntity(this.entityIds[0]);
+                    return entity ? t('issues.point_as_vertex.message', {
+                        feature: utilDisplayLabel(entity, context)
+                    }) : '';
+                },
+                reference: function showReference(selection) {
+                    selection.selectAll('.issue-reference')
+                        .data([0])
+                        .enter()
+                        .append('div')
+                        .attr('class', 'issue-reference')
+                        .text(t('issues.point_as_vertex.reference'));
+                },
+                entityIds: [entity.id]
+            });
+        }
+
+        return null;
+    }
+
+    var validation = function checkMismatchedGeometry(entity, graph) {
+        var issues = [
+            vertexTaggedAsPointIssue(entity, graph),
+            lineTaggedAsAreaIssue(entity, graph)
+        ];
+        return issues.filter(Boolean);
+    };
+
+    validation.type = type;
+
+    return validation;
+}

modules/validations/tag_suggests_area.js

@@ -1,129 +0,0 @@
-import { actionAddVertex } from '../actions/add_vertex';
-import { actionChangeTags } from '../actions/change_tags';
-import { actionMergeNodes } from '../actions/merge_nodes';
-import { geoHasSelfIntersections, geoSphericalDistance } from '../geo';
-import { t } from '../util/locale';
-import { utilDisplayLabel, utilTagText } from '../util';
-import { validationIssue, validationIssueFix } from '../core/validation';
-
-
-export function validationTagSuggestsArea(context) {
-    var type = 'tag_suggests_area';
-
-
-    var validation = function checkTagSuggestsArea(entity, graph) {
-        if (entity.type !== 'way' || entity.isClosed()) return [];
-
-        var tagSuggestingArea = entity.tagSuggestingArea();
-        if (!tagSuggestingArea) {
-            return [];
-        }
-
-        if (context.presets().matchTags(tagSuggestingArea, 'line') ===
-            context.presets().matchTags(tagSuggestingArea, 'area')) {
-            // these tags also allow lines and making this an area wouldn't matter
-            return [];
-        }
-
-        var tagText = utilTagText({ tags: tagSuggestingArea });
-        var fixes = [];
-
-        var connectEndpointsOnClick;
-
-        // must have at least three nodes to close this automatically
-        if (entity.nodes.length >= 3) {
-            var nodes = graph.childNodes(entity), testNodes;
-            var firstToLastDistanceMeters = geoSphericalDistance(nodes[0].loc, nodes[nodes.length-1].loc);
-
-            // if the distance is very small, attempt to merge the endpoints
-            if (firstToLastDistanceMeters < 0.75) {
-                testNodes = nodes.slice();   // shallow copy
-                testNodes.pop();
-                testNodes.push(testNodes[0]);
-                // make sure this will not create a self-intersection
-                if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) {
-                    connectEndpointsOnClick = function(context) {
-                        var way = context.entity(this.issue.entityIds[0]);
-                        context.perform(
-                            actionMergeNodes([way.nodes[0], way.nodes[way.nodes.length-1]], nodes[0].loc),
-                            t('issues.fix.connect_endpoints.annotation')
-                        );
-                    };
-                }
-            }
-
-            if (!connectEndpointsOnClick) {
-                // if the points were not merged, attempt to close the way
-                testNodes = nodes.slice();   // shallow copy
-                testNodes.push(testNodes[0]);
-                // make sure this will not create a self-intersection
-                if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) {
-                    connectEndpointsOnClick = function(context) {
-                        var wayId = this.issue.entityIds[0];
-                        var way = context.entity(wayId);
-                        var nodeId = way.nodes[0];
-                        var index = way.nodes.length;
-                        context.perform(
-                            actionAddVertex(wayId, nodeId, index),
-                            t('issues.fix.connect_endpoints.annotation')
-                        );
-                    };
-                }
-            }
-        }
-
-        fixes.push(new validationIssueFix({
-            title: t('issues.fix.connect_endpoints.title'),
-            onClick: connectEndpointsOnClick
-        }));
-
-        fixes.push(new validationIssueFix({
-            icon: 'iD-operation-delete',
-            title: t('issues.fix.remove_tag.title'),
-            onClick: function(context) {
-                var entityId = this.issue.entityIds[0];
-                var entity = context.entity(entityId);
-                var tags = Object.assign({}, entity.tags);  // shallow copy
-                for (var key in tagSuggestingArea) {
-                    delete tags[key];
-                }
-                context.perform(
-                    actionChangeTags(entityId, tags),
-                    t('issues.fix.remove_tag.annotation')
-                );
-            }
-        }));
-
-        return [new validationIssue({
-            type: type,
-            severity: 'warning',
-            message: function(context) {
-                var entity = context.hasEntity(this.entityIds[0]);
-                return entity ? t('issues.tag_suggests_area.message', {
-                    feature: utilDisplayLabel(entity, context),
-                    tag: tagText
-                }) : '';
-            },
-            reference: showReference,
-            entityIds: [entity.id],
-            hash: JSON.stringify(tagSuggestingArea) +
-                // avoid stale "connect endpoints" fix
-                (typeof connectEndpointsOnClick),
-            fixes: fixes
-        })];
-
-
-        function showReference(selection) {
-            selection.selectAll('.issue-reference')
-                .data([0])
-                .enter()
-                .append('div')
-                .attr('class', 'issue-reference')
-                .text(t('issues.tag_suggests_area.reference'));
-        }
-    };
-
-    validation.type = type;
-
-    return validation;
-}

test/index.html

@@ -157,12 +157,12 @@
     <script src='spec/validations/disconnected_way.js'></script>
     <script src='spec/validations/generic_name.js'></script>
     <script src='spec/validations/incompatible_source.js'></script>
+    <script src='spec/validations/mismatched_geometry.js'></script>
     <script src='spec/validations/missing_role.js'></script>
     <script src='spec/validations/missing_tag.js'></script>
     <script src='spec/validations/old_multipolygon.js'></script>
     <script src='spec/validations/outdated_tags.js'></script>
     <script src='spec/validations/private_data.js'></script>
-    <script src='spec/validations/tag_suggests_area.js'></script>
 
     <script>
         window.mocha.run();

test/spec/validations/mismatched_geometry.js

@@ -1,4 +1,4 @@
-describe('iD.validations.tag_suggests_area', function () {
+describe('iD.validations.mismatched_geometry', function () {
     var context;
 
     beforeEach(function() {
@@ -41,7 +41,7 @@ describe('iD.validations.tag_suggests_area', function () {
     }
 
     function validate() {
-        var validator = iD.validationTagSuggestsArea(context);
+        var validator = iD.validationMismatchedGeometry(context);
         var changes = context.history().changes();
         var entities = changes.modified.concat(changes.created);
         var issues = [];
@@ -86,7 +86,8 @@ describe('iD.validations.tag_suggests_area', function () {
         var issues = validate();
         expect(issues).to.have.lengthOf(1);
         var issue = issues[0];
-        expect(issue.type).to.eql('tag_suggests_area');
+        expect(issue.type).to.eql('mismatched_geometry');
+        expect(issue.subtype).to.eql('area_as_line');
         expect(issue.severity).to.eql('warning');
         expect(issue.entityIds).to.have.lengthOf(1);
         expect(issue.entityIds[0]).to.eql('w-1');
@@ -97,7 +98,8 @@ describe('iD.validations.tag_suggests_area', function () {
         var issues = validate();
         expect(issues).to.have.lengthOf(1);
         var issue = issues[0];
-        expect(issue.type).to.eql('tag_suggests_area');
+        expect(issue.type).to.eql('mismatched_geometry');
+        expect(issue.subtype).to.eql('area_as_line');
         expect(issue.severity).to.eql('warning');
         expect(issue.entityIds).to.have.lengthOf(1);
         expect(issue.entityIds[0]).to.eql('w-1');