Changeset 6538 in josm

Timestamp:

2013-12-26T15:19:31+01:00 (10 years ago)

Author:

simon04

Message:

see #9414 - MapCSS-based tagchecker: provide {i.key}, {i.value}, {i.tag} variables to messages/fixes which refer to the corresponding match condition

Location:

trunk

Files:

• data/validator/relation.mapcss (modified) (1 diff)
• src/org/openstreetmap/josm/command/ChangePropertyCommand.java (modified) (1 diff)
• src/org/openstreetmap/josm/command/Command.java (modified) (1 diff)
• src/org/openstreetmap/josm/data/validation/TestError.java (modified) (1 diff)
• src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (modified) (6 diffs)
• src/org/openstreetmap/josm/gui/mappaint/mapcss/Condition.java (modified) (3 diffs)
• src/org/openstreetmap/josm/gui/mappaint/mapcss/ExpressionFactory.java (modified) (2 diffs)
• src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java (modified) (3 diffs)
• src/org/openstreetmap/josm/tools/Utils.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (modified) (5 diffs)

trunk/data/validator/relation.mapcss

@@ -8 +8 @@
 
 /* from http://wiki.openstreetmap.org/wiki/Types_of_relation */
-relation[type=route][!route] {
-  throwWarning: tr("{0} relation without {0} tag", "route");
+/* see also #9071 */
+relation[type=route][!route],
+relation[type=route_master][!route_master],
+relation[type=restriction][!restriction],
+relation[type=boundary][!boundary],
+relation[type=site][!site],
+relation[type=public_transport][!public_transport],
+relation[type=waterway][!waterway],
+relation[type=enforcement][!enforcement] {
+  throwWarning: tr("{0} relation without {0} tag", "{1.key}");
   assertMatch: "relation type=route";
   assertNoMatch: "relation type=route route=train";
-}
-
-relation[type=route_master][!route_master] {
-  /* see #9071 */
-  throwWarning: tr("{0} relation without {0} tag", "route_master");
   assertMatch: "relation type=route_master";
   assertNoMatch: "relation type=route_master route_master=train";
-}
-
-relation[type=restriction][!restriction] {
-  throwWarning: tr("{0} relation without {0} tag", "restriction");
   assertMatch: "relation type=restriction";
   assertNoMatch: "relation type=restriction restriction=no_left_turn";
-}
-
-relation[type=boundary][!boundary] {
-  throwWarning: tr("{0} relation without {0} tag", "boundary");
   assertMatch: "relation type=boundary";
   assertNoMatch: "relation type=boundary boundary=administrative";
-}
-
-relation[type=site][!site] {
-  throwWarning: tr("{0} relation without {0} tag", "site");
   assertMatch: "relation type=site";
   assertNoMatch: "relation type=site site=administrative";
-}
-
-relation[type=public_transport][!public_transport] {
-  throwWarning: tr("{0} relation without {0} tag", "public_transport");
   assertMatch: "relation type=public_transport";
   assertNoMatch: "relation type=public_transport public_transport=stop_area";
-}
-
-relation[type=waterway][!waterway] {
-  throwWarning: tr("{0} relation without {0} tag", "waterway");
   assertMatch: "relation type=waterway";
   assertNoMatch: "relation type=waterway waterway=river";
-}
-
-relation[type=enforcement][!enforcement] {
-  throwWarning: tr("{0} relation without {0} tag", "enforcement");
   assertMatch: "relation type=enforcement";
   assertNoMatch: "relation type=enforcement enforcement=maxspeed";

trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java

@@ -221 +221 @@
         return children;
     }
+
+    public Map<String, String> getTags() {
+        return Collections.unmodifiableMap(tags);
+    }
 }

trunk/src/org/openstreetmap/josm/command/Command.java

@@ -86 +86 @@
      */
     public Command() {
-        this.layer = Main.main.getEditLayer();
+        this.layer = Main.main == null ? null : Main.main.getEditLayer();
     }
 

trunk/src/org/openstreetmap/josm/data/validation/TestError.java

@@ -219 +219 @@
     }
 
+    public void setTester(Test tester) {
+        this.tester = tester;
+    }
+
     /**
      * Gets the code

trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java

@@ -14 +14 @@
 import java.util.List;
 import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import org.openstreetmap.josm.command.ChangePropertyCommand;
@@ -29 +31 @@
 import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.gui.mappaint.Environment;
+import org.openstreetmap.josm.gui.mappaint.mapcss.Condition;
 import org.openstreetmap.josm.gui.mappaint.mapcss.Expression;
 import org.openstreetmap.josm.gui.mappaint.mapcss.Instruction;
@@ -173 +176 @@
          */
         boolean matchesPrimitive(OsmPrimitive primitive) {
+            return whichSelectorMatchesPrimitive(primitive) != null;
+        }
+
+        Selector whichSelectorMatchesPrimitive(OsmPrimitive primitive) {
             final Environment env = new Environment().withPrimitive(primitive);
             for (Selector i : selector) {
                 if (i.matches(env)) {
-                    return true;
-                }
-            }
-            return false;
+                    return i;
+                }
+            }
+            return null;
+        }
+
+        /**
+         * Determines the {@code index}-th key/value/tag (depending on {@code type}) of the {@link Selector.GeneralSelector}.
+         */
+        static String determineArgument(Selector.GeneralSelector matchingSelector, int index, String type) {
+            try {
+                final Condition c = matchingSelector.getConditions().get(index);
+                final Tag tag = c instanceof Condition.KeyCondition
+                        ? ((Condition.KeyCondition) c).asTag()
+                        : c instanceof Condition.KeyValueCondition
+                        ? ((Condition.KeyValueCondition) c).asTag()
+                        : null;
+                if (tag == null) {
+                    return null;
+                } else if ("key".equals(type)) {
+                    return tag.getKey();
+                } else if ("value".equals(type)) {
+                    return tag.getValue();
+                } else if ("tag".equals(type)) {
+                    return tag.toString();
+                }
+            } catch (IndexOutOfBoundsException ignore) {
+            }
+            return null;
+        }
+
+        /**
+         * Replaces occurrences of {@code {i.key}}, {@code {i.value}}, {@code {i.tag}} in {@code s} by the corresponding
+         * key/value/tag of the {@code index}-th {@link Condition} of {@code matchingSelector}.
+         */
+        static String insertArguments(Selector matchingSelector, String s) {
+            if (!(matchingSelector instanceof Selector.GeneralSelector) || s == null) {
+                return s;
+            }
+            final Matcher m = Pattern.compile("\\{(\\d+)\\.(key|value|tag)\\}").matcher(s);
+            final StringBuffer sb = new StringBuffer();
+            while (m.find()) {
+                m.appendReplacement(sb, determineArgument((Selector.GeneralSelector) matchingSelector, Integer.parseInt(m.group(1)), m.group(2)));
+            }
+            m.appendTail(sb);
+            return sb.toString();
         }
 
@@ -193 +242 @@
                 return null;
             }
+            final Selector matchingSelector = whichSelectorMatchesPrimitive(p);
             Collection<Command> cmds = new LinkedList<Command>();
             for (PrimitiveToTag toTag : change) {
                 final Tag tag = toTag.apply(p);
-                cmds.add(new ChangePropertyCommand(p, tag.getKey(), tag.getValue()));
+                final String key = insertArguments(matchingSelector, tag.getKey());
+                final String value = insertArguments(matchingSelector, tag.getValue());
+                cmds.add(new ChangePropertyCommand(p, key, value));
             }
             for (Map.Entry<String, String> i : keyChange.entrySet()) {
-                cmds.add(new ChangePropertyKeyCommand(p, i.getKey(), i.getValue()));
+                final String oldKey = insertArguments(matchingSelector, i.getKey());
+                final String newKey = insertArguments(matchingSelector, i.getValue());
+                cmds.add(new ChangePropertyKeyCommand(p, oldKey, newKey));
             }
             return new SequenceCommand(tr("Fix of {0}", getDescription()), cmds);
@@ -231 +285 @@
         }
 
+        /**
+         * Constructs a {@link TestError} for the given primitive, or returns null if the primitive does not give rise to an error.
+         *
+         * @param p the primitive to construct the error for
+         * @return an instance of {@link TestError}, or returns null if the primitive does not give rise to an error.
+         */
+        TestError getErrorForPrimitive(OsmPrimitive p) {
+            final Selector matchingSelector = whichSelectorMatchesPrimitive(p);
+            if (matchingSelector != null) {
+                final Command fix = fixPrimitive(p);
+                final String description = TagCheck.insertArguments(matchingSelector, getDescription());
+                if (fix != null) {
+                    return new FixableTestError(null, getSeverity(), description, 3000, p, fix);
+                } else {
+                    return new TestError(null, getSeverity(), description, 3000, p);
+                }
+            } else {
+                return null;
+            }
+        }
     }
 
@@ -240 +314 @@
     public void visit(OsmPrimitive p) {
         for (TagCheck check : checks) {
-            if (check.matchesPrimitive(p)) {
-                final Command fix = check.fixPrimitive(p);
-                if (fix != null) {
-                    errors.add(new FixableTestError(this, check.getSeverity(), check.getDescription(), 3000, p, fix));
-                } else {
-                    errors.add(new TestError(this, check.getSeverity(), check.getDescription(), 3000, p));
-                }
+            final TestError error = check.getErrorForPrimitive(p);
+            if (error != null) {
+                error.setTester(this);
+                errors.add(error);
             }
         }

trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Condition.java

@@ -12 +12 @@
 import org.openstreetmap.josm.data.osm.OsmUtils;
 import org.openstreetmap.josm.data.osm.Relation;
+import org.openstreetmap.josm.data.osm.Tag;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.mappaint.Cascade;
@@ -161 +162 @@
         public boolean applies(Environment env) {
             return op.eval(env.osm.get(k), v);
+        }
+
+        public Tag asTag() {
+            return new Tag(k, v);
         }
 
@@ -252 +257 @@
         }
 
+        public Tag asTag() {
+            return new Tag(label);
+        }
+
         @Override
         public String toString() {

trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/ExpressionFactory.java

@@ -431 +431 @@
             }
             Matcher m = Pattern.compile(pattern, f).matcher(target);
-            if (m.matches()) {
-                List<String> result = new ArrayList<String>(m.groupCount() + 1);
-                for (int i = 0; i <= m.groupCount(); i++) {
-                    result.add(m.group(i));
-                }
-                return result;
-            } else {
-                return null;
-            }
+            return Utils.getMatches(m);
         }
 
@@ -450 +442 @@
         public static List<String> regexp_match(String pattern, String target) {
             Matcher m = Pattern.compile(pattern).matcher(target);
-            if (m.matches()) {
-                List<String> result = new ArrayList<String>(m.groupCount() + 1);
-                for (int i = 0; i <= m.groupCount(); i++) {
-                    result.add(m.group(i));
-                }
-                return result;
-            } else {
-                return null;
-            }
+            return Utils.getMatches(m);
         }
 

trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java

@@ -2 +2 @@
 package org.openstreetmap.josm.gui.mappaint.mapcss;
 
+import java.util.Collections;
 import java.util.List;
 import java.util.regex.PatternSyntaxException;
@@ -231 +232 @@
             return true;
         }
+
+        public List<Condition> getConditions() {
+            return Collections.unmodifiableList(conds);
+        }
     }
 
@@ -310 +315 @@
         @Override
         public boolean matches(Environment e) {
-            if (!matchesBase(e)) return false;
-            return matchesConditions(e);
+            return matchesBase(e) && matchesConditions(e);
         }
 

trunk/src/org/openstreetmap/josm/tools/Utils.java

@@ -38 +38 @@
 import java.util.Iterator;
 import java.util.List;
+import java.util.regex.Matcher;
 import java.util.zip.GZIPInputStream;
 import java.util.zip.ZipFile;
@@ -835 +836 @@
         return String.format("%d %s %d %s", days, trn("day", "days", days), elapsedTime/3600000, tr("h"));
     }
+
+    /**
+     * Returns a list of capture groups if {@link Matcher#matches()}, or {@code null}.
+     * The first element (index 0) is the complete match.
+     * Further elements correspond to the parts in parentheses of the regular expression.
+     * @param m the matcher
+     * @return a list of capture groups if {@link Matcher#matches()}, or {@code null}.
+     */
+    public static List<String> getMatches(final Matcher m) {
+        if (m.matches()) {
+            List<String> result = new ArrayList<String>(m.groupCount() + 1);
+            for (int i = 0; i <= m.groupCount(); i++) {
+                result.add(m.group(i));
+            }
+            return result;
+        } else {
+            return null;
+        }
+    }
 }

trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java

@@ -4 +4 @@
 import org.junit.Test;
 import org.openstreetmap.josm.Main;
+import org.openstreetmap.josm.command.ChangePropertyCommand;
 import org.openstreetmap.josm.data.Preferences;
 import org.openstreetmap.josm.data.osm.Node;
@@ -10 +11 @@
 import org.openstreetmap.josm.data.osm.Tag;
 import org.openstreetmap.josm.data.osm.Way;
+import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.tools.TextTagParser;
 
 import java.io.StringReader;
+import java.text.MessageFormat;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -35 +38 @@
         final List<MapCSSTagChecker.TagCheck> checks = MapCSSTagChecker.TagCheck.readMapCSS(new StringReader("" +
                 "*[natural=marsh] {\n" +
-                "   throwWarning: tr(\"{0} is deprecated\", \"natural=marsh\");\n" +
-                "   fixRemove: \"natural\";\n" +
+                "   throwWarning: tr(\"{0} is deprecated\", \"{0.tag}\");\n" +
+                "   fixRemove: \"{0.key}\";\n" +
                 "   fixAdd: \"natural=wetland\";\n" +
                 "   fixAdd: \"wetland=marsh\";\n" +
@@ -43 +46 @@
         final MapCSSTagChecker.TagCheck check = checks.get(0);
         assertThat(check, notNullValue());
-        assertThat(check.change.get(0).apply(null), is(new Tag("natural")));
+        assertThat(check.getDescription(), is("{0.tag} is deprecated"));
+        assertThat(check.change.get(0).apply(null), is(new Tag("{0.key}")));
         assertThat(check.change.get(1).apply(null), is(new Tag("natural", "wetland")));
         assertThat(check.change.get(2).apply(null), is(new Tag("wetland", "marsh")));
-        assertThat(check.errors.keySet().iterator().next(), is("natural=marsh is deprecated"));
         final Node n1 = new Node();
         n1.put("natural", "marsh");
         assertTrue(check.matchesPrimitive(n1));
+        assertThat(check.getErrorForPrimitive(n1).getMessage(), is("natural=marsh is deprecated"));
+        assertThat(check.getErrorForPrimitive(n1).getSeverity(), is(Severity.WARNING));
+        assertThat(((ChangePropertyCommand) check.fixPrimitive(n1).getChildren().iterator().next()).getTags().toString(),
+                is("{natural=}"));
         final Node n2 = new Node();
         n2.put("natural", "wood");
         assertFalse(check.matchesPrimitive(n2));
+        assertThat(MapCSSTagChecker.TagCheck.insertArguments(check.selector.get(0), "The key is {0.key} and the value is {0.value}"),
+                is("The key is natural and the value is marsh"));
     }
 
@@ -97 +106 @@
                 final OsmPrimitive p = createPrimitiveForAssertion(i.getKey());
                 if (check.matchesPrimitive(p) != i.getValue()) {
-                    final String error = "Expecting test '" + check.getMessage() + "' to " + (i.getValue() ? "" : "not ") + "match " + i.getKey() + ", i.e., " + p.getKeys();
+                    final String error = MessageFormat.format("Expecting test ''{0}'' (i.e., {1}) to {2} {3} (i.e., {4})",
+                            check.getMessage(), check.selector, i.getValue() ? "match" : "not match", i.getKey(), p.getKeys());
                     System.err.println(error);
                     assertionErrors.add(error);