diff --git a/scripts/TagInfoExtract.java b/scripts/TagInfoExtract.java index 6a18235bd..ca15cc9d2 100644 --- a/scripts/TagInfoExtract.java +++ b/scripts/TagInfoExtract.java @@ -342,11 +342,8 @@ private void parseStyleSheet() throws IOException, ParseException { */ private List convertStyleSheet() { return styleSource.rules.stream() - .map(rule -> rule.selector) - .filter(Selector.GeneralSelector.class::isInstance) - .map(Selector.GeneralSelector.class::cast) - .map(Selector.AbstractSelector::getConditions) - .flatMap(Collection::stream) + .flatMap(rule -> rule.selectors.stream()) + .flatMap(selector -> selector.getConditions().stream()) .filter(ConditionFactory.SimpleKeyValueCondition.class::isInstance) .map(ConditionFactory.SimpleKeyValueCondition.class::cast) .map(condition -> condition.asTag(null)) @@ -393,7 +390,7 @@ Environment applyStylesheet(OsmPrimitive osm) { Environment env = new Environment(osm, mc, null, styleSource); for (MapCSSRule r : styleSource.rules) { env.clearSelectorMatchingInformation(); - if (r.selector.matches(env)) { + if (r.matches(env)) { // ignore selector range if (env.layer == null) { env.layer = "default"; diff --git a/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java b/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java index 37e6134f7..8b81fe41f 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java +++ b/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java @@ -9,40 +9,31 @@ import java.io.InputStream; import java.io.Reader; import java.io.StringReader; -import java.lang.reflect.Method; -import java.text.MessageFormat; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Consumer; import java.util.function.Predicate; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Stream; import org.openstreetmap.josm.command.ChangePropertyCommand; import org.openstreetmap.josm.command.ChangePropertyKeyCommand; import org.openstreetmap.josm.command.Command; import org.openstreetmap.josm.command.DeleteCommand; import org.openstreetmap.josm.command.SequenceCommand; -import org.openstreetmap.josm.data.coor.LatLon; -import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.IPrimitive; import org.openstreetmap.josm.data.osm.OsmPrimitive; -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.data.preferences.sources.SourceEntry; import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; import org.openstreetmap.josm.data.validation.OsmValidator; @@ -53,21 +44,13 @@ import org.openstreetmap.josm.gui.mappaint.Keyword; import org.openstreetmap.josm.gui.mappaint.MultiCascade; import org.openstreetmap.josm.gui.mappaint.mapcss.Condition; -import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.ClassCondition; -import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.ExpressionCondition; import org.openstreetmap.josm.gui.mappaint.mapcss.Expression; -import org.openstreetmap.josm.gui.mappaint.mapcss.ExpressionFactory.ParameterFunction; -import org.openstreetmap.josm.gui.mappaint.mapcss.Functions; import org.openstreetmap.josm.gui.mappaint.mapcss.Instruction; -import org.openstreetmap.josm.gui.mappaint.mapcss.LiteralExpression; import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSRule; -import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSRule.Declaration; +import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSStyleIndex; 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.GeneralSelector; -import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.OptimizedGeneralSelector; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.MapCSSParser; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.ParseException; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.TokenMgrError; @@ -78,13 +61,9 @@ import org.openstreetmap.josm.io.UTFInputStreamReader; import org.openstreetmap.josm.spi.preferences.Config; import org.openstreetmap.josm.tools.CheckParameterUtil; -import org.openstreetmap.josm.tools.DefaultGeoProperty; -import org.openstreetmap.josm.tools.GeoProperty; -import org.openstreetmap.josm.tools.GeoPropertyIndex; import org.openstreetmap.josm.tools.I18n; import org.openstreetmap.josm.tools.Logging; import org.openstreetmap.josm.tools.MultiMap; -import org.openstreetmap.josm.tools.Territories; import org.openstreetmap.josm.tools.Utils; /** @@ -92,48 +71,11 @@ * @since 6506 */ public class MapCSSTagChecker extends Test.TagTest { - MapCSSTagCheckerIndex indexData; + MapCSSStyleIndex indexData; + final Map ruleToCheckMap = new HashMap<>(); final Set tested = new HashSet<>(); - - /** - * A grouped MapCSSRule with multiple selectors for a single declaration. - * @see MapCSSRule - */ - public static class GroupedMapCSSRule { - /** MapCSS selectors **/ - public final List selectors; - /** MapCSS declaration **/ - public final Declaration declaration; - - /** - * Constructs a new {@code GroupedMapCSSRule}. - * @param selectors MapCSS selectors - * @param declaration MapCSS declaration - */ - public GroupedMapCSSRule(List selectors, Declaration declaration) { - this.selectors = selectors; - this.declaration = declaration; - } - - @Override - public int hashCode() { - return Objects.hash(selectors, declaration); - } - - @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); - } - - @Override - public String toString() { - return "GroupedMapCSSRule [selectors=" + selectors + ", declaration=" + declaration + ']'; - } - } + static final boolean ALL_TESTS = true; + static final boolean ONLY_SELECTED_TESTS = false; /** * The preference key for tag checker source entries. @@ -284,44 +226,48 @@ public ParseResult(List parseChecks, Collection parseErrors */ 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<>(); + protected final List fixCommands; /** Tags (or arbitrary strings) of alternatives to be presented to the user */ - protected final List alternatives = new ArrayList<>(); + protected final List alternatives; /** An {@link org.openstreetmap.josm.gui.mappaint.mapcss.Instruction.AssignmentInstruction}-{@link Severity} pair. * Is evaluated on the matching primitive to give the error message. Map is checked to contain exactly one element. */ - protected final Map errors = new HashMap<>(); - /** Unit tests */ - protected final Map assertions = new HashMap<>(); + protected final Map errors; /** MapCSS Classes to set on matching primitives */ - protected final Set setClassExpressions = new HashSet<>(); + protected final Collection setClassExpressions; /** Denotes whether the object should be deleted for fixing it */ protected boolean deletion; /** A string used to group similar tests */ protected String group; - TagCheck(GroupedMapCSSRule rule) { + TagCheck(MapCSSRule rule) { this.rule = rule; + this.fixCommands = new ArrayList<>(); + this.alternatives = new ArrayList<>(); + this.errors = new HashMap<>(); + this.setClassExpressions = new HashSet<>(); } - private static final String POSSIBLE_THROWS = possibleThrows(); + TagCheck(TagCheck check) { + this.rule = check.rule; + this.fixCommands = Utils.toUnmodifiableList(check.fixCommands); + this.alternatives = Utils.toUnmodifiableList(check.alternatives); + this.errors = Utils.toUnmodifiableMap(check.errors); + this.setClassExpressions = Utils.toUnmodifiableList(check.setClassExpressions); + this.deletion = check.deletion; + this.group = check.group; + } - static final String possibleThrows() { - StringBuilder sb = new StringBuilder(); - for (Severity s : Severity.values()) { - if (sb.length() > 0) { - sb.append('/'); - } - sb.append("throw") - .append(s.name().charAt(0)) - .append(s.name().substring(1).toLowerCase(Locale.ENGLISH)); - } - return sb.toString(); + TagCheck toImmutable() { + return new TagCheck(this); } - static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule) throws IllegalDataException { + private static final String POSSIBLE_THROWS = "throwError/throwWarning/throwOther"; + + static TagCheck ofMapCSSRule(final MapCSSRule rule, AssertionConsumer assertionConsumer) throws IllegalDataException { final TagCheck check = new TagCheck(rule); + final Map assertions = new HashMap<>(); for (Instruction i : rule.declaration.instructions) { if (i instanceof Instruction.AssignmentInstruction) { final Instruction.AssignmentInstruction ai = (Instruction.AssignmentInstruction) i; @@ -338,13 +284,15 @@ static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule) throws IllegalDataExc : ai.val instanceof Keyword ? ((Keyword) ai.val).val : null; - if (ai.key.startsWith("throw")) { - try { - check.errors.put(ai, Severity.valueOf(ai.key.substring("throw".length()).toUpperCase(Locale.ENGLISH))); - } catch (IllegalArgumentException e) { - Logging.log(Logging.LEVEL_WARN, - "Unsupported "+ai.key+" instruction. Allowed instructions are "+POSSIBLE_THROWS+'.', e); - } + if ("throwError".equals(ai.key)) { + check.errors.put(ai, Severity.ERROR); + } else if ("throwWarning".equals(ai.key)) { + check.errors.put(ai, Severity.WARNING); + } else if ("throwOther".equals(ai.key)) { + check.errors.put(ai, Severity.OTHER); + } else if (ai.key.startsWith("throw")) { + Logging.log(Logging.LEVEL_WARN, + "Unsupported " + ai.key + " instruction. Allowed instructions are " + POSSIBLE_THROWS + '.', null); } else if ("fixAdd".equals(ai.key)) { check.fixCommands.add(FixCommand.fixAdd(ai.val)); } else if ("fixRemove".equals(ai.key)) { @@ -361,9 +309,9 @@ static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule) throws IllegalDataExc } else if (val != null && "suggestAlternative".equals(ai.key)) { check.alternatives.add(val); } else if (val != null && "assertMatch".equals(ai.key)) { - check.assertions.put(val, Boolean.TRUE); + assertions.put(val, Boolean.TRUE); } else if (val != null && "assertNoMatch".equals(ai.key)) { - check.assertions.put(val, Boolean.FALSE); + assertions.put(val, Boolean.FALSE); } else if (val != null && "group".equals(ai.key)) { check.group = val; } else if (ai.key.startsWith("-")) { @@ -384,10 +332,13 @@ static TagCheck ofMapCSSRule(final GroupedMapCSSRule rule) throws IllegalDataExc "More than one "+POSSIBLE_THROWS+" given! You should specify a single validation error message for " + rule.selectors); } - return check; + if (assertionConsumer != null) { + MapCSSTagCheckerAsserts.checkAsserts(check, assertions, assertionConsumer); + } + return check.toImmutable(); } - static ParseResult readMapCSS(Reader css) throws ParseException { + static ParseResult readMapCSS(Reader css, AssertionConsumer assertionConsumer) throws ParseException { CheckParameterUtil.ensureParameterNotNull(css, "css"); final MapCSSStyleSource source = new MapCSSStyleSource(""); @@ -397,22 +348,10 @@ static ParseResult readMapCSS(Reader css) throws ParseException { } // Ignore "meta" rule(s) from external rules of JOSM wiki source.removeMetaRules(); - // group rules with common declaration block - Map> g = new LinkedHashMap<>(); - 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()) { + for (MapCSSRule rule : source.rules) { try { - parseChecks.add(TagCheck.ofMapCSSRule( - new GroupedMapCSSRule(map.getValue(), map.getKey()))); + parseChecks.add(TagCheck.ofMapCSSRule(rule, assertionConsumer)); } catch (IllegalDataException e) { Logging.error("Cannot add MapCss rule: "+e.getMessage()); source.logError(e); @@ -443,14 +382,14 @@ Selector whichSelectorMatchesEnvironment(Environment env) { /** * Determines the {@code index}-th key/value/tag (depending on {@code type}) of the - * {@link org.openstreetmap.josm.gui.mappaint.mapcss.Selector.GeneralSelector}. + * {@link GeneralSelector}. * @param matchingSelector matching selector * @param index index * @param type selector type ("key", "value" or "tag") * @param p OSM primitive * @return argument value, can be {@code null} */ - static String determineArgument(OptimizedGeneralSelector matchingSelector, int index, String type, OsmPrimitive p) { + static String determineArgument(GeneralSelector matchingSelector, int index, String type, OsmPrimitive p) { try { final Condition c = matchingSelector.getConditions().get(index); final Tag tag = c instanceof Condition.ToTagConvertable @@ -482,13 +421,13 @@ static String determineArgument(OptimizedGeneralSelector matchingSelector, int i static String insertArguments(Selector matchingSelector, String s, OsmPrimitive p) { if (s != null && matchingSelector instanceof Selector.ChildOrParentSelector) { return insertArguments(((Selector.ChildOrParentSelector) matchingSelector).right, s, p); - } else if (s == null || !(matchingSelector instanceof Selector.OptimizedGeneralSelector)) { + } else if (s == null || !(matchingSelector instanceof GeneralSelector)) { return s; } final Matcher m = Pattern.compile("\\{(\\d+)\\.(key|value|tag)\\}").matcher(s); final StringBuffer sb = new StringBuffer(); while (m.find()) { - final String argument = determineArgument((Selector.OptimizedGeneralSelector) matchingSelector, + final String argument = determineArgument((GeneralSelector) matchingSelector, Integer.parseInt(m.group(1)), m.group(2), p); try { // Perform replacement with null-safe + regex-safe handling @@ -635,55 +574,12 @@ public String toString() { return res; } - /** - * Returns the set of tagchecks on which this check depends on. - * @param schecks the collection of tagcheks to search in - * @return the set of tagchecks on which this check depends on - * @since 7881 - */ - public Set getTagCheckDependencies(Collection schecks) { - Set result = new HashSet<>(); - Set classes = getClassesIds(); - if (schecks != null && !classes.isEmpty()) { - for (TagCheck tc : schecks) { - if (this.equals(tc)) { - continue; - } - for (String id : tc.setClassExpressions) { - if (classes.contains(id)) { - result.add(tc); - break; - } - } - } - } - return result; - } - - /** - * Returns the list of ids of all MapCSS classes referenced in the rule selectors. - * @return the list of ids of all MapCSS classes referenced in the rule selectors - * @since 7881 - */ - 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); - } - } - } - } - return result; - } } static class MapCSSTagCheckerAndRule extends MapCSSTagChecker { - public final GroupedMapCSSRule rule; + public final MapCSSRule rule; - MapCSSTagCheckerAndRule(GroupedMapCSSRule rule) { + MapCSSTagCheckerAndRule(MapCSSRule rule) { this.rule = rule; } @@ -693,6 +589,18 @@ public String toString() { } } + static MapCSSStyleIndex createMapCSSTagCheckerIndex(MultiMap checks, boolean includeOtherSeverity, boolean allTests) { + final MapCSSStyleIndex index = new MapCSSStyleIndex(); + final Stream ruleStream = checks.values().stream() + .flatMap(Collection::stream) + // Ignore "information" level checks if not wanted, unless they also set a MapCSS class + .filter(c -> includeOtherSeverity || Severity.OTHER != c.getSeverity() || !c.setClassExpressions.isEmpty()) + .filter(c -> allTests || c.rule.selectors.stream().anyMatch(Selector.ChildOrParentSelector.class::isInstance)) + .map(c -> c.rule); + index.buildIndex(ruleStream); + return index; + } + /** * Obtains all {@link TestError}s for the {@link OsmPrimitive} {@code p}. * @param p The OSM primitive @@ -702,29 +610,29 @@ public String toString() { public synchronized Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity) { final List res = new ArrayList<>(); if (indexData == null) { - indexData = new MapCSSTagCheckerIndex(checks, includeOtherSeverity, MapCSSTagCheckerIndex.ALL_TESTS); + indexData = createMapCSSTagCheckerIndex(checks, includeOtherSeverity, ALL_TESTS); } - MapCSSRuleIndex matchingRuleIndex = indexData.get(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 - Declaration lastDeclUsed = null; - Iterator candidates = matchingRuleIndex.getRuleCandidates(p); + Iterator candidates = indexData.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) - TagCheck check = indexData.getCheck(r); + for (Selector selector : r.selectors) { + env.clearSelectorMatchingInformation(); + if (!selector.matches(env)) { // as side effect env.parent will be set (if s is a child selector) + continue; + } + TagCheck check = ruleToCheckMap.computeIfAbsent(r, rule -> checks.values().stream() + .flatMap(Collection::stream) + .filter(c -> Objects.equals(rule, c.rule)) + .findAny() + .orElse(null)); if (check != null) { - if (r.declaration == lastDeclUsed) - continue; // don't apply one declaration more than once - lastDeclUsed = r.declaration; - r.declaration.execute(env); if (!check.errors.isEmpty()) { - for (TestError e: check.getErrorsForPrimitive(p, r.selector, env, new MapCSSTagCheckerAndRule(check.rule))) { + for (TestError e: check.getErrorsForPrimitive(p, selector, env, new MapCSSTagCheckerAndRule(check.rule))) { addIfNotSimilar(e, res); } } @@ -774,7 +682,7 @@ private static boolean highlightedIsEqual(Collection highlighted, Collection< return false; } - private static Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity, + static Collection getErrorsForPrimitive(OsmPrimitive p, boolean includeOtherSeverity, Collection> checksCol) { final List r = new ArrayList<>(); final Environment env = new Environment(p, new MultiCascade(), Environment.DEFAULT_LAYER, null); @@ -812,6 +720,13 @@ public void check(OsmPrimitive p) { } } + /** + * A handler for assertion error messages (for not fulfilled "assertMatch", "assertNoMatch"). + */ + @FunctionalInterface + interface AssertionConsumer extends Consumer { + } + /** * Adds a new MapCSS config file from the given URL. * @param url The unique URL of the MapCSS config file @@ -821,6 +736,12 @@ public void check(OsmPrimitive p) { * @since 7275 */ public synchronized ParseResult addMapCSS(String url) throws ParseException, IOException { + // Check assertions, useful for development of local files + final boolean checkAssertions = Config.getPref().getBoolean("validator.check_assert_local_rules", false) && Utils.isLocalUrl(url); + return addMapCSS(url, checkAssertions ? Logging::warn : null); + } + + synchronized ParseResult addMapCSS(String url, AssertionConsumer assertionConsumer) throws ParseException, IOException { CheckParameterUtil.ensureParameterNotNull(url, "url"); ParseResult result; try (CachedFile cache = new CachedFile(url); @@ -829,16 +750,10 @@ public synchronized ParseResult addMapCSS(String url) throws ParseException, IOE Reader reader = new BufferedReader(UTFInputStreamReader.create(s))) { if (zip != null) I18n.addTexts(cache.getFile()); - result = TagCheck.readMapCSS(reader); + result = TagCheck.readMapCSS(reader, assertionConsumer); checks.remove(url); checks.putAll(url, result.parseChecks); indexData = null; - // 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)) { - Logging.warn(msg); - } - } } return result; } @@ -870,101 +785,7 @@ public synchronized void initialize() throws Exception { Logging.warn(ex); } } - } - - private static Method getFunctionMethod(String method) { - try { - return Functions.class.getDeclaredMethod(method, Environment.class, String.class); - } catch (NoSuchMethodException | SecurityException e) { - Logging.error(e); - return null; - } - } - - private static Optional getFirstInsideCountry(TagCheck check, Method insideMethod) { - return check.rule.selectors.stream() - .filter(s -> s instanceof GeneralSelector) - .flatMap(s -> ((GeneralSelector) s).getConditions().stream()) - .filter(c -> c instanceof ExpressionCondition) - .map(c -> ((ExpressionCondition) c).getExpression()) - .filter(c -> c instanceof ParameterFunction) - .map(c -> (ParameterFunction) c) - .filter(c -> c.getMethod().equals(insideMethod)) - .flatMap(c -> c.getArgs().stream()) - .filter(e -> e instanceof LiteralExpression) - .map(e -> ((LiteralExpression) e).getLiteral()) - .filter(l -> l instanceof String) - .map(l -> ((String) l).split(",")[0]) - .findFirst(); - } - - private static LatLon getLocation(TagCheck check, Method insideMethod) { - Optional inside = getFirstInsideCountry(check, insideMethod); - if (inside.isPresent()) { - GeoPropertyIndex index = Territories.getGeoPropertyIndex(inside.get()); - if (index != null) { - GeoProperty prop = index.getGeoProperty(); - if (prop instanceof DefaultGeoProperty) { - return ((DefaultGeoProperty) prop).getRandomLatLon(); - } - } - } - return LatLon.ZERO; - } - - /** - * Checks that rule assertions are met for the given set of TagChecks. - * @param schecks The TagChecks for which assertions have to be checked - * @return A set of error messages, empty if all assertions are met - * @since 7356 - */ - public Set checkAsserts(final Collection schecks) { - Set assertionErrors = new LinkedHashSet<>(); - final Method insideMethod = getFunctionMethod("inside"); - final DataSet ds = new DataSet(); - for (final TagCheck check : schecks) { - Logging.debug("Check: {0}", check); - for (final Map.Entry i : check.assertions.entrySet()) { - Logging.debug("- Assertion: {0}", i); - final OsmPrimitive p = OsmUtils.createPrimitive(i.getKey(), getLocation(check, insideMethod), true); - // Build minimal ordered list of checks to run to test the assertion - List> checksToRun = new ArrayList<>(); - Set checkDependencies = check.getTagCheckDependencies(schecks); - if (!checkDependencies.isEmpty()) { - checksToRun.add(checkDependencies); - } - checksToRun.add(Collections.singleton(check)); - // Add primitive to dataset to avoid DataIntegrityProblemException when evaluating selectors - addPrimitive(ds, p); - final Collection pErrors = getErrorsForPrimitive(p, true, checksToRun); - Logging.debug("- Errors: {0}", pErrors); - final boolean isError = pErrors.stream().anyMatch(e -> e.getTester() instanceof MapCSSTagCheckerAndRule - && ((MapCSSTagCheckerAndRule) e.getTester()).rule.equals(check.rule)); - if (isError != i.getValue()) { - assertionErrors.add(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())); - } - if (isError) { - // Check that autofix works as expected - Command fix = check.fixPrimitive(p); - if (fix != null && fix.executeCommand() && !getErrorsForPrimitive(p, true, checksToRun).isEmpty()) { - assertionErrors.add(MessageFormat.format("Autofix does not work for test ''{0}'' (i.e., {1})", - check.getMessage(p), check.rule.selectors)); - } - } - ds.removePrimitive(p); - } - } - return assertionErrors; - } - - private static void addPrimitive(DataSet ds, OsmPrimitive p) { - if (p instanceof Way) { - ((Way) p).getNodes().forEach(n -> addPrimitive(ds, n)); - } else if (p instanceof Relation) { - ((Relation) p).getMembers().forEach(m -> addPrimitive(ds, m.getMember())); - } - ds.addPrimitive(p); + MapCSSTagCheckerAsserts.clear(); } /** @@ -988,7 +809,7 @@ public synchronized void startTest(ProgressMonitor progressMonitor) { super.startTest(progressMonitor); super.setShowElements(true); if (indexData == null) { - indexData = new MapCSSTagCheckerIndex(checks, includeOtherSeverityChecks(), MapCSSTagCheckerIndex.ALL_TESTS); + indexData = createMapCSSTagCheckerIndex(checks, includeOtherSeverityChecks(), ALL_TESTS); } tested.clear(); } @@ -1001,7 +822,7 @@ public synchronized void endTest() { // rebuild index with a reduced set of rules (those that use ChildOrParentSelector) and thus may have left selectors // matching the previously tested elements - indexData = new MapCSSTagCheckerIndex(checks, includeOtherSeverityChecks(), MapCSSTagCheckerIndex.ONLY_SELECTED_TESTS); + indexData = createMapCSSTagCheckerIndex(checks, includeOtherSeverityChecks(), ONLY_SELECTED_TESTS); Set surrounding = new HashSet<>(); for (OsmPrimitive p : tested) { diff --git a/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerAsserts.java b/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerAsserts.java new file mode 100644 index 000000000..796cf5404 --- /dev/null +++ b/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerAsserts.java @@ -0,0 +1,167 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.data.validation.tests; + +import java.lang.reflect.Method; +import java.text.MessageFormat; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +import org.openstreetmap.josm.command.Command; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.OsmPrimitive; +import org.openstreetmap.josm.data.osm.OsmUtils; +import org.openstreetmap.josm.data.osm.Relation; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.validation.TestError; +import org.openstreetmap.josm.gui.mappaint.Environment; +import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory; +import org.openstreetmap.josm.gui.mappaint.mapcss.ExpressionFactory; +import org.openstreetmap.josm.gui.mappaint.mapcss.Functions; +import org.openstreetmap.josm.gui.mappaint.mapcss.LiteralExpression; +import org.openstreetmap.josm.gui.mappaint.mapcss.Selector; +import org.openstreetmap.josm.tools.DefaultGeoProperty; +import org.openstreetmap.josm.tools.GeoProperty; +import org.openstreetmap.josm.tools.GeoPropertyIndex; +import org.openstreetmap.josm.tools.Logging; +import org.openstreetmap.josm.tools.Territories; + +/** + * Utility class for checking rule assertions of {@link MapCSSTagChecker.TagCheck}. + */ +final class MapCSSTagCheckerAsserts { + + private MapCSSTagCheckerAsserts() { + // private constructor + } + + private static final ArrayList previousChecks = new ArrayList<>(); + + /** + * Checks that rule assertions are met for the given set of TagChecks. + * @param check The TagCheck for which assertions have to be checked + * @param assertionConsumer The handler for assertion error messages + */ + static void checkAsserts(final MapCSSTagChecker.TagCheck check, final Map assertions, + final MapCSSTagChecker.AssertionConsumer assertionConsumer) { + final Method insideMethod = getFunctionMethod("inside"); + final DataSet ds = new DataSet(); + Logging.debug("Check: {0}", check); + for (final Map.Entry i : assertions.entrySet()) { + Logging.debug("- Assertion: {0}", i); + final OsmPrimitive p = OsmUtils.createPrimitive(i.getKey(), getLocation(check, insideMethod), true); + // Build minimal ordered list of checks to run to test the assertion + List> checksToRun = new ArrayList<>(); + Set checkDependencies = getTagCheckDependencies(check, previousChecks); + if (!checkDependencies.isEmpty()) { + checksToRun.add(checkDependencies); + } + checksToRun.add(Collections.singleton(check)); + // Add primitive to dataset to avoid DataIntegrityProblemException when evaluating selectors + addPrimitive(ds, p); + final Collection pErrors = MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun); + Logging.debug("- Errors: {0}", pErrors); + final boolean isError = pErrors.stream().anyMatch(e -> e.getTester() instanceof MapCSSTagChecker.MapCSSTagCheckerAndRule + && ((MapCSSTagChecker.MapCSSTagCheckerAndRule) e.getTester()).rule.equals(check.rule)); + if (isError != i.getValue()) { + assertionConsumer.accept(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())); + } + if (isError) { + // Check that autofix works as expected + Command fix = check.fixPrimitive(p); + if (fix != null && fix.executeCommand() && !MapCSSTagChecker.getErrorsForPrimitive(p, true, checksToRun).isEmpty()) { + assertionConsumer.accept(MessageFormat.format("Autofix does not work for test ''{0}'' (i.e., {1})", + check.getMessage(p), check.rule.selectors)); + } + } + ds.removePrimitive(p); + } + previousChecks.add(check); + } + + public static void clear() { + previousChecks.clear(); + previousChecks.trimToSize(); + } + + private static Method getFunctionMethod(String method) { + try { + return Functions.class.getDeclaredMethod(method, Environment.class, String.class); + } catch (NoSuchMethodException | SecurityException e) { + Logging.error(e); + return null; + } + } + + private static void addPrimitive(DataSet ds, OsmPrimitive p) { + if (p instanceof Way) { + ((Way) p).getNodes().forEach(n -> addPrimitive(ds, n)); + } else if (p instanceof Relation) { + ((Relation) p).getMembers().forEach(m -> addPrimitive(ds, m.getMember())); + } + ds.addPrimitive(p); + } + + private static LatLon getLocation(MapCSSTagChecker.TagCheck check, Method insideMethod) { + Optional inside = getFirstInsideCountry(check, insideMethod); + if (inside.isPresent()) { + GeoPropertyIndex index = Territories.getGeoPropertyIndex(inside.get()); + if (index != null) { + GeoProperty prop = index.getGeoProperty(); + if (prop instanceof DefaultGeoProperty) { + return ((DefaultGeoProperty) prop).getRandomLatLon(); + } + } + } + return LatLon.ZERO; + } + + private static Optional getFirstInsideCountry(MapCSSTagChecker.TagCheck check, Method insideMethod) { + return check.rule.selectors.stream() + .filter(s -> s instanceof Selector.GeneralSelector) + .flatMap(s -> ((Selector.GeneralSelector) s).getConditions().stream()) + .filter(c -> c instanceof ConditionFactory.ExpressionCondition) + .map(c -> ((ConditionFactory.ExpressionCondition) c).getExpression()) + .filter(c -> c instanceof ExpressionFactory.ParameterFunction) + .map(c -> (ExpressionFactory.ParameterFunction) c) + .filter(c -> c.getMethod().equals(insideMethod)) + .flatMap(c -> c.getArgs().stream()) + .filter(e -> e instanceof LiteralExpression) + .map(e -> ((LiteralExpression) e).getLiteral()) + .filter(l -> l instanceof String) + .map(l -> ((String) l).split(",")[0]) + .findFirst(); + } + + /** + * Returns the set of tagchecks on which this check depends on. + * @param check the tagcheck + * @param schecks the collection of tagcheks to search in + * @return the set of tagchecks on which this check depends on + * @since 7881 + */ + private static Set getTagCheckDependencies(MapCSSTagChecker.TagCheck check, Collection schecks) { + Set result = new HashSet<>(); + Set classes = check.rule.selectors.stream() + .filter(s -> s instanceof Selector.AbstractSelector) + .flatMap(s -> ((Selector.AbstractSelector) s).getConditions().stream()) + .filter(c -> c instanceof ConditionFactory.ClassCondition) + .map(c -> ((ConditionFactory.ClassCondition) c).id) + .collect(Collectors.toSet()); + if (schecks != null && !classes.isEmpty()) { + return schecks.stream() + .filter(tc -> !check.equals(tc)) + .filter(tc -> tc.setClassExpressions.stream().anyMatch(classes::contains)) + .collect(Collectors.toSet()); + } + return result; + } +} diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/ConditionFactory.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/ConditionFactory.java index 059f9ac14..129ab8265 100644 --- a/src/org/openstreetmap/josm/gui/mappaint/mapcss/ConditionFactory.java +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/ConditionFactory.java @@ -359,7 +359,7 @@ public String toString() { * @throws PatternSyntaxException if the value syntax is invalid */ public KeyValueRegexpCondition(String k, String v, Op op, boolean considerValAsKey) { - super(k, v, op, considerValAsKey); + super(k, "" /* v is not needed */, op, considerValAsKey); CheckParameterUtil.ensureThat(!considerValAsKey, "considerValAsKey is not supported"); CheckParameterUtil.ensureThat(SUPPORTED_OPS.contains(op), "Op must be REGEX or NREGEX"); this.pattern = Pattern.compile(v); diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/Declaration.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/Declaration.java new file mode 100644 index 000000000..9adb581d2 --- /dev/null +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/Declaration.java @@ -0,0 +1,65 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.gui.mappaint.mapcss; + +import java.util.List; +import java.util.Objects; + +import org.openstreetmap.josm.gui.mappaint.Environment; +import org.openstreetmap.josm.gui.mappaint.StyleSource; +import org.openstreetmap.josm.tools.Utils; + +/** + * A declaration is a list of {@link Instruction}s + */ +public class Declaration { + /** + * The instructions in this declaration + */ + public final List instructions; + /** + * The index of this declaration + *

+ * declarations in the StyleSource are numbered consecutively + */ + public final int idx; + + /** + * Create a new {@link Declaration} + * @param instructions The instructions for this declaration + * @param idx The index in the {@link StyleSource} + */ + public Declaration(List instructions, int idx) { + this.instructions = Utils.toUnmodifiableList(instructions); + this.idx = idx; + } + + /** + *

Executes the instructions against the environment {@code env}

+ * + * @param env the environment + */ + public void execute(Environment env) { + for (Instruction i : instructions) { + i.execute(env); + } + } + + @Override + public int hashCode() { + return Objects.hash(instructions, idx); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + Declaration that = (Declaration) obj; + return idx == that.idx && + Objects.equals(instructions, that.instructions); + } + + @Override + public String toString() { + return "Declaration [instructions=" + instructions + ", idx=" + idx + ']'; + } +} diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/Instruction.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/Instruction.java index 0647da838..616d07246 100644 --- a/src/org/openstreetmap/josm/gui/mappaint/mapcss/Instruction.java +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/Instruction.java @@ -15,7 +15,7 @@ * * For example a simple assignment like width: 3;, but may also * be a set instruction (set highway;). - * A MapCSS {@link MapCSSRule.Declaration} is a list of instructions. + * A MapCSS {@link Declaration} is a list of instructions. */ @FunctionalInterface public interface Instruction extends StyleKeys { diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParser.jj b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParser.jj index 2451a1603..c707fce2c 100644 --- a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParser.jj +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParser.jj @@ -25,6 +25,7 @@ import org.openstreetmap.josm.gui.mappaint.mapcss.Condition.Context; import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory; import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.KeyMatchType; import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.Op; +import org.openstreetmap.josm.gui.mappaint.mapcss.Declaration; import org.openstreetmap.josm.gui.mappaint.mapcss.Expression; import org.openstreetmap.josm.gui.mappaint.mapcss.ExpressionFactory; import org.openstreetmap.josm.gui.mappaint.mapcss.ExpressionFactory.NullExpression; @@ -32,7 +33,6 @@ import org.openstreetmap.josm.gui.mappaint.mapcss.Instruction; import org.openstreetmap.josm.gui.mappaint.mapcss.LiteralExpression; import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSException; 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.Selector; import org.openstreetmap.josm.gui.mappaint.mapcss.Selector.ChildOrParentSelector; @@ -649,10 +649,8 @@ void rule(): { selectors=selectors() decl=declaration() - { - for (Selector s : selectors) { - sheet.rules.add(new MapCSSRule(s, decl)); - } + { + sheet.rules.add(new MapCSSRule(selectors, decl)); } } diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRule.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRule.java index 5a4b503e0..bdb81cba1 100644 --- a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRule.java +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSRule.java @@ -2,11 +2,9 @@ package org.openstreetmap.josm.gui.mappaint.mapcss; import java.util.List; -import java.util.Objects; import java.util.stream.Collectors; import org.openstreetmap.josm.gui.mappaint.Environment; -import org.openstreetmap.josm.gui.mappaint.StyleSource; import org.openstreetmap.josm.tools.Utils; /** @@ -21,82 +19,40 @@ /** * The selector. If it matches, this rule should be applied */ - public final Selector selector; + public final List selectors; /** * The instructions for this selector */ public final Declaration declaration; /** - * A declaration is a set of {@link Instruction}s + * Constructs a new {@code MapCSSRule}. + * @param selectors The selectors + * @param declaration The declaration */ - public static class Declaration { - /** - * The instructions in this declaration - */ - public final List instructions; - /** - * The index of this declaration - *

- * declarations in the StyleSource are numbered consecutively - */ - public final int idx; - - /** - * Create a new {@link Declaration} - * @param instructions The instructions for this dectlaration - * @param idx The index in the {@link StyleSource} - */ - public Declaration(List instructions, int idx) { - this.instructions = Utils.toUnmodifiableList(instructions); - this.idx = idx; - } - - /** - *

Executes the instructions against the environment {@code env}

- * - * @param env the environment - */ - public void execute(Environment env) { - for (Instruction i : instructions) { - i.execute(env); - } - } - - @Override - public int hashCode() { - return Objects.hash(instructions, idx); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) return true; - if (obj == null || getClass() != obj.getClass()) return false; - Declaration that = (Declaration) obj; - return idx == that.idx && - Objects.equals(instructions, that.instructions); - } - - @Override - public String toString() { - return "Declaration [instructions=" + instructions + ", idx=" + idx + ']'; - } + public MapCSSRule(List selectors, Declaration declaration) { + this.selectors = Utils.toUnmodifiableList(selectors); + this.declaration = declaration; } /** - * Constructs a new {@code MapCSSRule}. - * @param selector The selector - * @param declaration The declaration + * Test whether the selector of this rule applies to the primitive. + * + * @param env the Environment. env.mc and env.layer are read-only when matching a selector. + * env.source is not needed. This method will set the matchingReferrers field of env as + * a side effect! Make sure to clear it before invoking this method. + * @return true, if the selector applies + * @see Selector#matches */ - public MapCSSRule(Selector selector, Declaration declaration) { - this.selector = selector; - this.declaration = declaration; + public boolean matches(Environment env) { + return selectors.stream().anyMatch(s -> s.matches(env)); } /** *

Executes the instructions against the environment {@code env}

* * @param env the environment + * @see Declaration#execute */ public void execute(Environment env) { declaration.execute(env); @@ -109,9 +65,12 @@ public int compareTo(MapCSSRule o) { @Override public String toString() { - return selector + declaration.instructions.stream() + final String selectorsString = selectors.stream().map(String::valueOf) + .collect(Collectors.joining(",\n")); + final String declarationString = declaration.instructions.stream() .map(String::valueOf) .collect(Collectors.joining("\n ", " {\n ", "\n}")); + return selectorsString + declarationString; } } diff --git a/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerIndex.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleIndex.java similarity index 55% rename from src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerIndex.java rename to src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleIndex.java index 909042237..5fe45aad3 100644 --- a/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerIndex.java +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleIndex.java @@ -1,175 +1,166 @@ -// License: GPL. For details, see LICENSE file. -package org.openstreetmap.josm.data.validation.tests; - -import static org.openstreetmap.josm.tools.I18n.tr; - -import java.text.MessageFormat; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; - -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.validation.Severity; -import org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker.TagCheck; -import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSRule; -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.GeneralSelector; -import org.openstreetmap.josm.tools.JosmRuntimeException; -import org.openstreetmap.josm.tools.Logging; -import org.openstreetmap.josm.tools.MultiMap; - -/** - * Helper class for {@link MapCSSTagChecker} to store indexes of rules - * @author Gerd - * - */ -final class MapCSSTagCheckerIndex { - final Map ruleToCheckMap = new HashMap<>(); - - static final boolean ALL_TESTS = true; - static final boolean ONLY_SELECTED_TESTS = false; - - /** - * 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(); - - MapCSSTagCheckerIndex(MultiMap checks, boolean includeOtherSeverity, boolean allTests) { - buildIndex(checks, includeOtherSeverity, allTests); - } - - private void buildIndex(MultiMap checks, boolean includeOtherSeverity, boolean allTests) { - List allChecks = new ArrayList<>(); - for (Set cs : checks.values()) { - allChecks.addAll(cs); - } - - ruleToCheckMap.clear(); - nodeRules.clear(); - wayRules.clear(); - wayNoAreaRules.clear(); - relationRules.clear(); - multipolygonRules.clear(); - - // optimization: filter rules for different primitive types - for (TagCheck c : allChecks) { - if (!includeOtherSeverity && Severity.OTHER == c.getSeverity() - && c.setClassExpressions.isEmpty()) { - // Ignore "information" level checks if not wanted, unless they also set a MapCSS class - continue; - } - - for (Selector s : c.rule.selectors) { - // find the rightmost selector, this must be a GeneralSelector - boolean hasLeftRightSel = false; - Selector selRightmost = s; - while (selRightmost instanceof Selector.ChildOrParentSelector) { - hasLeftRightSel = true; - selRightmost = ((Selector.ChildOrParentSelector) selRightmost).right; - } - if (!allTests && !hasLeftRightSel) { - continue; - } - - MapCSSRule optRule = new MapCSSRule(s.optimizedBaseCheck(), c.rule.declaration); - - ruleToCheckMap.put(optRule, c); - final String base = ((GeneralSelector) selRightmost).getBase(); - switch (base) { - case Selector.BASE_NODE: - nodeRules.add(optRule); - break; - case Selector.BASE_WAY: - wayNoAreaRules.add(optRule); - wayRules.add(optRule); - break; - case Selector.BASE_AREA: - wayRules.add(optRule); - multipolygonRules.add(optRule); - break; - case Selector.BASE_RELATION: - relationRules.add(optRule); - multipolygonRules.add(optRule); - break; - case Selector.BASE_ANY: - nodeRules.add(optRule); - wayRules.add(optRule); - wayNoAreaRules.add(optRule); - relationRules.add(optRule); - multipolygonRules.add(optRule); - break; - case Selector.BASE_CANVAS: - case Selector.BASE_META: - case Selector.BASE_SETTING: - break; - default: - final RuntimeException e = new JosmRuntimeException(MessageFormat.format("Unknown MapCSS base selector {0}", base)); - Logging.warn(tr("Failed to index validator rules. Error was: {0}", e.getMessage())); - Logging.error(e); - } - } - } - nodeRules.initIndex(); - wayRules.initIndex(); - wayNoAreaRules.initIndex(); - relationRules.initIndex(); - multipolygonRules.initIndex(); - } - - /** - * Get the index of rules for the given primitive. - * @param p the primitve - * @return index of rules for the given primitive - */ - public MapCSSRuleIndex get(OsmPrimitive p) { - if (p instanceof INode) { - return nodeRules; - } else if (p instanceof IWay) { - if (OsmUtils.isFalse(p.get("area"))) { - return wayNoAreaRules; - } else { - return wayRules; - } - } else if (p instanceof IRelation) { - if (((IRelation) p).isMultipolygon()) { - return multipolygonRules; - } else { - return relationRules; - } - } else { - throw new IllegalArgumentException("Unsupported type: " + p); - } - } - - /** - * return the TagCheck for which the given indexed rule was created. - * @param rule an indexed rule - * @return the original TagCheck - */ - public TagCheck getCheck(MapCSSRule rule) { - return ruleToCheckMap.get(rule); - } -} +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.gui.mappaint.mapcss; + +import static org.openstreetmap.josm.tools.I18n.tr; + +import java.text.MessageFormat; +import java.util.Iterator; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.openstreetmap.josm.data.osm.INode; +import org.openstreetmap.josm.data.osm.IPrimitive; +import org.openstreetmap.josm.data.osm.IRelation; +import org.openstreetmap.josm.data.osm.IWay; +import org.openstreetmap.josm.data.osm.OsmUtils; +import org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSStyleSource.MapCSSRuleIndex; +import org.openstreetmap.josm.tools.JosmRuntimeException; +import org.openstreetmap.josm.tools.Logging; + +/** + * Store indexes of {@link MapCSSRule}s using {@link MapCSSRuleIndex} differentiated by {@linkplain Selector#getBase() base} + */ +public final class MapCSSStyleIndex { + + /** + * 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(); + + /** + * Clear the index. + *

+ * You must own the write lock STYLE_SOURCE_LOCK when calling this method. + */ + public void clear() { + nodeRules.clear(); + wayRules.clear(); + wayNoAreaRules.clear(); + relationRules.clear(); + multipolygonRules.clear(); + canvasRules.clear(); + } + + /** + * Builds and initializes the index. + *

+ * You must own the write lock of STYLE_SOURCE_LOCK when calling this method. + */ + public void buildIndex(Stream ruleStream) { + clear(); + // optimization: filter rules for different primitive types + ruleStream.forEach(rule -> { + final Map selectorsByBase = rule.selectors.stream() + .collect(Collectors.groupingBy(Selector::getBase, + Collectors.collectingAndThen(Collectors.toList(), selectors -> new MapCSSRule(selectors, rule.declaration)))); + selectorsByBase.forEach((base, optRule) -> { + switch (base) { + case Selector.BASE_NODE: + nodeRules.add(optRule); + break; + case Selector.BASE_WAY: + wayNoAreaRules.add(optRule); + wayRules.add(optRule); + break; + case Selector.BASE_AREA: + wayRules.add(optRule); + multipolygonRules.add(optRule); + break; + case Selector.BASE_RELATION: + relationRules.add(optRule); + multipolygonRules.add(optRule); + break; + case Selector.BASE_ANY: + nodeRules.add(optRule); + wayRules.add(optRule); + wayNoAreaRules.add(optRule); + relationRules.add(optRule); + multipolygonRules.add(optRule); + break; + case Selector.BASE_CANVAS: + canvasRules.add(optRule); + break; + case Selector.BASE_META: + case Selector.BASE_SETTING: + case Selector.BASE_SETTINGS: + break; + default: + final RuntimeException e = new JosmRuntimeException(MessageFormat.format("Unknown MapCSS base selector {0}", base)); + Logging.warn(tr("Failed to index validator rules. Error was: {0}", e.getMessage())); + Logging.error(e); + } + }); + }); + initIndex(); + } + + private void initIndex() { + nodeRules.initIndex(); + wayRules.initIndex(); + wayNoAreaRules.initIndex(); + relationRules.initIndex(); + multipolygonRules.initIndex(); + canvasRules.initIndex(); + } + + /** + * Get the index of rules for the given primitive. + * @param p the primitive + * @return index of rules for the given primitive + */ + public MapCSSRuleIndex get(IPrimitive p) { + if (p instanceof INode) { + return nodeRules; + } else if (p instanceof IWay) { + if (OsmUtils.isFalse(p.get("area"))) { + return wayNoAreaRules; + } else { + return wayRules; + } + } else if (p instanceof IRelation) { + if (((IRelation) p).isMultipolygon()) { + return multipolygonRules; + } else if (p.hasKey("#canvas")) { + return canvasRules; + } else { + return relationRules; + } + } else { + throw new IllegalArgumentException("Unsupported type: " + p); + } + } + + /** + * Get a subset of all rules that might match the primitive. Rules not included in the result are guaranteed to + * not match this primitive. + *

+ * You must have a read lock of STYLE_SOURCE_LOCK when calling this method. + * + * @param osm the primitive to match + * @return An iterator over possible rules in the right order. + */ + public Iterator getRuleCandidates(IPrimitive osm) { + return get(osm).getRuleCandidates(osm); + } +} diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleSource.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleSource.java index c959d18b6..86280c808 100644 --- a/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleSource.java +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSStyleSource.java @@ -13,7 +13,6 @@ import java.io.StringReader; import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; -import java.text.MessageFormat; import java.util.ArrayList; import java.util.BitSet; import java.util.Collections; @@ -25,6 +24,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.NoSuchElementException; +import java.util.Optional; import java.util.Set; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -33,13 +33,9 @@ import java.util.zip.ZipFile; import org.openstreetmap.josm.data.Version; -import org.openstreetmap.josm.data.osm.INode; import org.openstreetmap.josm.data.osm.IPrimitive; -import org.openstreetmap.josm.data.osm.IRelation; -import org.openstreetmap.josm.data.osm.IWay; import org.openstreetmap.josm.data.osm.KeyValueVisitor; import org.openstreetmap.josm.data.osm.Node; -import org.openstreetmap.josm.data.osm.OsmUtils; import org.openstreetmap.josm.data.osm.Tagged; import org.openstreetmap.josm.data.preferences.sources.SourceEntry; import org.openstreetmap.josm.gui.mappaint.Cascade; @@ -55,9 +51,7 @@ import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.KeyMatchType; import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.KeyValueCondition; import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.SimpleKeyValueCondition; -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.Selector.OptimizedGeneralSelector; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.MapCSSParser; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.ParseException; import org.openstreetmap.josm.gui.mappaint.mapcss.parsergen.TokenMgrError; @@ -88,29 +82,9 @@ */ public final List rules = new ArrayList<>(); /** - * Rules for nodes + * Index of rules in this style file */ - public final MapCSSRuleIndex nodeRules = new MapCSSRuleIndex(); - /** - * Rules for ways without tag area=no - */ - public final MapCSSRuleIndex wayRules = new MapCSSRuleIndex(); - /** - * Rules for ways with tag area=no - */ - public final MapCSSRuleIndex wayNoAreaRules = new MapCSSRuleIndex(); - /** - * Rules for relations that are not multipolygon relations - */ - public final MapCSSRuleIndex relationRules = new MapCSSRuleIndex(); - /** - * Rules for multipolygon relations - */ - public final MapCSSRuleIndex multipolygonRules = new MapCSSRuleIndex(); - /** - * rules to apply canvas properties - */ - public final MapCSSRuleIndex canvasRules = new MapCSSRuleIndex(); + private final MapCSSStyleIndex ruleIndex = new MapCSSStyleIndex(); private Color backgroundColorOverride; private String css; @@ -291,27 +265,24 @@ public void initIndex() { Collections.sort(rules); for (int ruleIndex = 0; ruleIndex < rules.size(); ruleIndex++) { MapCSSRule r = rules.get(ruleIndex); - // find the rightmost selector, this must be a GeneralSelector - Selector selRightmost = r.selector; - while (selRightmost instanceof ChildOrParentSelector) { - selRightmost = ((ChildOrParentSelector) selRightmost).right; - } - OptimizedGeneralSelector s = (OptimizedGeneralSelector) selRightmost; - if (s.conds == null) { - remaining.set(ruleIndex); - continue; - } - List sk = new ArrayList<>(Utils.filteredCollection(s.conds, - SimpleKeyValueCondition.class)); - if (!sk.isEmpty()) { - SimpleKeyValueCondition c = sk.get(sk.size() - 1); - getEntryInIndex(c.k).addForKeyAndValue(c.v, ruleIndex); - } else { - String key = findAnyRequiredKey(s.conds); - if (key != null) { - getEntryInIndex(key).addForKey(ruleIndex); - } else { + for (Selector selector : r.selectors) { + final List conditions = selector.getConditions(); + if (conditions == null || conditions.isEmpty()) { remaining.set(ruleIndex); + continue; + } + Optional lastCondition = Utils.filteredCollection(conditions, SimpleKeyValueCondition.class) + .stream() + .reduce((first, last) -> last); + if (lastCondition.isPresent()) { + getEntryInIndex(lastCondition.get().k).addForKeyAndValue(lastCondition.get().v, ruleIndex); + } else { + String key = findAnyRequiredKey(conditions); + if (key != null) { + getEntryInIndex(key).addForKey(ruleIndex); + } else { + remaining.set(ruleIndex); + } } } } @@ -423,12 +394,7 @@ public void loadStyleSource(boolean metadataOnly) { try { init(); rules.clear(); - nodeRules.clear(); - wayRules.clear(); - wayNoAreaRules.clear(); - relationRules.clear(); - multipolygonRules.clear(); - canvasRules.clear(); + ruleIndex.clear(); // remove "areaStyle" pseudo classes intended only for validator (causes StackOverflowError otherwise), see #16183 removeAreaStylePseudoClass = url == null || !url.contains("validator"); // resource://data/validator/ or xxx.validator.mapcss try (InputStream in = getSourceInputStream()) { @@ -466,57 +432,7 @@ public void loadStyleSource(boolean metadataOnly) { return; } // optimization: filter rules for different primitive types - for (MapCSSRule r: rules) { - // 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); - final String base = ((GeneralSelector) selRightmost).getBase(); - switch (base) { - case Selector.BASE_NODE: - nodeRules.add(optRule); - break; - case Selector.BASE_WAY: - wayNoAreaRules.add(optRule); - wayRules.add(optRule); - break; - case Selector.BASE_AREA: - wayRules.add(optRule); - multipolygonRules.add(optRule); - break; - case Selector.BASE_RELATION: - relationRules.add(optRule); - multipolygonRules.add(optRule); - break; - case Selector.BASE_ANY: - nodeRules.add(optRule); - wayRules.add(optRule); - wayNoAreaRules.add(optRule); - relationRules.add(optRule); - multipolygonRules.add(optRule); - break; - case Selector.BASE_CANVAS: - canvasRules.add(r); - break; - case Selector.BASE_META: - case Selector.BASE_SETTING: - case Selector.BASE_SETTINGS: - 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); - logError(e); - } - } - nodeRules.initIndex(); - wayRules.initIndex(); - wayNoAreaRules.initIndex(); - relationRules.initIndex(); - multipolygonRules.initIndex(); - canvasRules.initIndex(); + ruleIndex.buildIndex(rules.stream()); loaded = true; } finally { STYLE_SOURCE_LOCK.writeLock().unlock(); @@ -599,12 +515,12 @@ private void loadSettings() { // Parse rules for (MapCSSRule r : rules) { - if (r.selector instanceof GeneralSelector) { - GeneralSelector gs = (GeneralSelector) r.selector; + final Selector gs = r.selectors.get(0); + if (gs instanceof GeneralSelector) { if (Selector.BASE_SETTING.equals(gs.getBase())) { - loadSettings(r, gs, env); + loadSettings(r, ((GeneralSelector) gs), env); } else if (Selector.BASE_SETTINGS.equals(gs.getBase())) { - loadSettings(r, gs, envGroups); + loadSettings(r, ((GeneralSelector) gs), envGroups); } } } @@ -650,14 +566,11 @@ private Cascade constructSpecial(String type) { Environment env = new Environment(n, mc, "default", this); for (MapCSSRule r : rules) { - if (r.selector instanceof GeneralSelector) { - GeneralSelector gs = (GeneralSelector) r.selector; - if (gs.getBase().equals(type)) { - if (!gs.matchesConditions(env)) { - continue; - } - r.execute(env); - } + final boolean matches = r.selectors.stream().anyMatch(gs -> gs instanceof GeneralSelector + && gs.getBase().equals(type) + && ((GeneralSelector) gs).matchesConditions(env)); + if (matches) { + r.execute(env); } } return mc.getCascade("default"); @@ -670,39 +583,21 @@ public Color getBackgroundColorOverride() { @Override public void apply(MultiCascade mc, IPrimitive osm, double scale, boolean pretendWayIsClosed) { - MapCSSRuleIndex matchingRuleIndex; - if (osm instanceof INode) { - matchingRuleIndex = nodeRules; - } else if (osm instanceof IWay) { - if (OsmUtils.isFalse(osm.get("area"))) { - matchingRuleIndex = wayNoAreaRules; - } else { - matchingRuleIndex = wayRules; - } - } else if (osm instanceof IRelation) { - if (((IRelation) osm).isMultipolygon()) { - matchingRuleIndex = multipolygonRules; - } else if (osm.hasKey("#canvas")) { - matchingRuleIndex = canvasRules; - } else { - matchingRuleIndex = relationRules; - } - } else { - throw new IllegalArgumentException("Unsupported type: " + osm); - } Environment env = new Environment(osm, mc, null, this); // the declaration indices are sorted, so it suffices to save the last used index int lastDeclUsed = -1; - Iterator candidates = matchingRuleIndex.getRuleCandidates(osm); + Iterator candidates = ruleIndex.getRuleCandidates(osm); while (candidates.hasNext()) { MapCSSRule r = candidates.next(); - env.clearSelectorMatchingInformation(); - env.layer = r.selector.getSubpart().getId(env); - String sub = env.layer; - if (r.selector.matches(env)) { // as side effect env.parent will be set (if s is a child selector) - Selector s = r.selector; + for (Selector s : r.selectors) { + env.clearSelectorMatchingInformation(); + env.layer = s.getSubpart().getId(env); + String sub = env.layer; + if (!s.matches(env)) { // as side effect env.parent will be set (if s is a child selector) + continue; + } if (s.getRange().contains(scale)) { mc.range = Range.cut(mc.range, s.getRange()); } else { @@ -757,15 +652,7 @@ public boolean evalSupportsDeclCondition(String feature, Object val) { * @since 13633 */ public void removeMetaRules() { - for (Iterator it = rules.iterator(); it.hasNext();) { - MapCSSRule x = it.next(); - if (x.selector instanceof GeneralSelector) { - GeneralSelector gs = (GeneralSelector) x.selector; - if (Selector.BASE_META.equals(gs.base)) { - it.remove(); - } - } - } + rules.removeIf(x -> x.selectors.get(0) instanceof GeneralSelector && Selector.BASE_META.equals(x.selectors.get(0).getBase())); } /** diff --git a/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java b/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java index 21089f147..ca85f24d0 100644 --- a/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java +++ b/src/org/openstreetmap/josm/gui/mappaint/mapcss/Selector.java @@ -31,6 +31,7 @@ import org.openstreetmap.josm.gui.mappaint.Range; import org.openstreetmap.josm.gui.mappaint.mapcss.ConditionFactory.OpenEndPseudoClassCondition; import org.openstreetmap.josm.tools.CheckParameterUtil; +import org.openstreetmap.josm.tools.CompositeList; import org.openstreetmap.josm.tools.Geometry; import org.openstreetmap.josm.tools.Geometry.PolygonIntersection; import org.openstreetmap.josm.tools.Logging; @@ -105,19 +106,13 @@ */ Range getRange(); + String getBase(); + /** - * Create an "optimized" copy of this selector that omits the base check. - * - * For the style source, the list of rules is preprocessed, such that - * there is a separate list of rules for nodes, ways, ... - * - * This means that the base check does not have to be performed - * for each rule, but only once for each primitive. - * - * @return a selector that is identical to this object, except the base of the - * "rightmost" selector is not checked + * Returns the list of conditions. + * @return the list of conditions */ - Selector optimizedBaseCheck(); + List getConditions(); /** * The type of child of parent selector. @@ -166,6 +161,17 @@ public ChildOrParentSelector(Selector a, LinkSelector link, Selector b, ChildOrP this.type = type; } + @Override + public String getBase() { + // take the base from the rightmost selector + return right.getBase(); + } + + @Override + public List getConditions() { + return new CompositeList<>(left.getConditions(), right.getConditions()); + } + /** *

Finds the first referrer matching {@link #left}

* @@ -396,14 +402,14 @@ private void visitBBox(Environment e, AbstractFinder finder) { boolean withNodes = finder instanceof ContainsFinder; if (e.osm.getDataSet() == null) { // do nothing - } else if (left instanceof OptimizedGeneralSelector) { - if (withNodes && ((OptimizedGeneralSelector) left).matchesBase(OsmPrimitiveType.NODE)) { + } else if (left instanceof GeneralSelector) { + if (withNodes && ((GeneralSelector) left).matchesBase(OsmPrimitiveType.NODE)) { finder.visit(e.osm.getDataSet().searchNodes(e.osm.getBBox())); } - if (((OptimizedGeneralSelector) left).matchesBase(OsmPrimitiveType.WAY)) { + if (((GeneralSelector) left).matchesBase(OsmPrimitiveType.WAY)) { finder.visit(e.osm.getDataSet().searchWays(e.osm.getBBox())); } - if (((OptimizedGeneralSelector) left).matchesBase(OsmPrimitiveType.RELATION)) { + if (((GeneralSelector) left).matchesBase(OsmPrimitiveType.RELATION)) { finder.visit(e.osm.getDataSet().searchRelations(e.osm.getBBox())); } } else { @@ -454,9 +460,9 @@ public boolean matches(Environment e) { } else if (ChildOrParentSelectorType.CROSSING == type && e.osm instanceof IWay) { e.parent = e.osm; - if (right instanceof OptimizedGeneralSelector + if (right instanceof GeneralSelector && e.osm.getDataSet() != null - && ((OptimizedGeneralSelector) right).matchesBase(OsmPrimitiveType.WAY)) { + && ((GeneralSelector) right).matchesBase(OsmPrimitiveType.WAY)) { final CrossingFinder crossingFinder = new CrossingFinder(e); crossingFinder.visit(e.osm.getDataSet().searchWays(e.osm.getBBox())); } @@ -533,11 +539,6 @@ public Range getRange() { return right.getRange(); } - @Override - public Selector optimizedBaseCheck() { - return new ChildOrParentSelector(left, link, right.optimizedBaseCheck(), type); - } - @Override public String toString() { return left.toString() + ' ' + (ChildOrParentSelectorType.PARENT == type ? '<' : '>') + link + ' ' + right; @@ -576,10 +577,7 @@ public boolean matches(Environment env) { return true; } - /** - * Returns the list of conditions. - * @return the list of conditions - */ + @Override public List getConditions() { return conds; } @@ -602,17 +600,17 @@ public boolean matches(Environment env) { } @Override - public Subpart getSubpart() { - throw new UnsupportedOperationException("Not supported yet."); + public String getBase() { + throw new UnsupportedOperationException(); } @Override - public Range getRange() { - throw new UnsupportedOperationException("Not supported yet."); + public Subpart getSubpart() { + throw new UnsupportedOperationException(); } @Override - public Selector optimizedBaseCheck() { + public Range getRange() { throw new UnsupportedOperationException(); } @@ -625,47 +623,19 @@ public String toString() { /** * General selector. See wiki */ - class GeneralSelector extends OptimizedGeneralSelector { - - public GeneralSelector(String base, Range zoom, List conds, Subpart subpart) { - super(base, zoom, conds, subpart); - } - - public boolean matchesConditions(Environment e) { - return super.matches(e); - } - - @Override - public Selector optimizedBaseCheck() { - return new OptimizedGeneralSelector(this); - } + class GeneralSelector extends AbstractSelector { - @Override - public boolean matches(Environment e) { - return matchesBase(e) && super.matches(e); - } - } - - /** - * Superclass of {@link GeneralSelector}. Used to create an "optimized" copy of this selector that omits the base check. - * @see Selector#optimizedBaseCheck - */ - class OptimizedGeneralSelector extends AbstractSelector { public final String base; public final Range range; public final Subpart subpart; - public OptimizedGeneralSelector(String base, Range range, List conds, Subpart subpart) { + public GeneralSelector(String base, Range range, List conds, Subpart subpart) { super(conds); this.base = checkBase(base); this.range = Objects.requireNonNull(range, "range"); this.subpart = subpart != null ? subpart : Subpart.DEFAULT_SUBPART; } - public OptimizedGeneralSelector(GeneralSelector s) { - this(s.base, s.range, s.conds, s.subpart); - } - @Override public Subpart getSubpart() { return subpart; @@ -676,6 +646,15 @@ public Range getRange() { return range; } + public boolean matchesConditions(Environment e) { + return super.matches(e); + } + + @Override + public boolean matches(Environment e) { + return matchesBase(e) && super.matches(e); + } + /** * Set base and check if this is a known value. * @param base value for base @@ -698,6 +677,7 @@ private static String checkBase(String base) { } } + @Override public String getBase() { return base; } @@ -734,11 +714,6 @@ public boolean matchesBase(Environment e) { return matchesBase(e.osm); } - @Override - public Selector optimizedBaseCheck() { - throw new UnsupportedOperationException(); - } - public static Range fromLevel(int a, int b) { // for input validation in Range constructor below double lower = 0; diff --git a/src/org/openstreetmap/josm/tools/Utils.java b/src/org/openstreetmap/josm/tools/Utils.java index 1a02f4085..9a2317941 100644 --- a/src/org/openstreetmap/josm/tools/Utils.java +++ b/src/org/openstreetmap/josm/tools/Utils.java @@ -47,6 +47,7 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; @@ -68,6 +69,7 @@ import javax.script.ScriptEngine; import javax.script.ScriptEngineManager; +import com.kitfox.svg.xml.XMLParseUtil; import org.openstreetmap.josm.spi.preferences.Config; /** @@ -767,6 +769,21 @@ public B get(int index) { } } + /** + * Returns an unmodifiable map for the given map. + * Makes use of {@link Collections#emptyMap()} and {@link Collections#singletonMap} and {@link Map#ofEntries(Map.Entry[])} to save memory. + * + * @param map the map for which an unmodifiable map is to be returned + * @param the type of keys maintained by this map + * @param the type of mapped values + * @return an unmodifiable map + * @see + * How to Prevent Your Java Collections From Wasting Memory + */ + public static Map toUnmodifiableMap(Map map) { + return XMLParseUtil.toUnmodifiableMap(map); + } + /** * Returns the first not empty string in the given candidates, otherwise the default string. * @param defaultString default string returned if all candidates would be empty if stripped diff --git a/test/performance/org/openstreetmap/josm/gui/mappaint/MapRendererPerformanceTest.java b/test/performance/org/openstreetmap/josm/gui/mappaint/MapRendererPerformanceTest.java index a5d2c13be..d7b27a7ee 100644 --- a/test/performance/org/openstreetmap/josm/gui/mappaint/MapRendererPerformanceTest.java +++ b/test/performance/org/openstreetmap/josm/gui/mappaint/MapRendererPerformanceTest.java @@ -197,7 +197,7 @@ public void run() throws IOException { } nc.zoomTo(ProjectionRegistry.getProjection().latlon2eastNorth(center), scale); if (checkScale) { - int lvl = Selector.OptimizedGeneralSelector.scale2level(nc.getDist100Pixel()); + int lvl = Selector.GeneralSelector.scale2level(nc.getDist100Pixel()); Assert.assertEquals(17, lvl); } diff --git a/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java b/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java index edcdef51b..7d10ca653 100644 --- a/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java +++ b/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.Set; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.openstreetmap.josm.TestUtils; @@ -27,6 +28,8 @@ import org.openstreetmap.josm.data.osm.IPrimitive; import org.openstreetmap.josm.data.osm.OsmPrimitive; import org.openstreetmap.josm.data.osm.OsmUtils; +import org.openstreetmap.josm.data.preferences.sources.ExtendedSourceEntry; +import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper; import org.openstreetmap.josm.data.validation.Severity; import org.openstreetmap.josm.data.validation.TestError; import org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker.ParseResult; @@ -52,9 +55,19 @@ @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") public JOSMTestRules test = new JOSMTestRules().projection().territories().preferences(); + /** + * Setup test. + */ + @Before + public void setUp() { + MapCSSTagCheckerAsserts.clear(); + } + static MapCSSTagChecker buildTagChecker(String css) throws ParseException { final MapCSSTagChecker test = new MapCSSTagChecker(); - test.checks.putAll("test", TagCheck.readMapCSS(new StringReader(css)).parseChecks); + Set errors = new HashSet<>(); + test.checks.putAll("test", TagCheck.readMapCSS(new StringReader(css), errors::add).parseChecks); + assertTrue(errors.toString(), errors.isEmpty()); return test; } @@ -71,7 +84,7 @@ public void testNaturalMarsh() throws ParseException { " fixRemove: \"{0.key}\";\n" + " fixAdd: \"natural=wetland\";\n" + " fixAdd: \"wetland=marsh\";\n" + - "}")); + "}"), null); final List checks = result.parseChecks; assertEquals(1, checks.size()); assertTrue(result.parseErrors.isEmpty()); @@ -110,7 +123,7 @@ public void testTicket10913() throws ParseException { "throwError: \"error\";" + "fixChangeKey: \"highway => construction\";\n" + "fixAdd: \"highway=construction\";\n" + - "}")).parseChecks.get(0); + "}"), null).parseChecks.get(0); final Command command = check.fixPrimitive(p); assertTrue(command instanceof SequenceCommand); final Iterator it = command.getChildren().iterator(); @@ -155,7 +168,7 @@ public void testTicket10859() throws ParseException { @Test public void testTicket13630() throws ParseException { ParseResult result = TagCheck.readMapCSS(new StringReader( - "node[crossing=zebra] {fixRemove: \"crossing=zebra\";}")); + "node[crossing=zebra] {fixRemove: \"crossing=zebra\";}"), null); assertTrue(result.parseChecks.isEmpty()); assertEquals(1, result.parseErrors.size()); } @@ -181,11 +194,22 @@ public void testPreprocessing() throws ParseException { public void testInit() throws Exception { MapCSSTagChecker c = new MapCSSTagChecker(); c.initialize(); + } + /** + * Unit test for all {@link MapCSSTagChecker.TagTest} assertions. + * @throws Exception if an error occurs + */ + @Test + public void testAssertions() throws Exception { + MapCSSTagChecker c = new MapCSSTagChecker(); Set assertionErrors = new LinkedHashSet<>(); - for (Set schecks : c.checks.values()) { - assertionErrors.addAll(c.checkAsserts(schecks)); + + // initialize + for (ExtendedSourceEntry entry : ValidatorPrefHelper.INSTANCE.getDefault()) { + c.addMapCSS(entry.url, assertionErrors::add); } + for (String msg : assertionErrors) { Logging.error(msg); } @@ -204,8 +228,7 @@ public void testAssertInsideCountry() throws ParseException { " assertMatch: \"node amenity=parking\";\n" + " assertNoMatch: \"node amenity=restaurant\";\n" + "}"); - Set errors = test.checkAsserts(test.checks.get("test")); - assertTrue(errors.toString(), errors.isEmpty()); + assertNotNull(test); } /** @@ -220,8 +243,7 @@ public void testTicket17058() throws ParseException { " assertMatch: \"way name=Hauptstraße\";\n" + " assertNoMatch: \"way name=Hauptstrasse\";\n" + "}"); - Set errors = test.checkAsserts(test.checks.get("test")); - assertTrue(errors.toString(), errors.isEmpty()); + assertNotNull(test); } /** @@ -233,7 +255,7 @@ public void testTicket13762() throws ParseException { final ParseResult parseResult = TagCheck.readMapCSS(new StringReader("" + "meta[lang=de] {\n" + " title: \"Deutschlandspezifische Regeln\";" + - "}")); + "}"), null); assertTrue(parseResult.parseErrors.isEmpty()); } diff --git a/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/ChildOrParentSelectorTest.java b/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/ChildOrParentSelectorTest.java index 613123dce..5eca7aa8d 100644 --- a/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/ChildOrParentSelectorTest.java +++ b/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/ChildOrParentSelectorTest.java @@ -73,7 +73,7 @@ ChildOrParentSelector parse(String css) { MapCSSStyleSource source = new MapCSSStyleSource(css); source.loadStyleSource(); assertEquals(1, source.rules.size()); - return (ChildOrParentSelector) source.rules.get(0).selector; + return (ChildOrParentSelector) source.rules.get(0).selectors.get(0); } @Test diff --git a/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParserTest.java b/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParserTest.java index 07908488d..0d02b9ea2 100644 --- a/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParserTest.java +++ b/test/unit/org/openstreetmap/josm/gui/mappaint/mapcss/MapCSSParserTest.java @@ -451,7 +451,7 @@ public void testParentTags() throws Exception { source.loadStyleSource(); assertEquals(1, source.rules.size()); Environment e = new Environment(n, new MultiCascade(), Environment.DEFAULT_LAYER, null); - assertTrue(source.rules.get(0).selector.matches(e)); + assertTrue(source.rules.get(0).matches(e)); source.rules.get(0).declaration.execute(e); assertEquals("x2;x10", e.getCascade(Environment.DEFAULT_LAYER).get("refs", null, String.class)); } @@ -466,14 +466,14 @@ public void testSort() throws Exception { source.loadStyleSource(); assertEquals(1, source.rules.size()); Environment e = new Environment(way1, new MultiCascade(), Environment.DEFAULT_LAYER, null); - assertTrue(source.rules.get(0).selector.matches(e)); + assertTrue(source.rules.get(0).matches(e)); source.rules.get(0).declaration.execute(e); assertEquals(Functions.join(",", "Alpha", "Beta"), e.getCascade(Environment.DEFAULT_LAYER).get("sorted", null, String.class)); source = new MapCSSStyleSource("way[ref] {sorted: join_list(\",\", sort_list(split(\";\", tag(\"ref\"))));}"); source.loadStyleSource(); e = new Environment(way1, new MultiCascade(), Environment.DEFAULT_LAYER, null); - assertTrue(source.rules.get(0).selector.matches(e)); + assertTrue(source.rules.get(0).matches(e)); source.rules.get(0).declaration.execute(e); assertEquals(Functions.join(",", "A8", "A9"), e.getCascade(Environment.DEFAULT_LAYER).get("sorted", null, String.class)); } @@ -531,7 +531,7 @@ public void testCountRoles() throws Exception { source.loadStyleSource(); assertEquals(1, source.rules.size()); e = new Environment(rel1, new MultiCascade(), Environment.DEFAULT_LAYER, null); - assertTrue(source.rules.get(0).selector.matches(e)); + assertTrue(source.rules.get(0).matches(e)); source.rules.get(0).declaration.execute(e); assertEquals((Integer) 1, e.getCascade(Environment.DEFAULT_LAYER).get("roles", null, Integer.class)); } diff --git a/test/unit/org/openstreetmap/josm/tools/UtilsTest.java b/test/unit/org/openstreetmap/josm/tools/UtilsTest.java index 3f4096f2f..5443b7892 100644 --- a/test/unit/org/openstreetmap/josm/tools/UtilsTest.java +++ b/test/unit/org/openstreetmap/josm/tools/UtilsTest.java @@ -13,9 +13,12 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedList; import java.util.List; import java.util.Locale; +import java.util.Map; +import java.util.TreeMap; import java.util.regex.Pattern; import org.junit.Rule; @@ -573,4 +576,23 @@ public void testToUnmodifiableList() { assertEquals(Arrays.asList("foo", "bar", "baz"), Utils.toUnmodifiableList(new ArrayList<>(Arrays.asList("foo", "bar", "baz")))); assertEquals(Arrays.asList("foo", "bar", "baz"), Utils.toUnmodifiableList(new LinkedList<>(Arrays.asList("foo", "bar", "baz")))); } + + /** + * Test of {@link Utils#toUnmodifiableMap} + */ + @Test + public void testToUnmodifiableMap() { + assertSame(Collections.emptyMap(), Utils.toUnmodifiableMap(null)); + assertSame(Collections.emptyMap(), Utils.toUnmodifiableMap(Collections.emptyMap())); + assertSame(Collections.emptyMap(), Utils.toUnmodifiableMap(new HashMap<>())); + assertSame(Collections.emptyMap(), Utils.toUnmodifiableMap(new TreeMap<>())); + assertEquals(Collections.singletonMap("foo", "bar"), Utils.toUnmodifiableMap(new HashMap<>(Collections.singletonMap("foo", "bar")))); + assertEquals(Collections.singletonMap("foo", "bar"), Utils.toUnmodifiableMap(new TreeMap<>(Collections.singletonMap("foo", "bar")))); + final Map map4 = new HashMap<>(); + map4.put("jjj", "foo"); + map4.put("ooo", "bar"); + map4.put("sss", "baz"); + map4.put("mmm", ":-)"); + assertEquals(map4, Utils.toUnmodifiableMap(map4)); + } }