Index: /trunk/src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java =================================================================== --- /trunk/src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java (revision 5786) +++ /trunk/src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java (revision 5787) @@ -5,5 +5,4 @@ import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; @@ -18,4 +17,6 @@ import org.openstreetmap.josm.data.osm.Relation; import org.openstreetmap.josm.data.osm.RelationMember; +import org.openstreetmap.josm.data.osm.Tag; +import org.openstreetmap.josm.data.osm.TagCollection; import org.openstreetmap.josm.data.osm.Way; @@ -28,10 +29,35 @@ * for the user to confirm. */ - public class ReverseWayTagCorrector extends TagCorrector { - private static class PrefixSuffixSwitcher { - - private static final String SEPARATOR = "[:_]"; + private static final String SEPARATOR = "[:_]"; + + private static final Pattern getPatternFor(String s) { + return getPatternFor(s, false); + } + + private static final Pattern getPatternFor(String s, boolean exactMatch) { + if (exactMatch) { + return Pattern.compile("(^)(" + s + ")($)"); + } else { + return Pattern.compile("(^|.*" + SEPARATOR + ")(" + s + ")(" + SEPARATOR + ".*|$)", + Pattern.CASE_INSENSITIVE); + } + } + + private static final Collection ignoredKeys = new ArrayList(); + static { + for (String s : OsmPrimitive.getUninterestingKeys()) { + ignoredKeys.add(getPatternFor(s)); + } + for (String s : new String[]{"name", "ref", "tiger:county"}) { + ignoredKeys.add(getPatternFor(s, false)); + } + for (String s : new String[]{"tiger:county", "turn:lanes", "change:lanes", "placement"}) { + ignoredKeys.add(getPatternFor(s, true)); + } + } + + private static class StringSwitcher { private final String a; @@ -39,10 +65,8 @@ private final Pattern pattern; - public PrefixSuffixSwitcher(String a, String b) { + public StringSwitcher(String a, String b) { this.a = a; this.b = b; - this.pattern = Pattern.compile( - "(^|.*" + SEPARATOR + ")(" + a + "|" + b + ")(" + SEPARATOR + ".*|$)", - Pattern.CASE_INSENSITIVE); + this.pattern = getPatternFor(a + "|" + b); } @@ -64,66 +88,27 @@ } - private static final PrefixSuffixSwitcher[] prefixSuffixSwitchers = - new PrefixSuffixSwitcher[] { - new PrefixSuffixSwitcher("left", "right"), - new PrefixSuffixSwitcher("forward", "backward"), - new PrefixSuffixSwitcher("forwards", "backwards"), - new PrefixSuffixSwitcher("up", "down"), - new PrefixSuffixSwitcher("east", "west"), - new PrefixSuffixSwitcher("north", "south"), - }; - /** - * Tests whether way can be reversed without semantic change, i.e., whether tags have to be changed. - * Looks for keys like oneway, oneway:bicycle, cycleway:right:oneway, left/right. - * @param way - * @return false if tags should be changed to keep semantic, true otherwise. + * Reverses a given tag. + * @since 5787 */ - public static boolean isReversible(Way way) { - for (String key : way.keySet()) { - for (String k : Arrays.asList("oneway", "incline", "direction")) { - if (key.startsWith(k) || key.endsWith(k)) { - return false; - } - } - String value = way.get(key); - for (PrefixSuffixSwitcher prefixSuffixSwitcher : prefixSuffixSwitchers) { - if (!key.equals(prefixSuffixSwitcher.apply(key)) || !value.equals(prefixSuffixSwitcher.apply(value))) { - return false; - } - } - } - return true; - } - - public static List irreversibleWays(List ways) { - List newWays = new ArrayList(ways); - for (Way way : ways) { - if (isReversible(way)) { - newWays.remove(way); - } - } - return newWays; - } - - public String invertNumber(String value) { - Pattern pattern = Pattern.compile("^([+-]?)(\\d.*)$", Pattern.CASE_INSENSITIVE); - Matcher matcher = pattern.matcher(value); - if (!matcher.matches()) return value; - String sign = matcher.group(1); - String rest = matcher.group(2); - sign = sign.equals("-") ? "" : "-"; - return sign + rest; - } - - @Override - public Collection execute(Way oldway, Way way) throws UserCancelException { - Map> tagCorrectionsMap = - new HashMap>(); - - ArrayList tagCorrections = new ArrayList(); - for (String key : way.keySet()) { + public static class TagSwitcher { + + /** + * Reverses a given tag. + * @param tag The tag to reverse + * @return The reversed tag (is equal to tag if no change is needed) + */ + public static final Tag apply(final Tag tag) { + return apply(tag.getKey(), tag.getValue()); + } + + /** + * Reverses a given tag (key=value). + * @param key The tag key + * @param value The tag value + * @return The reversed tag (is equal to key=value if no change is needed) + */ + public static final Tag apply(final String key, final String value) { String newKey = key; - String value = way.get(key); String newValue = value; @@ -136,11 +121,14 @@ } else if (key.startsWith("incline") || key.endsWith("incline") || key.startsWith("direction") || key.endsWith("direction")) { - PrefixSuffixSwitcher switcher = new PrefixSuffixSwitcher("up", "down"); - newValue = switcher.apply(value); + newValue = UP_DOWN.apply(value); if (newValue.equals(value)) { newValue = invertNumber(value); } - } else if (!ignoreKeyForPrefixSuffixCorrection(key)) { - for (PrefixSuffixSwitcher prefixSuffixSwitcher : prefixSuffixSwitchers) { + } else if (key.endsWith(":forward") || key.endsWith(":backward")) { + // Change key but not left/right value (fix #8518) + newKey = FORWARD_BACKWARD.apply(key); + + } else if (!ignoreKeyForCorrection(key)) { + for (StringSwitcher prefixSuffixSwitcher : stringSwitchers) { newKey = prefixSuffixSwitcher.apply(key); if (!key.equals(newKey)) { @@ -153,4 +141,65 @@ } } + return new Tag(newKey, newValue); + } + } + + private static final StringSwitcher FORWARD_BACKWARD = new StringSwitcher("forward", "backward"); + private static final StringSwitcher UP_DOWN = new StringSwitcher("up", "down"); + + private static final StringSwitcher[] stringSwitchers = new StringSwitcher[] { + new StringSwitcher("left", "right"), + new StringSwitcher("forwards", "backwards"), + new StringSwitcher("east", "west"), + new StringSwitcher("north", "south"), + FORWARD_BACKWARD, UP_DOWN + }; + + /** + * Tests whether way can be reversed without semantic change, i.e., whether tags have to be changed. + * Looks for keys like oneway, oneway:bicycle, cycleway:right:oneway, left/right. + * @param way + * @return false if tags should be changed to keep semantic, true otherwise. + */ + public static boolean isReversible(Way way) { + for (Tag tag : TagCollection.from(way)) { + if (!tag.equals(TagSwitcher.apply(tag))) { + return false; + } + } + return true; + } + + public static List irreversibleWays(List ways) { + List newWays = new ArrayList(ways); + for (Way way : ways) { + if (isReversible(way)) { + newWays.remove(way); + } + } + return newWays; + } + + public static String invertNumber(String value) { + Pattern pattern = Pattern.compile("^([+-]?)(\\d.*)$", Pattern.CASE_INSENSITIVE); + Matcher matcher = pattern.matcher(value); + if (!matcher.matches()) return value; + String sign = matcher.group(1); + String rest = matcher.group(2); + sign = sign.equals("-") ? "" : "-"; + return sign + rest; + } + + @Override + public Collection execute(Way oldway, Way way) throws UserCancelException { + Map> tagCorrectionsMap = + new HashMap>(); + + ArrayList tagCorrections = new ArrayList(); + for (String key : way.keySet()) { + String value = way.get(key); + Tag newTag = TagSwitcher.apply(key, value); + String newKey = newTag.getKey(); + String newValue = newTag.getValue(); boolean needsCorrection = !key.equals(newKey); @@ -190,5 +239,5 @@ boolean found = false; String newRole = null; - for (PrefixSuffixSwitcher prefixSuffixSwitcher : prefixSuffixSwitchers) { + for (StringSwitcher prefixSuffixSwitcher : stringSwitchers) { newRole = prefixSuffixSwitcher.apply(member.getRole()); if (!newRole.equals(member.getRole())) { @@ -215,7 +264,11 @@ } - private static boolean ignoreKeyForPrefixSuffixCorrection(String key) { - return key.contains("name") || key.equals("tiger:county") - || key.equalsIgnoreCase("fixme") || key.startsWith("note"); + private static boolean ignoreKeyForCorrection(String key) { + for (Pattern ignoredKey : ignoredKeys) { + if (ignoredKey.matcher(key).matches()) { + return true; + } + } + return false; } } Index: /trunk/test/unit/org/openstreetmap/josm/corrector/ReverseWayTagCorrectorTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/corrector/ReverseWayTagCorrectorTest.java (revision 5787) +++ /trunk/test/unit/org/openstreetmap/josm/corrector/ReverseWayTagCorrectorTest.java (revision 5787) @@ -0,0 +1,99 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.corrector; + +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; +import org.openstreetmap.josm.Main; +import org.openstreetmap.josm.data.Preferences; +import org.openstreetmap.josm.data.osm.Tag; + +/** + * Unit tests of {@link ReverseWayTagCorrector} class. + */ +public class ReverseWayTagCorrectorTest { + + /** + * Setup test. + */ + @BeforeClass + public static void setUp() { + Main.pref = new Preferences(); + } + + /** + * Test of {@link ReverseWayTagCorrector.TagSwitcher#apply} method. + */ + @Test + public void testTagSwitch() { + // oneway + assertSwitch(new Tag("oneway", "yes"), new Tag("oneway", "-1")); + assertSwitch(new Tag("oneway", "true"), new Tag("oneway", "-1")); + assertSwitch(new Tag("oneway", "-1"), new Tag("oneway", "yes")); + assertSwitch(new Tag("oneway", "no"), new Tag("oneway", "no")); + assertSwitch(new Tag("oneway", "something"), new Tag("oneway", "something")); + // incline/direction + for (String k : new String[]{"incline", "direction"}) { + assertSwitch(new Tag(k, "up"), new Tag(k, "down")); + assertSwitch(new Tag(k, "down"), new Tag(k, "up")); + assertSwitch(new Tag(k, "something"), new Tag(k, "something")); + } + // :forward/:backward (see #8518) + assertSwitch(new Tag("turn:forward", "right"), new Tag("turn:backward", "right")); + assertSwitch(new Tag("change:forward", "not_right"), new Tag("change:backward", "not_right")); + assertSwitch(new Tag("placement:forward", "right_of:1"), new Tag("placement:backward", "right_of:1")); + assertSwitch(new Tag("turn:lanes:forward", "left|right"), new Tag("turn:lanes:backward", "left|right")); + assertSwitch(new Tag("change:lanes:forward", "not_right|only_left"), new Tag("change:lanes:backward", "not_right|only_left")); + // keys + assertSwitch(new Tag("forward", "something"), new Tag("backward", "something")); + assertSwitch(new Tag("backward", "something"), new Tag("forward", "something")); + assertSwitch(new Tag("up", "something"), new Tag("down", "something")); + assertSwitch(new Tag("down", "something"), new Tag("up", "something")); + assertSwitch(new Tag("east", "something"), new Tag("west", "something")); + assertSwitch(new Tag("west", "something"), new Tag("east", "something")); + assertSwitch(new Tag("south", "something"), new Tag("north", "something")); + assertSwitch(new Tag("north", "something"), new Tag("south", "something")); + // values + assertSwitch(new Tag("something", "forward"), new Tag("something", "backward")); + assertSwitch(new Tag("something", "backward"), new Tag("something", "forward")); + assertSwitch(new Tag("something", "up"), new Tag("something", "down")); + assertSwitch(new Tag("something", "down"), new Tag("something", "up")); + assertSwitch(new Tag("something", "east"), new Tag("something", "west")); + assertSwitch(new Tag("something", "west"), new Tag("something", "east")); + assertSwitch(new Tag("something", "south"), new Tag("something", "north")); + assertSwitch(new Tag("something", "north"), new Tag("something", "south")); + // value[:_]suffix + assertSwitch(new Tag("something", "forward:suffix"), new Tag("something", "backward:suffix")); + assertSwitch(new Tag("something", "backward_suffix"), new Tag("something", "forward_suffix")); + assertSwitch(new Tag("something", "up:suffix"), new Tag("something", "down:suffix")); + assertSwitch(new Tag("something", "down_suffix"), new Tag("something", "up_suffix")); + assertSwitch(new Tag("something", "east:suffix"), new Tag("something", "west:suffix")); + assertSwitch(new Tag("something", "west_suffix"), new Tag("something", "east_suffix")); + assertSwitch(new Tag("something", "south:suffix"), new Tag("something", "north:suffix")); + assertSwitch(new Tag("something", "north_suffix"), new Tag("something", "south_suffix")); + // prefix[:_]value + assertSwitch(new Tag("something", "prefix:forward"), new Tag("something", "prefix:backward")); + assertSwitch(new Tag("something", "prefix_backward"), new Tag("something", "prefix_forward")); + assertSwitch(new Tag("something", "prefix:up"), new Tag("something", "prefix:down")); + assertSwitch(new Tag("something", "prefix_down"), new Tag("something", "prefix_up")); + assertSwitch(new Tag("something", "prefix:east"), new Tag("something", "prefix:west")); + assertSwitch(new Tag("something", "prefix_west"), new Tag("something", "prefix_east")); + assertSwitch(new Tag("something", "prefix:south"), new Tag("something", "prefix:north")); + assertSwitch(new Tag("something", "prefix_north"), new Tag("something", "prefix_south")); + // prefix[:_]value[:_]suffix + assertSwitch(new Tag("something", "prefix:forward:suffix"), new Tag("something", "prefix:backward:suffix")); + assertSwitch(new Tag("something", "prefix_backward:suffix"), new Tag("something", "prefix_forward:suffix")); + assertSwitch(new Tag("something", "prefix:up_suffix"), new Tag("something", "prefix:down_suffix")); + assertSwitch(new Tag("something", "prefix_down_suffix"), new Tag("something", "prefix_up_suffix")); + assertSwitch(new Tag("something", "prefix:east:suffix"), new Tag("something", "prefix:west:suffix")); + assertSwitch(new Tag("something", "prefix_west:suffix"), new Tag("something", "prefix_east:suffix")); + assertSwitch(new Tag("something", "prefix:south_suffix"), new Tag("something", "prefix:north_suffix")); + assertSwitch(new Tag("something", "prefix_north_suffix"), new Tag("something", "prefix_south_suffix")); + // #8499 + assertSwitch(new Tag("type", "drawdown"), new Tag("type", "drawdown")); + } + + private void assertSwitch(Tag oldTag, Tag newTag) { + Assert.assertEquals(ReverseWayTagCorrector.TagSwitcher.apply(oldTag), newTag); + } +}