Index: test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java =================================================================== --- test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 14442) +++ test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (working copy) @@ -93,7 +93,7 @@ assertEquals("{natural=}", ((ChangePropertyCommand) check.fixPrimitive(n1).getChildren().iterator().next()).getTags().toString()); assertFalse(check.test(n2)); assertEquals("The key is natural and the value is marsh", - TagCheck.insertArguments(check.rule.selectors.get(0), "The key is {0.key} and the value is {0.value}", null)); + TagCheck.insertArguments(check.rule.selector, "The key is {0.key} and the value is {0.value}", null)); } /** Index: src/org/openstreetmap/josm/data/validation/Test.java =================================================================== --- src/org/openstreetmap/josm/data/validation/Test.java (revision 14442) +++ src/org/openstreetmap/josm/data/validation/Test.java (working copy) @@ -188,7 +188,7 @@ if (startTime > 0) { // fix #11567 where elapsedTime is < 0 long elapsedTime = Math.max(0, System.currentTimeMillis() - startTime); - Logging.debug(tr("Test ''{0}'' completed in {1}", getName(), Utils.getDurationString(elapsedTime))); + Logging.warn(tr("Test ''{0}'' completed in {1}", getName(), Utils.getDurationString(elapsedTime))); } } Index: src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 14442) +++ src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (working copy) @@ -16,7 +16,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashMap; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; @@ -35,6 +35,9 @@ import org.openstreetmap.josm.command.DeleteCommand; import org.openstreetmap.josm.command.SequenceCommand; import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.INode; +import org.openstreetmap.josm.data.osm.IRelation; +import org.openstreetmap.josm.data.osm.IWay; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.data.osm.OsmUtils; import org.openstreetmap.josm.data.osm.Tag; @@ -52,10 +55,11 @@ import org.openstreetmap.josm.gui.mappaint.mapcss.Expression; import org.openstreetmap.josm.gui.mappaint.mapcss.Instruction; import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSRule; -import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSRule.Declaration; import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSStyleSource; +import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSStyleSource.MapCSSRuleIndex; import org.openstreetmap.josm.gui.mappaint.mapcss.Selector; import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.AbstractSelector; +import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.ChildOrParentSelector; import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.GeneralSelector; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.MapCSSParser; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.ParseException; @@ -67,10 +71,12 @@ import org.openstreetmap.josm.spi.preferences.Config; import org.openstreetmap.josm.tools.CheckParameterUtil; import org.openstreetmap.josm.tools.I18n; +import org.openstreetmap.josm.tools.JosmRuntimeException; import org.openstreetmap.josm.tools.Logging; import org.openstreetmap.josm.tools.MultiMap; import org.openstreetmap.josm.tools.Utils; + /** * MapCSS-based tag checker/fixer. * @since 6506 @@ -78,42 +84,172 @@ public class MapCSSTagChecker extends Test.TagTest { /** - * A grouped MapCSSRule with multiple selectors for a single declaration. - * @see MapCSSRule + * The rules from one url. + * @author GerdP + * */ - public static class GroupedMapCSSRule { - /** MapCSS selectors **/ - public final List selectors; - /** MapCSS declaration **/ - public final Declaration declaration; + static class RulesGroup { + /** + * Name of the group. + */ + private String url; + List errors; /** - * Constructs a new {@code GroupedMapCSSRule}. - * @param selectors MapCSS selectors - * @param declaration MapCSS declaration + * all checks in this file */ - public GroupedMapCSSRule(List selectors, Declaration declaration) { - this.selectors = selectors; - this.declaration = declaration; - } + final List checks = new ArrayList<>(); + /** + * Rules for nodes + */ + final MapCSSRuleIndex nodeRules = new MapCSSRuleIndex(); + /** + * Rules for ways without tag area=no + */ + final MapCSSRuleIndex wayRules = new MapCSSRuleIndex(); + /** + * Rules for ways with tag area=no + */ + final MapCSSRuleIndex wayNoAreaRules = new MapCSSRuleIndex(); + /** + * Rules for relations that are not multipolygon relations + */ + final MapCSSRuleIndex relationRules = new MapCSSRuleIndex(); + /** + * Rules for multipolygon relations + */ + final MapCSSRuleIndex multipolygonRules = new MapCSSRuleIndex(); + /** + * rules to apply canvas properties + */ + final MapCSSRuleIndex canvasRules = new MapCSSRuleIndex(); - @Override - public int hashCode() { - return Objects.hash(selectors, declaration); + final MultiMap map = new MultiMap<>(); + + public RulesGroup(String url, List parseChecks) { + this.url = url; + this.checks.addAll(parseChecks); + buildIndex(); } - @Override - public boolean equals(Object obj) { - if (this == obj) return true; - if (obj == null || getClass() != obj.getClass()) return false; - GroupedMapCSSRule that = (GroupedMapCSSRule) obj; - return Objects.equals(selectors, that.selectors) && - Objects.equals(declaration, that.declaration); + private void buildIndex() { + map.clear(); + nodeRules.clear(); + wayRules.clear(); + wayNoAreaRules.clear(); + relationRules.clear(); + multipolygonRules.clear(); + canvasRules.clear(); + // optimization: filter rules for different primitive types + + for (TagCheck c : checks) { + MapCSSRule r = c.rule; + // find the rightmost selector, this must be a GeneralSelector + Selector selRightmost = r.selector; + while (selRightmost instanceof ChildOrParentSelector) { + selRightmost = ((ChildOrParentSelector) selRightmost).right; + } + MapCSSRule optRule = new MapCSSRule(r.selector.optimizedBaseCheck(), r.declaration); + map.put(optRule, c); + final String base = ((GeneralSelector) selRightmost).getBase(); + switch (base) { + case "node": + nodeRules.add(optRule); + break; + case "way": + wayNoAreaRules.add(optRule); + wayRules.add(optRule); + break; + case "area": + wayRules.add(optRule); + multipolygonRules.add(optRule); + break; + case "relation": + relationRules.add(optRule); + multipolygonRules.add(optRule); + break; + case "*": + nodeRules.add(optRule); + wayRules.add(optRule); + wayNoAreaRules.add(optRule); + relationRules.add(optRule); + multipolygonRules.add(optRule); + break; + case "canvas": + canvasRules.add(r); + break; + case "meta": + case "setting": + break; + default: + final RuntimeException e = new JosmRuntimeException(MessageFormat.format("Unknown MapCSS base selector {0}", base)); + Logging.warn(tr("Failed to parse Mappaint styles from ''{0}''. Error was: {1}", url, e.getMessage())); + Logging.error(e); + } + } + nodeRules.initIndex(); + wayRules.initIndex(); + wayNoAreaRules.initIndex(); + relationRules.initIndex(); + multipolygonRules.initIndex(); + canvasRules.initIndex(); } - @Override - public String toString() { - return "GroupedMapCSSRule [selectors=" + selectors + ", declaration=" + declaration + ']'; + public List apply(OsmPrimitive p, boolean includeOtherSeverity) { + final List res = new ArrayList<>(); + MapCSSRuleIndex matchingRuleIndex; + if (p instanceof INode) { + matchingRuleIndex = nodeRules; + } else if (p instanceof IWay) { + if (OsmUtils.isFalse(p.get("area"))) { + matchingRuleIndex = wayNoAreaRules; + } else { + matchingRuleIndex = wayRules; + } + } else if (p instanceof IRelation) { + if (((IRelation) p).isMultipolygon()) { + matchingRuleIndex = multipolygonRules; + } else if (p.hasKey("#canvas")) { + matchingRuleIndex = canvasRules; + } else { + matchingRuleIndex = relationRules; + } + } else { + throw new IllegalArgumentException("Unsupported type: " + p); + } + + Environment env = new Environment(p, new MultiCascade(), Environment.DEFAULT_LAYER, null); + // the declaration indices are sorted, so it suffices to save the last used index + int lastDeclUsed = -1; + + Iterator candidates = matchingRuleIndex.getRuleCandidates(p); + while (candidates.hasNext()) { + MapCSSRule r = candidates.next(); + env.clearSelectorMatchingInformation(); + if (r.selector.matches(env)) { // as side effect env.parent will be set (if s is a child selector) + Set tests = map.get(r); + for (TagCheck check: tests) { + boolean ignoreError = Severity.OTHER == check.getSeverity() && !includeOtherSeverity; + // Do not run "information" level checks if not wanted, unless they also set a MapCSS class + if (ignoreError && check.setClassExpressions.isEmpty()) { + continue; + } + if (r.declaration.idx == lastDeclUsed) + continue; // don't apply one declaration more than once + lastDeclUsed = r.declaration.idx; + + r.declaration.execute(env); + if (!ignoreError && !check.errors.isEmpty()) { + final TestError error = check.getErrorForPrimitive(p, check.rule.selector, env, new MapCSSTagCheckerAndRule(check.rule)); + if (error != null) { + res.add(error); + } + } + + } + } + } + return res; } } @@ -239,6 +375,7 @@ } final MultiMap checks = new MultiMap<>(); + final LinkedHashSet checks2 = new LinkedHashSet<>(); /** * Result of {@link TagCheck#readMapCSS} @@ -266,7 +403,7 @@ */ public static class TagCheck implements Predicate { /** The selector of this {@code TagCheck} */ - protected final GroupedMapCSSRule rule; + protected final MapCSSRule rule; /** Commands to apply in order to fix a matching primitive */ protected final List fixCommands = new ArrayList<>(); /** Tags (or arbitraty strings) of alternatives to be presented to the user */ @@ -283,7 +420,7 @@ /** A string used to group similar tests */ protected String group; - TagCheck(GroupedMapCSSRule rule) { + TagCheck(MapCSSRule rule) { this.rule = rule; } @@ -302,7 +439,7 @@ return sb.toString(); } - static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule) throws IllegalDataException { + static TagCheck ofMapCSSRule(final MapCSSRule rule) throws IllegalDataException { final TagCheck check = new TagCheck(rule); for (Instruction i : rule.declaration.instructions) { if (i instanceof Instruction.AssignmentInstruction) { @@ -357,11 +494,11 @@ } if (check.errors.isEmpty() && check.setClassExpressions.isEmpty()) { throw new IllegalDataException( - "No "+POSSIBLE_THROWS+" given! You should specify a validation error message for " + rule.selectors); + "No "+POSSIBLE_THROWS+" given! You should specify a validation error message for " + rule.selector); } else if (check.errors.size() > 1) { throw new IllegalDataException( "More than one "+POSSIBLE_THROWS+" given! You should specify a single validation error message for " - + rule.selectors); + + rule.selector); } return check; } @@ -376,22 +513,10 @@ parser.sheet(source); // Ignore "meta" rule(s) from external rules of JOSM wiki source.removeMetaRules(); - // group rules with common declaration block - Map> g = new LinkedHashMap<>(); + List parseChecks = new ArrayList<>(); for (MapCSSRule rule : source.rules) { - if (!g.containsKey(rule.declaration)) { - List sels = new ArrayList<>(); - sels.add(rule.selector); - g.put(rule.declaration, sels); - } else { - g.get(rule.declaration).add(rule.selector); - } - } - List parseChecks = new ArrayList<>(); - for (Map.Entry> map : g.entrySet()) { try { - parseChecks.add(TagCheck.ofMapCSSRule( - new GroupedMapCSSRule(map.getValue(), map.getKey()))); + parseChecks.add(TagCheck.ofMapCSSRule(rule)); } catch (IllegalDataException e) { Logging.error("Cannot add MapCss rule: "+e.getMessage()); source.logError(e); @@ -402,24 +527,9 @@ @Override public boolean test(OsmPrimitive primitive) { - // Tests whether the primitive contains a deprecated tag which is represented by this MapCSSTagChecker. - return whichSelectorMatchesPrimitive(primitive) != null; + return rule.selector.matches(new Environment(primitive)); } - Selector whichSelectorMatchesPrimitive(OsmPrimitive primitive) { - return whichSelectorMatchesEnvironment(new Environment(primitive)); - } - - Selector whichSelectorMatchesEnvironment(Environment env) { - for (Selector i : rule.selectors) { - env.clearSelectorMatchingInformation(); - if (i.matches(env)) { - return i; - } - } - return null; - } - /** * Determines the {@code index}-th key/value/tag (depending on {@code type}) of the * {@link org.openstreetmap.josm.gui.mappaint.mapcss.Selector.GeneralSelector}. @@ -492,7 +602,7 @@ return null; } try { - final Selector matchingSelector = whichSelectorMatchesPrimitive(p); + final Selector matchingSelector = rule.selector; Collection cmds = new LinkedList<>(); for (FixCommand fixCommand : fixCommands) { cmds.add(fixCommand.createCommand(p, matchingSelector)); @@ -563,16 +673,9 @@ public String toString() { return getDescription(null); } - - /** - * 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 Environment env = new Environment(p); - return getErrorForPrimitive(p, whichSelectorMatchesEnvironment(env), env, null); + return getErrorForPrimitive(p, rule.selector, env, null); } TestError getErrorForPrimitive(OsmPrimitive p, Selector matchingSelector, Environment env, Test tester) { @@ -632,12 +735,10 @@ */ public Set getClassesIds() { Set result = new HashSet<>(); - for (Selector s : rule.selectors) { - if (s instanceof AbstractSelector) { - for (Condition c : ((AbstractSelector) s).getConditions()) { - if (c instanceof ClassCondition) { - result.add(((ClassCondition) c).id); - } + if (rule.selector instanceof AbstractSelector) { + for (Condition c : ((AbstractSelector) rule.selector).getConditions()) { + if (c instanceof ClassCondition) { + result.add(((ClassCondition) c).id); } } } @@ -646,9 +747,9 @@ } static class MapCSSTagCheckerAndRule extends MapCSSTagChecker { - public final GroupedMapCSSRule rule; + public final MapCSSRule rule; - MapCSSTagCheckerAndRule(GroupedMapCSSRule rule) { + MapCSSTagCheckerAndRule(MapCSSRule rule) { this.rule = rule; } @@ -656,7 +757,7 @@ public synchronized boolean equals(Object obj) { return super.equals(obj) || (obj instanceof TagCheck && rule.equals(((TagCheck) obj).rule)) - || (obj instanceof GroupedMapCSSRule && rule.equals(obj)); + || (obj instanceof MapCSSRule && rule.equals(obj)); } @Override @@ -677,7 +778,12 @@ * @return all errors for the given primitive, with or without those of "info" severity */ public synchronized Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity) { - return getErrorsForPrimitive(p, includeOtherSeverity, checks.values()); + List errors = new ArrayList<>(); + for (RulesGroup g : checks2) { + errors.addAll(g.apply(p, includeOtherSeverity)); + } + return errors; + } private static Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity, @@ -684,6 +790,7 @@ Collection> checksCol) { final List r = new ArrayList<>(); final Environment env = new Environment(p, new MultiCascade(), Environment.DEFAULT_LAYER, null); + for (Set schecks : checksCol) { for (TagCheck check : schecks) { boolean ignoreError = Severity.OTHER == check.getSeverity() && !includeOtherSeverity; @@ -691,15 +798,15 @@ if (ignoreError && check.setClassExpressions.isEmpty()) { continue; } - final Selector selector = check.whichSelectorMatchesEnvironment(env); - if (selector != null) { + if (check.rule.selector.matches(env)) { check.rule.declaration.execute(env); if (!ignoreError && !check.errors.isEmpty()) { - final TestError error = check.getErrorForPrimitive(p, selector, env, new MapCSSTagCheckerAndRule(check.rule)); + final TestError error = check.getErrorForPrimitive(p, check.rule.selector, env, new MapCSSTagCheckerAndRule(check.rule)); if (error != null) { r.add(error); } } + } } } @@ -706,6 +813,7 @@ return r; } + /** * Visiting call for primitives. * @@ -713,7 +821,12 @@ */ @Override public void check(OsmPrimitive p) { - errors.addAll(getErrorsForPrimitive(p, ValidatorPrefHelper.PREF_OTHER.get())); + boolean includeOtherSeverity = ValidatorPrefHelper.PREF_OTHER.get(); + + for (RulesGroup g : checks2) { + errors.addAll(g.apply(p, includeOtherSeverity)); + } + } /** @@ -736,6 +849,8 @@ result = TagCheck.readMapCSS(reader); checks.remove(url); checks.putAll(url, result.parseChecks); + RulesGroup rg = new RulesGroup(url, result.parseChecks); + checks2.add(rg); // Check assertions, useful for development of local files if (Config.getPref().getBoolean("validator.check_assert_local_rules", false) && Utils.isLocalUrl(url)) { for (String msg : checkAsserts(result.parseChecks)) { @@ -749,6 +864,8 @@ @Override public synchronized void initialize() throws Exception { checks.clear(); + checks2.clear(); + for (SourceEntry source : new ValidatorPrefHelper().get()) { if (!source.active) { continue; @@ -803,7 +920,7 @@ final boolean isError = pErrors.stream().anyMatch(e -> e.getTester().equals(check.rule)); if (isError != i.getValue()) { final String error = MessageFormat.format("Expecting test ''{0}'' (i.e., {1}) to {2} {3} (i.e., {4})", - check.getMessage(p), check.rule.selectors, i.getValue() ? "match" : "not match", i.getKey(), p.getKeys()); + check.getMessage(p), check.rule.selector, i.getValue() ? "match" : "not match", i.getKey(), p.getKeys()); assertionErrors.add(error); } ds.removePrimitive(p);