Merge pull request #2809 from openstreetmap/conflicting-tags

Disable joining ways with conflicting tags

data/core.yaml

@@ -114,6 +114,7 @@ en:
       not_adjacent: These lines can't be merged because they aren't connected.
       restriction: These lines can't be merged because at least one is a member of a "{relation}" relation.
       incomplete_relation: These features can't be merged because at least one hasn't been fully downloaded.
+      conflicting_tags: These lines can't be merged because some of their tags have conflicting values.
     move:
       title: Move
       description: Move this to a different location.

dist/locales/en.json

@@ -143,7 +143,8 @@
             "not_eligible": "These features can't be merged.",
             "not_adjacent": "These lines can't be merged because they aren't connected.",
             "restriction": "These lines can't be merged because at least one is a member of a \"{relation}\" relation.",
-            "incomplete_relation": "These features can't be merged because at least one hasn't been fully downloaded."
+            "incomplete_relation": "These features can't be merged because at least one hasn't been fully downloaded.",
+            "conflicting_tags": "These lines can't be merged because some of their tags have conflicting values."
         },
         "move": {
             "title": "Move",

index.html

@@ -226,7 +226,7 @@
         <script src='js/id/core/relation.js'></script>
         <script src='js/id/core/way.js'></script>
         <script src='js/id/core/tree.js'></script>
-        <script src='js/id/core/oneway_tags.js'></script>
+        <script src='js/id/core/tags.js'></script>
 
         <script src='js/id/presets.js'></script>
         <script src='js/id/presets/preset.js'></script>

js/id/actions/join.js

@@ -57,18 +57,31 @@ iD.actions.Join = function(ids) {
             return 'not_adjacent';
 
         var nodeIds = _.pluck(joined[0].nodes, 'id').slice(1, -1),
-            relation;
+            relation,
+            tags = {},
+            conflicting = false;
 
         joined[0].forEach(function(way) {
             var parents = graph.parentRelations(way);
             parents.forEach(function(parent) {
                 if (parent.isRestriction() && parent.members.some(function(m) { return nodeIds.indexOf(m.id) >= 0; }))
                     relation = parent;
             });
+
+            for (var k in way.tags) {
+                if (!(k in tags)) {
+                    tags[k] = way.tags[k];
+                } else if (tags[k] && iD.interestingTag(k) && tags[k] !== way.tags[k]) {
+                    conflicting = true;
+                }
+            }
         });
 
         if (relation)
             return 'restriction';
+
+        if (conflicting)
+            return 'conflicting_tags';
     };
 
     return action;

js/id/actions/reverse.js

@@ -29,7 +29,7 @@
       http://wiki.openstreetmap.org/wiki/Route#Members
       http://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java
  */
-iD.actions.Reverse = function(wayId) {
+iD.actions.Reverse = function(wayId, options) {
     var replacements = [
             [/:right$/, ':left'], [/:left$/, ':right'],
             [/:forward$/, ':backward'], [/:backward$/, ':forward']
@@ -59,6 +59,8 @@ iD.actions.Reverse = function(wayId) {
             return value.replace(numeric, function(_, sign) { return sign === '-' ? '' : '-'; });
         } else if (key === 'incline' || key === 'direction') {
             return {up: 'down', down: 'up'}[value] || value;
+        } else if (options && options.reverseOneway && key === 'oneway') {
+            return {yes: '-1', '1': '-1', '-1': 'yes'}[value] || value;
         } else {
             return {left: 'right', right: 'left'}[value] || value;
         }

js/id/core/entity.js

@@ -116,13 +116,7 @@ iD.Entity.prototype = {
     },
 
     hasInterestingTags: function() {
-        return _.keys(this.tags).some(function(key) {
-            return key !== 'attribution' &&
-                key !== 'created_by' &&
-                key !== 'source' &&
-                key !== 'odbl' &&
-                key.indexOf('tiger:') !== 0;
-        });
+        return _.keys(this.tags).some(iD.interestingTag);
     },
 
     isHighwayIntersection: function() {

js/id/core/oneway_tags.js → js/id/core/tags.js

@@ -29,3 +29,12 @@ iD.oneWayTags = {
         'stream': true
     }
 };
+
+iD.interestingTag = function (key) {
+    return key !== 'attribution' &&
+        key !== 'created_by' &&
+        key !== 'source' &&
+        key !== 'odbl' &&
+        key.indexOf('tiger:') !== 0;
+
+};

js/id/geo/multipolygon.js

@@ -81,7 +81,7 @@ iD.geo.joinWays = function(array, graph) {
     }
 
     function reverse(member) {
-        return member.tags ? iD.actions.Reverse(member.id)(graph).entity(member.id) : member;
+        return member.tags ? iD.actions.Reverse(member.id, {reverseOneway: true})(graph).entity(member.id) : member;
     }
 
     while (array.length) {

test/index.html

@@ -201,7 +201,7 @@
     <script src='../js/id/core/relation.js'></script>
     <script src='../js/id/core/way.js'></script>
     <script src='../js/id/core/tree.js'></script>
-    <script src='../js/id/core/oneway_tags.js'></script>
+    <script src='../js/id/core/tags.js'></script>
 
     <script src='../js/id/presets.js'></script>
     <script src='../js/id/presets/preset.js'></script>

test/spec/actions/join.js

@@ -194,6 +194,54 @@ describe("iD.actions.Join", function () {
 
             expect(iD.actions.Join(['-', '=']).disabled(graph)).not.to.be.ok;
         });
+
+        it("returns 'conflicting_tags' for two entities that have conflicting tags", function () {
+            var graph = iD.Graph([
+                iD.Node({id: 'a'}),
+                iD.Node({id: 'b'}),
+                iD.Node({id: 'c'}),
+                iD.Way({id: '-', nodes: ['a', 'b'], tags: {highway: 'primary'}}),
+                iD.Way({id: '=', nodes: ['b', 'c'], tags: {highway: 'secondary'}})
+            ]);
+
+            expect(iD.actions.Join(['-', '=']).disabled(graph)).to.equal('conflicting_tags');
+        });
+
+        it("takes tag reversals into account when calculating conflicts", function () {
+            var graph = iD.Graph([
+                iD.Node({id: 'a'}),
+                iD.Node({id: 'b'}),
+                iD.Node({id: 'c'}),
+                iD.Way({id: '-', nodes: ['a', 'b'], tags: {'oneway': 'yes'}}),
+                iD.Way({id: '=', nodes: ['c', 'b'], tags: {'oneway': '-1'}})
+            ]);
+
+            expect(iD.actions.Join(['-', '=']).disabled(graph)).not.to.be.ok;
+        });
+
+        it("returns falsy for exceptions to tag conflicts: missing tag", function () {
+            var graph = iD.Graph([
+                iD.Node({id: 'a'}),
+                iD.Node({id: 'b'}),
+                iD.Node({id: 'c'}),
+                iD.Way({id: '-', nodes: ['a', 'b'], tags: {highway: 'primary'}}),
+                iD.Way({id: '=', nodes: ['b', 'c'], tags: {}})
+            ]);
+
+            expect(iD.actions.Join(['-', '=']).disabled(graph)).not.to.be.ok;
+        });
+
+        it("returns falsy for exceptions to tag conflicts: uninteresting tag", function () {
+            var graph = iD.Graph([
+                iD.Node({id: 'a'}),
+                iD.Node({id: 'b'}),
+                iD.Node({id: 'c'}),
+                iD.Way({id: '-', nodes: ['a', 'b'], tags: {'tiger:cfcc': 'A41'}}),
+                iD.Way({id: '=', nodes: ['b', 'c'], tags: {'tiger:cfcc': 'A42'}})
+            ]);
+
+            expect(iD.actions.Join(['-', '=']).disabled(graph)).not.to.be.ok;
+        });
     });
 
     it("joins a --> b ==> c", function () {

test/spec/actions/reverse.js

@@ -23,6 +23,24 @@ describe("iD.actions.Reverse", function () {
         expect(graph.entity(way.id).tags).to.eql({'oneway': 'yes'});
     });
 
+    it("reverses oneway tags if reverseOneway: true is provided", function () {
+        var graph = iD.Graph([
+                iD.Way({id: 'yes', tags: {oneway: 'yes'}}),
+                iD.Way({id: 'no', tags: {oneway: 'no'}}),
+                iD.Way({id: '1', tags: {oneway: '1'}}),
+                iD.Way({id: '-1', tags: {oneway: '-1'}})
+            ]);
+
+        expect(iD.actions.Reverse('yes', {reverseOneway: true})(graph)
+            .entity('yes').tags).to.eql({oneway: '-1'});
+        expect(iD.actions.Reverse('no', {reverseOneway: true})(graph)
+            .entity('no').tags).to.eql({oneway: 'no'});
+        expect(iD.actions.Reverse('1', {reverseOneway: true})(graph)
+            .entity('1').tags).to.eql({oneway: '-1'});
+        expect(iD.actions.Reverse('-1', {reverseOneway: true})(graph)
+            .entity('-1').tags).to.eql({oneway: 'yes'});
+    });
+
     it("transforms *:right=* ⟺ *:left=*", function () {
         var way = iD.Way({tags: {'cycleway:right': 'lane'}}),
             graph = iD.Graph([way]);

test/spec/geo/multipolygon.js

@@ -86,11 +86,11 @@ describe("iD.geo.joinWays", function() {
                 iD.Node({id: 'b'}),
                 iD.Node({id: 'c'}),
                 iD.Way({id: '-', nodes: ['a', 'b']}),
-                iD.Way({id: '=', nodes: ['c', 'b'], tags: {'lanes:forward': 2}})
+                iD.Way({id: '=', nodes: ['c', 'b'], tags: {'oneway': 'yes', 'lanes:forward': 2}})
             ]);
 
         var result = iD.geo.joinWays([graph.entity('-'), graph.entity('=')], graph);
-        expect(result[0][1].tags).to.eql({'lanes:backward': 2});
+        expect(result[0][1].tags).to.eql({'oneway': '-1', 'lanes:backward': 2});
     });
 
     it("ignores non-way members", function() {