From 12b1af7002050c6d6c50273b77233c751c980eac Mon Sep 17 00:00:00 2001 From: Quincy Morgan Date: Mon, 25 Feb 2019 12:42:39 -0500 Subject: [PATCH] Don't say tags imply area for tags that iD doesn't have a preset for (close #5933) --- data/presets/presets.json | 8 +- data/presets/presets/_amenity.json | 3 +- data/presets/presets/_leisure.json | 1 + data/presets/presets/_natural.json | 1 + data/presets/presets/_tourism.json | 1 + data/taginfo.json | 8 +- modules/osm/way.js | 2 +- modules/presets/index.js | 65 +++++----- modules/presets/preset.js | 6 +- modules/validations/tag_suggests_area.js | 150 ++++++++++++----------- test/spec/presets/preset.js | 10 +- 11 files changed, 133 insertions(+), 122 deletions(-) diff --git a/data/presets/presets.json b/data/presets/presets.json index 5d7eebdfd..85246732a 100644 --- a/data/presets/presets.json +++ b/data/presets/presets.json @@ -2,22 +2,22 @@ "presets": { "aerialway": {"fields": ["aerialway"], "geometry": ["point", "vertex", "line"], "tags": {"aerialway": "*"}, "searchable": false, "name": "Aerialway"}, "aeroway": {"icon": "maki-airport", "fields": ["aeroway"], "geometry": ["point", "vertex", "line", "area"], "tags": {"aeroway": "*"}, "searchable": false, "name": "Aeroway"}, - "amenity": {"fields": ["amenity"], "geometry": ["point", "vertex", "area"], "tags": {"amenity": "*"}, "searchable": false, "name": "Amenity"}, + "amenity": {"fields": ["amenity"], "geometry": ["point", "vertex", "line", "area"], "tags": {"amenity": "*"}, "searchable": false, "name": "Amenity"}, "attraction": {"icon": "maki-star", "fields": ["name", "attraction", "operator", "opening_hours"], "moreFields": ["opening_hours", "fee", "payment_multi", "address", "website", "phone", "email", "fax"], "geometry": ["point", "vertex", "line", "area"], "tags": {"attraction": "*"}, "searchable": false, "name": "Attraction"}, "boundary": {"fields": ["boundary"], "geometry": ["line"], "tags": {"boundary": "*"}, "searchable": false, "name": "Boundary"}, "circular": {"geometry": ["vertex", "line"], "fields": ["name"], "tags": {"junction": "circular"}, "name": "Traffic Circle", "searchable": false}, "embankment": {"geometry": ["line"], "tags": {"embankment": "yes"}, "name": "Embankment", "matchScore": 0.2, "searchable": false}, "highway": {"fields": ["name", "highway"], "geometry": ["point", "vertex", "line", "area"], "tags": {"highway": "*"}, "searchable": false, "name": "Highway"}, "landuse": {"fields": ["name", "landuse"], "geometry": ["area"], "tags": {"landuse": "*"}, "matchScore": 0.9, "searchable": false, "name": "Land Use"}, - "leisure": {"icon": "maki-pitch", "fields": ["name", "leisure"], "geometry": ["point", "vertex", "area"], "tags": {"leisure": "*"}, "searchable": false, "name": "Leisure"}, + "leisure": {"icon": "maki-pitch", "fields": ["name", "leisure"], "geometry": ["point", "vertex", "line", "area"], "tags": {"leisure": "*"}, "searchable": false, "name": "Leisure"}, "man_made": {"icon": "temaki-storage_tank", "fields": ["name", "man_made"], "moreFields": ["material"], "geometry": ["point", "vertex", "line", "area"], "tags": {"man_made": "*"}, "searchable": false, "name": "Man Made"}, - "natural": {"icon": "maki-natural", "fields": ["name", "natural"], "geometry": ["point", "vertex", "area"], "tags": {"natural": "*"}, "searchable": false, "name": "Natural"}, + "natural": {"icon": "maki-natural", "fields": ["name", "natural"], "geometry": ["point", "vertex", "line", "area"], "tags": {"natural": "*"}, "searchable": false, "name": "Natural"}, "place": {"fields": ["name", "place"], "geometry": ["point", "vertex", "area"], "tags": {"place": "*"}, "searchable": false, "name": "Place"}, "power": {"geometry": ["point", "vertex", "line", "area"], "tags": {"power": "*"}, "fields": ["power"], "moreFields": ["material"], "searchable": false, "name": "Power"}, "railway": {"fields": ["railway"], "geometry": ["point", "vertex", "line", "area"], "tags": {"railway": "*"}, "searchable": false, "name": "Railway"}, "roundabout": {"geometry": ["vertex", "line"], "fields": ["name"], "tags": {"junction": "roundabout"}, "name": "Roundabout", "searchable": false}, "seamark": {"icon": "maki-harbor", "fields": ["seamark/type"], "geometry": ["point", "vertex", "line", "area"], "tags": {"seamark:type": "*"}, "searchable": false, "name": "Seamark"}, - "tourism": {"icon": "maki-attraction", "fields": ["name", "tourism"], "geometry": ["point", "vertex", "area"], "tags": {"tourism": "*"}, "searchable": false, "name": "Tourism"}, + "tourism": {"icon": "maki-attraction", "fields": ["name", "tourism"], "geometry": ["point", "vertex", "line", "area"], "tags": {"tourism": "*"}, "searchable": false, "name": "Tourism"}, "waterway": {"fields": ["name", "waterway"], "geometry": ["point", "vertex", "line", "area"], "tags": {"waterway": "*"}, "searchable": false, "name": "Waterway"}, "address": {"fields": ["address"], "geometry": ["point", "vertex", "area"], "tags": {"addr:*": "*"}, "addTags": {}, "removeTags": {}, "reference": {"key": "addr"}, "name": "Address", "matchScore": 0.15}, "advertising/billboard": {"fields": ["direction", "lit"], "geometry": ["point", "vertex", "line"], "tags": {"advertising": "billboard"}, "name": "Billboard"}, diff --git a/data/presets/presets/_amenity.json b/data/presets/presets/_amenity.json index 58737aacb..cf77ac75c 100644 --- a/data/presets/presets/_amenity.json +++ b/data/presets/presets/_amenity.json @@ -5,6 +5,7 @@ "geometry": [ "point", "vertex", + "line", "area" ], "tags": { @@ -12,4 +13,4 @@ }, "searchable": false, "name": "Amenity" -} \ No newline at end of file +} diff --git a/data/presets/presets/_leisure.json b/data/presets/presets/_leisure.json index 09991718d..bf86b6520 100644 --- a/data/presets/presets/_leisure.json +++ b/data/presets/presets/_leisure.json @@ -7,6 +7,7 @@ "geometry": [ "point", "vertex", + "line", "area" ], "tags": { diff --git a/data/presets/presets/_natural.json b/data/presets/presets/_natural.json index 9f9264728..13ad81113 100644 --- a/data/presets/presets/_natural.json +++ b/data/presets/presets/_natural.json @@ -7,6 +7,7 @@ "geometry": [ "point", "vertex", + "line", "area" ], "tags": { diff --git a/data/presets/presets/_tourism.json b/data/presets/presets/_tourism.json index f03692da3..298cb1a30 100644 --- a/data/presets/presets/_tourism.json +++ b/data/presets/presets/_tourism.json @@ -7,6 +7,7 @@ "geometry": [ "point", "vertex", + "line", "area" ], "tags": { diff --git a/data/taginfo.json b/data/taginfo.json index ff4c9a3ed..664efc5f1 100644 --- a/data/taginfo.json +++ b/data/taginfo.json @@ -5,22 +5,22 @@ "tags": [ {"key": "aerialway", "description": "🄿 Aerialway (unsearchable), 🄵 Type", "object_types": ["node", "way"]}, {"key": "aeroway", "description": "🄿 Aeroway (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/airport-15.svg?sanitize=true"}, - {"key": "amenity", "description": "🄿 Amenity (unsearchable), 🄵 Type", "object_types": ["node", "area"]}, + {"key": "amenity", "description": "🄿 Amenity (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"]}, {"key": "attraction", "description": "🄿 Attraction (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/star-15.svg?sanitize=true"}, {"key": "boundary", "description": "🄿 Boundary (unsearchable), 🄵 Type", "object_types": ["way"]}, {"key": "junction", "value": "circular", "description": "🄿 Traffic Circle (unsearchable), 🄵 Junction", "object_types": ["node", "way"]}, {"key": "embankment", "value": "yes", "description": "🄿 Embankment (unsearchable)", "object_types": ["way"]}, {"key": "highway", "description": "🄿 Highway (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"]}, {"key": "landuse", "description": "🄿 Land Use (unsearchable), 🄵 Type", "object_types": ["area"]}, - {"key": "leisure", "description": "🄿 Leisure (unsearchable), 🄵 Type", "object_types": ["node", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/pitch-15.svg?sanitize=true"}, + {"key": "leisure", "description": "🄿 Leisure (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/pitch-15.svg?sanitize=true"}, {"key": "man_made", "description": "🄿 Man Made (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/bhousel/temaki/master/icons/storage_tank.svg?sanitize=true"}, - {"key": "natural", "description": "🄿 Natural (unsearchable), 🄵 Natural", "object_types": ["node", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/natural-15.svg?sanitize=true"}, + {"key": "natural", "description": "🄿 Natural (unsearchable), 🄵 Natural", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/natural-15.svg?sanitize=true"}, {"key": "place", "description": "🄿 Place (unsearchable), 🄵 Type", "object_types": ["node", "area"]}, {"key": "power", "description": "🄿 Power (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"]}, {"key": "railway", "description": "🄿 Railway (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"]}, {"key": "junction", "value": "roundabout", "description": "🄿 Roundabout (unsearchable), 🄵 Junction", "object_types": ["node", "way"]}, {"key": "seamark:type", "description": "🄿 Seamark (unsearchable), 🄵 Seamark", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/harbor-15.svg?sanitize=true"}, - {"key": "tourism", "description": "🄿 Tourism (unsearchable), 🄵 Type", "object_types": ["node", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/attraction-15.svg?sanitize=true"}, + {"key": "tourism", "description": "🄿 Tourism (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"], "icon_url": "https://raw.githubusercontent.com/mapbox/maki/master/icons/attraction-15.svg?sanitize=true"}, {"key": "waterway", "description": "🄿 Waterway (unsearchable), 🄵 Type", "object_types": ["node", "way", "area"]}, {"key": "addr:*", "description": "🄿 Address", "object_types": ["node", "area"]}, {"key": "advertising", "value": "billboard", "description": "🄿 Billboard", "object_types": ["node", "way"]}, diff --git a/modules/osm/way.js b/modules/osm/way.js index f53a69b89..82fdfd3c5 100644 --- a/modules/osm/way.js +++ b/modules/osm/way.js @@ -193,7 +193,7 @@ _extend(osmWay.prototype, { return true; }, - // returns an objects with the tag that implies this is an area, if any + // returns an object with the tag that implies this is an area, if any tagSuggestingArea: function() { if (this.tags.area === 'yes') return { area: 'yes' }; diff --git a/modules/presets/index.js b/modules/presets/index.js index a66e35b91..44a3694d6 100644 --- a/modules/presets/index.js +++ b/modules/presets/index.js @@ -40,44 +40,49 @@ export function presetIndex() { all.match = function(entity, resolver) { return resolver.transient(entity, 'presetMatch', function() { var geometry = entity.geometry(resolver); - var address; // Treat entities on addr:interpolation lines as points, not vertices - #3241 if (geometry === 'vertex' && entity.isOnAddressLine(resolver)) { geometry = 'point'; } - var geometryMatches = _index[geometry]; - var best = -1; - var match; - - for (var k in entity.tags) { - // If any part of an address is present, - // allow fallback to "Address" preset - #4353 - if (/^addr:/.test(k) && geometryMatches['addr:*']) { - address = geometryMatches['addr:*'][0]; - } - - var keyMatches = geometryMatches[k]; - if (!keyMatches) continue; - - for (var i = 0; i < keyMatches.length; i++) { - var score = keyMatches[i].matchScore(entity); - if (score > best) { - best = score; - match = keyMatches[i]; - } - } - - } - - if (address && (!match || match.isFallback())) { - match = address; - } - return match || all.item(geometry); + return all.matchTags(entity.tags, geometry); }); }; + all.matchTags = function(tags, geometry) { + + var address; + var geometryMatches = _index[geometry]; + var best = -1; + var match; + + for (var k in tags) { + // If any part of an address is present, + // allow fallback to "Address" preset - #4353 + if (/^addr:/.test(k) && geometryMatches['addr:*']) { + address = geometryMatches['addr:*'][0]; + } + + var keyMatches = geometryMatches[k]; + if (!keyMatches) continue; + + for (var i = 0; i < keyMatches.length; i++) { + var score = keyMatches[i].matchScore(tags); + if (score > best) { + best = score; + match = keyMatches[i]; + } + } + + } + + if (address && (!match || match.isFallback())) { + match = address; + } + return match || all.item(geometry); + }; + all.allowsVertex = function(entity, resolver) { if (entity.type !== 'node') return false; if (_isEmpty(entity.tags)) return true; @@ -93,7 +98,7 @@ export function presetIndex() { didFindMatches = true; for (var i = 0; i < keyMatches.length; i++) { var preset = keyMatches[i]; - if (preset.searchable !== false && preset.matchScore(entity) > -1) { + if (preset.searchable !== false && preset.matchScore(entity.tags) > -1) { return preset; } } diff --git a/modules/presets/preset.js b/modules/presets/preset.js index a35796163..33e67fb55 100644 --- a/modules/presets/preset.js +++ b/modules/presets/preset.js @@ -111,14 +111,14 @@ export function presetPreset(id, preset, fields, visible, rawPresets) { preset.originalScore = preset.matchScore || 1; - preset.matchScore = function(entity) { + preset.matchScore = function(entityTags) { var tags = preset.tags; var score = 0; for (var t in tags) { - if (entity.tags[t] === tags[t]) { + if (entityTags[t] === tags[t]) { score += preset.originalScore; - } else if (tags[t] === '*' && t in entity.tags) { + } else if (tags[t] === '*' && t in entityTags) { score += preset.originalScore / 2; } else { return -1; diff --git a/modules/validations/tag_suggests_area.js b/modules/validations/tag_suggests_area.js index 709f314cd..98507be33 100644 --- a/modules/validations/tag_suggests_area.js +++ b/modules/validations/tag_suggests_area.js @@ -12,93 +12,95 @@ export function validationTagSuggestsArea() { var validation = function(entity, context) { - if (entity.type !== 'way') return []; + if (entity.type !== 'way' || entity.isClosed()) return []; - var issues = []; - var graph = context.graph(); var tagSuggestingArea = entity.tagSuggestingArea(); - var tagSuggestsArea = !entity.isClosed() && tagSuggestingArea; + if (!tagSuggestingArea) { + return []; + } - if (tagSuggestsArea) { - var tagText = utilTagText({ tags: tagSuggestingArea }); - var fixes = []; + if (context.presets().matchTags(entity.tags, 'line') === + context.presets().matchTags(entity.tags, 'area')) { + // the preset allows lines and making this an area wouldn't matter + return []; + } - var connectEndpointsOnClick; + var tagText = utilTagText({ tags: tagSuggestingArea }); + var fixes = []; - // 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); + var connectEndpointsOnClick; - // if the distance is very small, attempt to merge the endpoints - if (firstToLastDistanceMeters < 0.75) { - testNodes = _clone(nodes); - testNodes.pop(); - testNodes.push(testNodes[0]); - // make sure this will not create a self-intersection - if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) { - connectEndpointsOnClick = function() { - var way = this.issue.entities[0]; - context.perform( - actionMergeNodes([way.nodes[0], way.nodes[way.nodes.length-1]], nodes[0].loc), - t('issues.fix.connect_endpoints.annotation') - ); - }; - } - } + // must have at least three nodes to close this automatically + if (entity.nodes.length >= 3) { + var nodes = context.graph().childNodes(entity), testNodes; + var firstToLastDistanceMeters = geoSphericalDistance(nodes[0].loc, nodes[nodes.length-1].loc); - if (!connectEndpointsOnClick) { - // if the points were not merged, attempt to close the way - testNodes = _clone(nodes); - testNodes.push(testNodes[0]); - // make sure this will not create a self-intersection - if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) { - connectEndpointsOnClick = function() { - var way = this.issue.entities[0]; - var nodeId = way.nodes[0]; - var index = way.nodes.length; - context.perform( - actionAddVertex(way.id, nodeId, index), - t('issues.fix.connect_endpoints.annotation') - ); - }; - } + // if the distance is very small, attempt to merge the endpoints + if (firstToLastDistanceMeters < 0.75) { + testNodes = _clone(nodes); + testNodes.pop(); + testNodes.push(testNodes[0]); + // make sure this will not create a self-intersection + if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) { + connectEndpointsOnClick = function() { + var way = this.issue.entities[0]; + context.perform( + actionMergeNodes([way.nodes[0], way.nodes[way.nodes.length-1]], nodes[0].loc), + 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() { - var entity = this.issue.entities[0]; - var tags = _clone(entity.tags); - for (var key in tagSuggestingArea) { - delete tags[key]; - } - context.perform( - actionChangeTags(entity.id, tags), - t('issues.fix.remove_tag.annotation') - ); + if (!connectEndpointsOnClick) { + // if the points were not merged, attempt to close the way + testNodes = _clone(nodes); + testNodes.push(testNodes[0]); + // make sure this will not create a self-intersection + if (!geoHasSelfIntersections(testNodes, testNodes[0].id)) { + connectEndpointsOnClick = function() { + var way = this.issue.entities[0]; + var nodeId = way.nodes[0]; + var index = way.nodes.length; + context.perform( + actionAddVertex(way.id, nodeId, index), + t('issues.fix.connect_endpoints.annotation') + ); + }; } - })); - - var featureLabel = utilDisplayLabel(entity, context); - issues.push(new validationIssue({ - type: type, - severity: 'warning', - message: t('issues.tag_suggests_area.message', { feature: featureLabel, tag: tagText }), - tooltip: t('issues.tag_suggests_area.tip'), - entities: [entity], - fixes: fixes - })); + } } - return issues; + 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() { + var entity = this.issue.entities[0]; + var tags = _clone(entity.tags); + for (var key in tagSuggestingArea) { + delete tags[key]; + } + context.perform( + actionChangeTags(entity.id, tags), + t('issues.fix.remove_tag.annotation') + ); + } + })); + + var featureLabel = utilDisplayLabel(entity, context); + return [new validationIssue({ + type: type, + severity: 'warning', + message: t('issues.tag_suggests_area.message', { feature: featureLabel, tag: tagText }), + tooltip: t('issues.tag_suggests_area.tip'), + entities: [entity], + fixes: fixes + })]; }; validation.type = type; diff --git a/test/spec/presets/preset.js b/test/spec/presets/preset.js index af2dbfeb4..56f5a9641 100644 --- a/test/spec/presets/preset.js +++ b/test/spec/presets/preset.js @@ -20,29 +20,29 @@ describe('iD.presetPreset', function() { it('returns -1 if preset does not match tags', function() { var preset = iD.presetPreset('test', {tags: {foo: 'bar'}}); var entity = iD.osmWay({tags: {highway: 'motorway'}}); - expect(preset.matchScore(entity)).to.equal(-1); + expect(preset.matchScore(entity.tags)).to.equal(-1); }); it('returns the value of the matchScore property when matched', function() { var preset = iD.presetPreset('test', {tags: {highway: 'motorway'}, matchScore: 0.2}); var entity = iD.osmWay({tags: {highway: 'motorway'}}); - expect(preset.matchScore(entity)).to.equal(0.2); + expect(preset.matchScore(entity.tags)).to.equal(0.2); }); it('defaults to the number of matched tags', function() { var preset = iD.presetPreset('test', {tags: {highway: 'residential'}}); var entity = iD.osmWay({tags: {highway: 'residential'}}); - expect(preset.matchScore(entity)).to.equal(1); + expect(preset.matchScore(entity.tags)).to.equal(1); preset = iD.presetPreset('test', {tags: {highway: 'service', service: 'alley'}}); entity = iD.osmWay({tags: {highway: 'service', service: 'alley'}}); - expect(preset.matchScore(entity)).to.equal(2); + expect(preset.matchScore(entity.tags)).to.equal(2); }); it('counts * as a match for any value with score 0.5', function() { var preset = iD.presetPreset('test', {tags: {building: '*'}}); var entity = iD.osmWay({tags: {building: 'yep'}}); - expect(preset.matchScore(entity)).to.equal(0.5); + expect(preset.matchScore(entity.tags)).to.equal(0.5); }); });