Index: trunk/data/validator/relation.mapcss =================================================================== --- trunk/data/validator/relation.mapcss (revision 6537) +++ trunk/data/validator/relation.mapcss (revision 6538) @@ -8,49 +8,28 @@ /* 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"; Index: trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java =================================================================== --- trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java (revision 6538) @@ -221,3 +221,7 @@ return children; } + + public Map getTags() { + return Collections.unmodifiableMap(tags); + } } Index: trunk/src/org/openstreetmap/josm/command/Command.java =================================================================== --- trunk/src/org/openstreetmap/josm/command/Command.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/command/Command.java (revision 6538) @@ -86,5 +86,5 @@ */ public Command() { - this.layer = Main.main.getEditLayer(); + this.layer = Main.main == null ? null : Main.main.getEditLayer(); } Index: trunk/src/org/openstreetmap/josm/data/validation/TestError.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/TestError.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/data/validation/TestError.java (revision 6538) @@ -219,4 +219,8 @@ } + public void setTester(Test tester) { + this.tester = tester; + } + /** * Gets the code Index: trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 6538) @@ -14,4 +14,6 @@ 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,4 +31,5 @@ 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,11 +176,57 @@ */ 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,11 +242,16 @@ return null; } + final Selector matchingSelector = whichSelectorMatchesPrimitive(p); Collection cmds = new LinkedList(); 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 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,4 +285,24 @@ } + /** + * 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,11 +314,8 @@ 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); } } Index: trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Condition.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Condition.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Condition.java (revision 6538) @@ -12,4 +12,5 @@ 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,4 +162,8 @@ public boolean applies(Environment env) { return op.eval(env.osm.get(k), v); + } + + public Tag asTag() { + return new Tag(k, v); } @@ -252,4 +257,8 @@ } + public Tag asTag() { + return new Tag(label); + } + @Override public String toString() { Index: trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/ExpressionFactory.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/ExpressionFactory.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/ExpressionFactory.java (revision 6538) @@ -431,13 +431,5 @@ } Matcher m = Pattern.compile(pattern, f).matcher(target); - if (m.matches()) { - List result = new ArrayList(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,13 +442,5 @@ public static List regexp_match(String pattern, String target) { Matcher m = Pattern.compile(pattern).matcher(target); - if (m.matches()) { - List result = new ArrayList(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); } Index: trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java =================================================================== --- trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java (revision 6538) @@ -2,4 +2,5 @@ package org.openstreetmap.josm.gui.mappaint.mapcss; +import java.util.Collections; import java.util.List; import java.util.regex.PatternSyntaxException; @@ -231,4 +232,8 @@ return true; } + + public List getConditions() { + return Collections.unmodifiableList(conds); + } } @@ -310,6 +315,5 @@ @Override public boolean matches(Environment e) { - if (!matchesBase(e)) return false; - return matchesConditions(e); + return matchesBase(e) && matchesConditions(e); } Index: trunk/src/org/openstreetmap/josm/tools/Utils.java =================================================================== --- trunk/src/org/openstreetmap/josm/tools/Utils.java (revision 6537) +++ trunk/src/org/openstreetmap/josm/tools/Utils.java (revision 6538) @@ -38,4 +38,5 @@ import java.util.Iterator; import java.util.List; +import java.util.regex.Matcher; import java.util.zip.GZIPInputStream; import java.util.zip.ZipFile; @@ -835,3 +836,22 @@ 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 getMatches(final Matcher m) { + if (m.matches()) { + List result = new ArrayList(m.groupCount() + 1); + for (int i = 0; i <= m.groupCount(); i++) { + result.add(m.group(i)); + } + return result; + } else { + return null; + } + } } Index: trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 6537) +++ trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 6538) @@ -4,4 +4,5 @@ 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,7 +11,9 @@ 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,6 +38,6 @@ final List 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,14 +46,20 @@ 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,5 +106,6 @@ 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);