diff --git a/CHANGELOG.md b/CHANGELOG.md index be3006965..0218a0772 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ _Breaking developer changes, which may affect downstream projects or sites that * Render `*_link` roads narrower than their "regular" counterpart ([#10722]) #### :scissors: Operations * Fix splitting of closed ways (or areas) when two or more split-points are selected +* Keep `red_turn:left/right` tags unchanged when reversing a way ([#10737], thanks [@burrscurr]) #### :camera: Street-Level #### :white_check_mark: Validation * Add warning if aeroways cross each other, buildings or highways ([#9315], thanks [@k-yle]) @@ -71,6 +72,7 @@ _Breaking developer changes, which may affect downstream projects or sites that [#10720]: https://github.com/openstreetmap/iD/issues/10720 [#10722]: https://github.com/openstreetmap/iD/pull/10722 [#10727]: https://github.com/openstreetmap/iD/issues/10727 +[#10737]: https://github.com/openstreetmap/iD/pull/10737 [#10747]: https://github.com/openstreetmap/iD/issues/10747 [#10748]: https://github.com/openstreetmap/iD/issues/10748 [#10755]: https://github.com/openstreetmap/iD/issues/10755 @@ -80,6 +82,7 @@ _Breaking developer changes, which may affect downstream projects or sites that [@hlfan]: https://github.com/hlfan [@Deeptanshu-sankhwar]: https://github.com/Deeptanshu-sankhwar [@draunger]: https://github.com/draunger +[@burrscurr]: https://github.com/burrscurr # 2.31.1 diff --git a/modules/actions/reverse.js b/modules/actions/reverse.js index bb9703692..711cd4961 100644 --- a/modules/actions/reverse.js +++ b/modules/actions/reverse.js @@ -18,10 +18,8 @@ References: http://wiki.openstreetmap.org/wiki/Key:traffic_sign#On_a_way_or_area */ export function actionReverse(entityID, options) { - var ignoreKey = /^.*(_|:)?(description|name|note|website|ref|source|comment|watch|attribution)(_|:)?/; var numeric = /^([+\-]?)(?=[\d.])/; var directionKey = /direction$/; - var turn_lanes = /^turn:lanes:?/; var keyReplacements = [ [/:right$/, ':left'], [/:left$/, ':right'], @@ -42,13 +40,27 @@ export function actionReverse(entityID, options) { forwards: 'backward', backwards: 'forward', }; - // tags whose values should not be reversed when certain other tags are also present - // https://github.com/openstreetmap/iD/issues/10128 - const valueReplacementsExceptions = { - 'side': [ - {highway: 'cyclist_waiting_aid'} - ] - }; + // For some tags, keys or values like left/right/… don't refer to + // way direction and thus should not be reversed. + const keysToKeepUnchanged = [ + // https://github.com/openstreetmap/iD/issues/10736 + /^red_turn:(right|left):?/ + ]; + // If a key matches the key regex and any of the provided context + // tag sets, it will not be reversed. + const keyValuesToKeepUnchanged = [{ + keyRegex: /^.*(_|:)?(description|name|note|website|ref|source|comment|watch|attribution)(_|:)?/, + prerequisiteTags: [{}] + }, { + // Turn lanes are left/right to key (not way) direction - #5674 + keyRegex: /^turn:lanes:?/, + prerequisiteTags: [{}] + }, { + // https://github.com/openstreetmap/iD/issues/10128 + keyRegex: /^side$/, + prerequisiteTags: [{highway: 'cyclist_waiting_aid'}] + } + ]; var roleReplacements = { forward: 'backward', backward: 'forward', @@ -82,6 +94,9 @@ export function actionReverse(entityID, options) { function reverseKey(key) { + if (keysToKeepUnchanged.some(keyRegex => keyRegex.test(key))) { + return key; + } for (var i = 0; i < keyReplacements.length; ++i) { var replacement = keyReplacements[i]; if (replacement[0].test(key)) { @@ -93,13 +108,17 @@ export function actionReverse(entityID, options) { function reverseValue(key, value, includeAbsolute, allTags) { - if (ignoreKey.test(key)) return value; + for (let { keyRegex, prerequisiteTags } of keyValuesToKeepUnchanged) { + if (keyRegex.test(key) && prerequisiteTags.some(expectedTags => + Object.entries(expectedTags).every(([k, v]) => { + return allTags[k] && (v === '*' || allTags[k] === v); + }) + )) { + return value; + } + } - // Turn lanes are left/right to key (not way) direction - #5674 - if (turn_lanes.test(key)) { - return value; - - } else if (key === 'incline' && numeric.test(value)) { + if (key === 'incline' && numeric.test(value)) { return value.replace(numeric, function(_, sign) { return sign === '-' ? '' : '-'; }); } else if (options && options.reverseOneway && key === 'oneway') { @@ -122,17 +141,6 @@ export function actionReverse(entityID, options) { } }).join(';'); } - - if (valueReplacementsExceptions[key] && valueReplacementsExceptions[key].some(exceptionTags => - Object.keys(exceptionTags).every(k => { - const v = exceptionTags[k]; - return allTags[k] && (v === '*' || allTags[k] === v); - }) - )) { - // don't reverse, for example, side=left/right on highway=cyclist_waiting_aid features - // see https://github.com/openstreetmap/iD/issues/10128 - return value; - } return valueReplacements[value] || value; } diff --git a/test/spec/actions/reverse.js b/test/spec/actions/reverse.js index 58d70c993..3e0be2244 100644 --- a/test/spec/actions/reverse.js +++ b/test/spec/actions/reverse.js @@ -15,6 +15,7 @@ describe('iD.actionReverse', function () { expect(graph.entity(way.id).tags).to.eql({'highway': 'residential'}); }); + describe('reverses directional tags on nodes', function () { it('reverses relative directions', function () { var node1 = iD.osmNode({ tags: { 'direction': 'forward' } }); @@ -95,6 +96,7 @@ describe('iD.actionReverse', function () { }); }); + describe('reverses oneway', function () { it('preserves oneway tags', function () { var way = iD.osmWay({tags: {'oneway': 'yes'}}); @@ -588,6 +590,47 @@ describe('iD.actionReverse', function () { var target = graph.entity(node2.id); expect(target.tags['traffic_signals:direction']).to.eql('empty'); }); + }); + + + describe('does not reverse values which are relative to another reversed tag', function () { + it('preserves the turn direction of a single lane road', function () { + var way = iD.osmWay({tags: {'turn:lanes': 'right'}}); + var graph = iD.actionReverse(way.id)(iD.coreGraph([way])); + var target = graph.entity(way.id); + expect(target.tags['turn:lanes']).to.eql('right'); + }); + + it('preserves the turn directions of a multi-lane road', function () { + var way = iD.osmWay({tags: {'turn:lanes': 'through|through|right'}}); + var graph = iD.actionReverse(way.id)(iD.coreGraph([way])); + var target = graph.entity(way.id); + expect(target.tags['turn:lanes']).to.eql('through|through|right'); + }); + + // https://github.com/openstreetmap/iD/issues/5674 + it('preserves the turn direction of each direction with a single lane', function () { + var way = iD.osmWay({tags: {'turn:lanes:forward': 'right', 'turn:lanes:backward': 'left'}}); + var graph = iD.actionReverse(way.id)(iD.coreGraph([way])); + var target = graph.entity(way.id); + expect(target.tags['turn:lanes:backward']).to.eql('right'); + expect(target.tags['turn:lanes:forward']).to.eql('left'); + }); + + it('preserves the turn direction of each direction with multiple lanes', function () { + var way = iD.osmWay({tags: {'turn:lanes:forward': 'through|right', 'turn:lanes:backward': 'through|through|left'}}); + var graph = iD.actionReverse(way.id)(iD.coreGraph([way])); + var target = graph.entity(way.id); + expect(target.tags['turn:lanes:backward']).to.eql('through|right'); + expect(target.tags['turn:lanes:forward']).to.eql('through|through|left'); + }); + + it('preserves the turn direction of explicitly bidirectional turn lane values', function () { + var way = iD.osmWay({tags: {'turn:lanes:both_ways': 'left'}}); + var graph = iD.actionReverse(way.id)(iD.coreGraph([way])); + var target = graph.entity(way.id); + expect(target.tags['turn:lanes:both_ways']).to.eql('left'); + }); it('preserves the value of the side tag of a cycling waiting aid', function () { var node1 = iD.osmNode(); @@ -603,5 +646,22 @@ describe('iD.actionReverse', function () { expect(target.tags.direction).to.eql('backward'); expect(target.tags.side).to.eql('right'); }); + + it('preserves the direction of a red turn at a traffic signal', function () { + var node1 = iD.osmNode(); + var node2 = iD.osmNode({tags: { + 'highway': 'traffic_signals', + 'traffic_signals:direction': 'forward', + 'red_turn:right': 'no', + 'red_turn:right:bicycle': 'yes' + }}); + var node3 = iD.osmNode(); + var way = iD.osmWay({nodes: [node1.id, node2.id, node3.id]}); + var graph = iD.actionReverse(way.id)(iD.coreGraph([node1, node2, node3, way])); + var target = graph.entity(node2.id); + expect(target.tags['traffic_signals:direction']).to.eql('backward'); + expect(target.tags['red_turn:right']).to.eql('no'); + expect(target.tags['red_turn:right:bicycle']).to.eql('yes'); + }); }); });