From 44a064f8db716e7d356abbf9a054d19074533e9d Mon Sep 17 00:00:00 2001 From: simonpoole Date: Mon, 26 Feb 2024 14:29:51 +0100 Subject: [PATCH] Add support for exception to the normal tag reversing behaviour This allows per key exceptions to the usual way/node tag reversal rules by specifying the key and a JosmFilter expression that has to match. See https://github.com/openstreetmap/iD/issues/10128 --- .../java/de/blau/android/osm/Reverse.java | 85 +++++++++++++------ src/main/java/de/blau/android/osm/Tags.java | 69 ++++++++------- .../java/de/blau/android/search/Wrapper.java | 35 +++++++- .../java/de/blau/android/osm/ReverseTest.java | 24 ++++++ .../de/blau/android/search/WrapperTest.java | 15 +++- 5 files changed, 167 insertions(+), 61 deletions(-) diff --git a/src/main/java/de/blau/android/osm/Reverse.java b/src/main/java/de/blau/android/osm/Reverse.java index b20db903be..240a4ef560 100644 --- a/src/main/java/de/blau/android/osm/Reverse.java +++ b/src/main/java/de/blau/android/osm/Reverse.java @@ -1,8 +1,10 @@ package de.blau.android.osm; +import java.io.ByteArrayInputStream; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Locale; @@ -14,6 +16,10 @@ import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import ch.poole.osm.josmfilterparser.Condition; +import ch.poole.osm.josmfilterparser.JosmFilterParseException; +import ch.poole.osm.josmfilterparser.JosmFilterParser; +import de.blau.android.search.Wrapper; /** * Logic for reversing direction dependent tags, this is one of the more arcane things about the OSM data model @@ -22,7 +28,8 @@ * */ final class Reverse { - private static final String DEBUG_TAG = "Reverse"; + + private static final String DEBUG_TAG = Reverse.class.getSimpleName(); private static final String LEFT_INFIX = ":left:"; private static final String RIGHT_INFIX = ":right:"; @@ -40,6 +47,15 @@ final class Reverse { private static Set directionDependentValues = Collections .unmodifiableSet(new HashSet<>(Arrays.asList(Tags.VALUE_RIGHT, Tags.VALUE_LEFT, Tags.VALUE_FORWARD, Tags.VALUE_BACKWARD))); + private static Map reverseExceptions = new HashMap<>(); + static { + try { + reverseExceptions.put(Tags.KEY_SIDE, compilePattern(Tags.KEY_HIGHWAY + "=" + Tags.VALUE_CYCLIST_WAITING_AID)); + } catch (JosmFilterParseException e) { + Log.e(DEBUG_TAG, e.getMessage()); + } + } + /** * Private constructor */ @@ -51,33 +67,43 @@ private Reverse() { * Return the direction dependent tags and associated values oneway, *:left, *:right, *:backward, *:forward from the * element * - * Probably we should check for issues with relation membership too - * * @param e element that we extract the direction dependent tags from * @return map containing the tags */ - @Nullable + @NonNull public static Map getDirectionDependentTags(@NonNull OsmElement e) { - Map result = null; + Map result = new TreeMap<>(); Map tags = e.getTags(); - if (tags != null) { - for (Entry entry : tags.entrySet()) { - String key = entry.getKey(); - String value = entry.getValue(); - if ((Tags.KEY_HIGHWAY.equals(key) && (Tags.VALUE_MOTORWAY.equals(value) || Tags.VALUE_MOTORWAY_LINK.equals(value))) - || directionDependentKeys.contains(key) || key.endsWith(LEFT_POSTFIX) || key.endsWith(RIGHT_POSTFIX) || key.endsWith(BACKWARD_POSTFIX) - || key.endsWith(FORWARD_POSTFIX) || key.contains(FORWARD_INFIX) || key.contains(BACKWARD_INFIX) || key.contains(RIGHT_INFIX) - || key.contains(LEFT_INFIX) || directionDependentValues.contains(value)) { - if (result == null) { - result = new TreeMap<>(); - } - result.put(key, value); - } + for (Entry entry : tags.entrySet()) { + String key = entry.getKey(); + String value = entry.getValue(); + if (((Tags.KEY_HIGHWAY.equals(key) && (Tags.VALUE_MOTORWAY.equals(value) || Tags.VALUE_MOTORWAY_LINK.equals(value))) + || directionDependentKeys.contains(key) || key.endsWith(LEFT_POSTFIX) || key.endsWith(RIGHT_POSTFIX) || key.endsWith(BACKWARD_POSTFIX) + || key.endsWith(FORWARD_POSTFIX) || key.contains(FORWARD_INFIX) || key.contains(BACKWARD_INFIX) || key.contains(RIGHT_INFIX) + || key.contains(LEFT_INFIX) || directionDependentValues.contains(value)) && !matchExceptions(e, key)) { + result.put(key, value); } } return result; } + /** + * Check if a key is in the exceptions map and the object matches the pattern + * + * @param e the OsmElement + * @param key the key to check + * @return true if this is an exception we should not reverse + */ + private static boolean matchExceptions(@NonNull OsmElement e, @NonNull String key) { + Condition c = reverseExceptions.get(key); + if (c != null) { + Wrapper meta = new Wrapper(); + meta.setElement(e); + return c.eval(Wrapper.toJosmFilterType(e), meta, e.getTags()); + } + return false; + } + /** * Return a list of (route) relations that the element is a member of with a direction dependent role * @@ -232,9 +258,9 @@ private static String reverseIncline(@NonNull final String value) { * @return reversed value */ private static String reverseOneway(@NonNull final String value) { - if (Tags.VALUE_YES.equalsIgnoreCase(value) || Tags.VALUE_TRUE.equalsIgnoreCase(value) || "1".equals(value)) { - return "-1"; - } else if (Tags.VALUE_REVERSE.equalsIgnoreCase(value) || "-1".equals(value)) { + if (Tags.VALUE_YES.equalsIgnoreCase(value) || Tags.VALUE_TRUE.equalsIgnoreCase(value) || Tags.VALUE_ONE.equals(value)) { + return Tags.VALUE_MINUS_ONE; + } else if (Tags.VALUE_REVERSE.equalsIgnoreCase(value) || Tags.VALUE_MINUS_ONE.equals(value)) { return Tags.VALUE_YES; } return value; @@ -256,7 +282,7 @@ public static void reverseDirectionDependentTags(@NonNull OsmElement e, @NonNull } Map tags = new TreeMap<>(e.getTags()); - // remove all dir dependent key first + // remove all dir dependent keys first for (String key : dirTags.keySet()) { tags.remove(key); } @@ -271,7 +297,7 @@ public static void reverseDirectionDependentTags(@NonNull OsmElement e, @NonNull if (reverseOneway && Tags.KEY_HIGHWAY.equals(key) && (Tags.VALUE_MOTORWAY.equals(value) || Tags.VALUE_MOTORWAY_LINK.equals(value))) { tags.put(key, value); // don't have to change anything if (!dirTags.containsKey(Tags.KEY_ONEWAY)) { - tags.put(Tags.KEY_ONEWAY, "-1"); + tags.put(Tags.KEY_ONEWAY, Tags.VALUE_MINUS_ONE); } continue; } @@ -329,7 +355,7 @@ public static void reverseDirectionDependentTags(@NonNull OsmElement e, @NonNull continue; } if (Tags.VALUE_FORWARD.equals(value)) { - tags.put(key, "backward"); + tags.put(key, Tags.VALUE_BACKWARD); continue; } if (Tags.VALUE_BACKWARD.equals(value)) { @@ -359,4 +385,15 @@ private static String floatToString(float f) { return String.format(Locale.US, "%s", f); } } + + /** + * Compile a JosmFilter pattern + * + * @param pattern the pattern to compile + * @return the compiled pattern in a Condition object + * @throws JosmFilterParseException if parsing fails + */ + private static Condition compilePattern(@NonNull String pattern) throws JosmFilterParseException { + return new JosmFilterParser(new ByteArrayInputStream(pattern.getBytes())).condition(false); + } } diff --git a/src/main/java/de/blau/android/osm/Tags.java b/src/main/java/de/blau/android/osm/Tags.java index 8c20a9e132..c11342d790 100644 --- a/src/main/java/de/blau/android/osm/Tags.java +++ b/src/main/java/de/blau/android/osm/Tags.java @@ -126,38 +126,43 @@ public static boolean isLikeAName(@NonNull String key) { return false; } - public static final String KEY_NAME_LEFT = "name:left"; - public static final String KEY_NAME_RIGHT = "name:right"; - public static final String KEY_REF = "ref"; - public static final String KEY_NONAME = "noname"; - public static final String KEY_VALIDATE_NO_NAME = "validate:no_name"; - public static final String KEY_NOREF = "noref"; - public static final String KEY_HIGHWAY = "highway"; - public static final String VALUE_ROAD = "road"; - public static final String VALUE_MOTORWAY = "motorway"; - public static final String VALUE_MOTORWAY_LINK = "motorway_link"; - public static final String VALUE_FOOTWAY = "footway"; - public static final String VALUE_CYCLEWAY = "cycleway"; - public static final String VALUE_PATH = "path"; - public static final String VALUE_STEPS = "steps"; - public static final String VALUE_TRACK = "track"; - public static final String KEY_TRACKTYPE = "tracktype"; - public static final String KEY_SIDEWALK = "sidewalk"; - public static final String KEY_BRIDGE = "bridge"; - public static final String KEY_TUNNEL = "tunnel"; - public static final String VALUE_CULVERT = "culvert"; - public static final String KEY_BARRIER = "barrier"; - public static final String VALUE_RETAINING_WALL = "retaining_wall"; - public static final String VALUE_KERB = "kerb"; - public static final String VALUE_GUARD_RAIL = "guard_rail"; - public static final String VALUE_CITY_WALL = "city_wall"; - public static final String KEY_TWO_SIDED = "two_sided"; - public static final String KEY_MAN_MADE = "man_made"; - public static final String VALUE_EMBANKMENT = "embankment"; - public static final String KEY_ONEWAY = "oneway"; - public static final String VALUE_REVERSE = "reverse"; - public static final String KEY_WATERWAY = "waterway"; - public static final String VALUE_RIVERBANK = "riverbank"; + public static final String KEY_NAME_LEFT = "name:left"; + public static final String KEY_NAME_RIGHT = "name:right"; + public static final String KEY_REF = "ref"; + public static final String KEY_NONAME = "noname"; + public static final String KEY_VALIDATE_NO_NAME = "validate:no_name"; + public static final String KEY_NOREF = "noref"; + public static final String KEY_HIGHWAY = "highway"; + public static final String VALUE_ROAD = "road"; + public static final String VALUE_MOTORWAY = "motorway"; + public static final String VALUE_MOTORWAY_LINK = "motorway_link"; + public static final String VALUE_FOOTWAY = "footway"; + public static final String VALUE_CYCLEWAY = "cycleway"; + public static final String VALUE_PATH = "path"; + public static final String VALUE_STEPS = "steps"; + public static final String VALUE_TRACK = "track"; + public static final String KEY_TRACKTYPE = "tracktype"; + public static final String KEY_SIDEWALK = "sidewalk"; + public static final String KEY_BRIDGE = "bridge"; + public static final String KEY_TUNNEL = "tunnel"; + public static final String VALUE_CULVERT = "culvert"; + public static final String KEY_BARRIER = "barrier"; + public static final String VALUE_RETAINING_WALL = "retaining_wall"; + public static final String VALUE_KERB = "kerb"; + public static final String VALUE_GUARD_RAIL = "guard_rail"; + public static final String VALUE_CITY_WALL = "city_wall"; + public static final String KEY_TWO_SIDED = "two_sided"; + public static final String KEY_MAN_MADE = "man_made"; + public static final String VALUE_EMBANKMENT = "embankment"; + public static final String KEY_ONEWAY = "oneway"; + public static final String VALUE_REVERSE = "reverse"; + public static final String VALUE_MINUS_ONE = "-1"; + public static final String VALUE_ONE = "1"; + public static final String VALUE_CYCLIST_WAITING_AID = "cyclist_waiting_aid"; + public static final String KEY_SIDE = "side"; + + public static final String KEY_WATERWAY = "waterway"; + public static final String VALUE_RIVERBANK = "riverbank"; public static final String KEY_LANDUSE = "landuse"; public static final String VALUE_CONSTRUCTION = "construction"; diff --git a/src/main/java/de/blau/android/search/Wrapper.java b/src/main/java/de/blau/android/search/Wrapper.java index e8bf2d32b2..8a2fc167ea 100644 --- a/src/main/java/de/blau/android/search/Wrapper.java +++ b/src/main/java/de/blau/android/search/Wrapper.java @@ -13,6 +13,7 @@ import android.content.Context; import androidx.annotation.NonNull; +import androidx.annotation.Nullable; import ch.poole.osm.josmfilterparser.Condition; import ch.poole.osm.josmfilterparser.ElementState.State; import ch.poole.osm.josmfilterparser.Meta; @@ -48,12 +49,20 @@ public class Wrapper implements Meta { final Context context; final Logic logic; + /** + * Create a new wrapper object + * + */ + public Wrapper() { + this(null); + } + /** * Create a new wrapper object * * @param context an Android Context */ - public Wrapper(@NonNull Context context) { + public Wrapper(@Nullable Context context) { this.context = context; this.logic = App.getLogic(); } @@ -93,7 +102,18 @@ public static Type toJosmFilterType(@NonNull OsmElement element) { @Override public String getUser() { - throw new IllegalArgumentException(context.getString(R.string.search_objects_unsupported, "user")); + throw unsupported("user"); + } + + /** + * Construct an IllegalArgumentException + * + * @param expression the unsupported expression + * @return an IllegalArgumentException + */ + private IllegalArgumentException unsupported(@NonNull String expression) { + return new IllegalArgumentException( + context != null ? context.getString(R.string.search_objects_unsupported, expression) : "Unsupported expression \"" + expression + "\""); } @Override @@ -108,7 +128,7 @@ public long getVersion() { @Override public long getChangeset() { - throw new IllegalArgumentException(context.getString(R.string.search_objects_unsupported, "changeset")); + throw unsupported("changeset"); } @Override @@ -176,7 +196,7 @@ private int getMemberTypeCount(@NonNull String type) { @Override public int getAreaSize() { - throw new IllegalArgumentException(context.getString(R.string.search_objects_unsupported, "areasize")); + throw unsupported("areasize"); } @Override @@ -227,6 +247,9 @@ public boolean hasRole(@NonNull String role) { @Override public Object getPreset(@NonNull String presetPath) { + if (context == null) { + throw unsupported("preset:"); + } if (presetPath.endsWith("*")) { presetPath = presetPath.substring(0, presetPath.length() - 1); } @@ -412,6 +435,7 @@ public boolean isEmpty() { * @param c the Condition to check * @return a SearchResult object */ + @NonNull SearchResult getMatchingElementsInternal(@NonNull Condition c) { StorageDelegator delegator = App.getDelegator(); SearchResult result = new SearchResult(); @@ -439,6 +463,9 @@ SearchResult getMatchingElementsInternal(@NonNull Condition c) { @Override public @NotNull Meta wrap(Object arg0) { + if (context == null) { + throw unsupported("unknown"); + } Wrapper wrapper = new Wrapper(context); wrapper.setElement((OsmElement) arg0); return wrapper; diff --git a/src/test/java/de/blau/android/osm/ReverseTest.java b/src/test/java/de/blau/android/osm/ReverseTest.java index 0a1bd6e3a4..bac1d2ee62 100644 --- a/src/test/java/de/blau/android/osm/ReverseTest.java +++ b/src/test/java/de/blau/android/osm/ReverseTest.java @@ -1,6 +1,7 @@ package de.blau.android.osm; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.util.HashMap; @@ -33,6 +34,29 @@ public void reverse() { reverseTag("priority","backward","forward"); } + /** + * Test that way reversing has the intended effects + */ + @Test + public void reverseSide() { + Node e = OsmElementFactory.createNode(-1L, 1L, System.currentTimeMillis() / 1000, OsmElement.STATE_CREATED, 0, 0); + Map tags = new HashMap<>(); + tags.put("traffic_sign", "stop"); + tags.put(Tags.KEY_SIDE, Tags.VALUE_RIGHT); + e.setTags(tags); + Map dirTags = Reverse.getDirectionDependentTags(e); + assertTrue(dirTags.containsKey(Tags.KEY_SIDE)); + Reverse.reverseDirectionDependentTags(e, dirTags, true); + assertTrue(e.hasTagWithValue(Tags.KEY_SIDE, Tags.VALUE_LEFT)); + // + tags = new HashMap<>(); + tags.put(Tags.KEY_HIGHWAY, Tags.VALUE_CYCLIST_WAITING_AID); + tags.put(Tags.KEY_SIDE, Tags.VALUE_RIGHT); + e.setTags(tags); + dirTags = Reverse.getDirectionDependentTags(e); + assertFalse(dirTags.containsKey(Tags.KEY_SIDE)); + } + /** * Get the tag value changed by assuming that the way was reversed * diff --git a/src/test/java/de/blau/android/search/WrapperTest.java b/src/test/java/de/blau/android/search/WrapperTest.java index 098ede44df..65ec9c304a 100644 --- a/src/test/java/de/blau/android/search/WrapperTest.java +++ b/src/test/java/de/blau/android/search/WrapperTest.java @@ -3,7 +3,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -287,6 +286,20 @@ public void getPresetTest() { assertEquals("Streets", ((PresetGroup) streets).getName()); } + /** + * getPreset should fail if no Context is supplied + */ + @Test + public void getPresetTes2t() { + Wrapper wrapper2 = new Wrapper(); + try { + wrapper2.getPreset("Highways|Streets|Secondary"); + fail("Should have thrown an IllegalArgumentException"); + } catch (IllegalArgumentException iaex) { + // expected + } + } + /** * Check matchesPreset */