diff --git a/assets/layers/climbing_gym/climbing_gym.json b/assets/layers/climbing_gym/climbing_gym.json index 7948a2b9e..ae085eda4 100644 --- a/assets/layers/climbing_gym/climbing_gym.json +++ b/assets/layers/climbing_gym/climbing_gym.json @@ -135,7 +135,6 @@ "de": "Kletterschuhe können hier ausgeliehen werden" }, "addExtraTags": [ - "service:climbing_shoes:rental:fee=", "service:climbing_shoes:rental:charge=" ] }, diff --git a/assets/themes/campersite/campersite.json b/assets/themes/campersite/campersite.json index 64dc9a020..40eecd571 100644 --- a/assets/themes/campersite/campersite.json +++ b/assets/themes/campersite/campersite.json @@ -231,12 +231,7 @@ } }, { - "if": { - "and": [ - "fee=no", - "charge=" - ] - }, + "if": "fee=no", "then": { "en": "Can be used for free", "id": "Boleh digunakan tanpa bayaran", @@ -1562,4 +1557,4 @@ ] }, "credits": "joost schouppe" -} \ No newline at end of file +} diff --git a/src/Models/ThemeConfig/Conversion/Validation.ts b/src/Models/ThemeConfig/Conversion/Validation.ts index 6588e5edf..89fe06697 100644 --- a/src/Models/ThemeConfig/Conversion/Validation.ts +++ b/src/Models/ThemeConfig/Conversion/Validation.ts @@ -1,21 +1,22 @@ -import { DesugaringStep, Each, Fuse, On } from "./Conversion" -import { LayerConfigJson } from "../Json/LayerConfigJson" +import {DesugaringStep, Each, Fuse, On} from "./Conversion" +import {LayerConfigJson} from "../Json/LayerConfigJson" import LayerConfig from "../LayerConfig" -import { Utils } from "../../../Utils" +import {Utils} from "../../../Utils" import Constants from "../../Constants" -import { Translation } from "../../../UI/i18n/Translation" -import { LayoutConfigJson } from "../Json/LayoutConfigJson" +import {Translation} from "../../../UI/i18n/Translation" +import {LayoutConfigJson} from "../Json/LayoutConfigJson" import LayoutConfig from "../LayoutConfig" -import { TagRenderingConfigJson } from "../Json/TagRenderingConfigJson" -import { TagUtils } from "../../../Logic/Tags/TagUtils" -import { ExtractImages } from "./FixImages" -import { And } from "../../../Logic/Tags/And" +import {TagRenderingConfigJson} from "../Json/TagRenderingConfigJson" +import {TagUtils} from "../../../Logic/Tags/TagUtils" +import {ExtractImages} from "./FixImages" +import {And} from "../../../Logic/Tags/And" import Translations from "../../../UI/i18n/Translations" import Svg from "../../../Svg" import FilterConfigJson from "../Json/FilterConfigJson" import DeleteConfig from "../DeleteConfig" -import { QuestionableTagRenderingConfigJson } from "../Json/QuestionableTagRenderingConfigJson" +import {QuestionableTagRenderingConfigJson} from "../Json/QuestionableTagRenderingConfigJson" import Validators from "../../../UI/InputElement/Validators" +import TagRenderingConfig from "../TagRenderingConfig"; class ValidateLanguageCompleteness extends DesugaringStep { private readonly _languages: string[] @@ -46,12 +47,12 @@ class ValidateLanguageCompleteness extends DesugaringStep { .forEach((missing) => { errors.push( context + - "A theme should be translation-complete for " + - neededLanguage + - ", but it lacks a translation for " + - missing.context + - ".\n\tThe known translation is " + - missing.tr.textFor("en") + "A theme should be translation-complete for " + + neededLanguage + + ", but it lacks a translation for " + + missing.context + + ".\n\tThe known translation is " + + missing.tr.textFor("en") ) }) } @@ -85,7 +86,7 @@ export class DoesImageExist extends DesugaringStep { context: string ): { result: string; errors?: string[]; warnings?: string[]; information?: string[] } { if (this._ignore?.has(image)) { - return { result: image } + return {result: image} } const errors = [] @@ -93,22 +94,22 @@ export class DoesImageExist extends DesugaringStep { const information = [] if (image.indexOf("{") >= 0) { information.push("Ignoring image with { in the path: " + image) - return { result: image } + return {result: image} } if (image === "assets/SocialImage.png") { - return { result: image } + return {result: image} } if (image.match(/[a-z]*/)) { if (Svg.All[image + ".svg"] !== undefined) { // This is a builtin img, e.g. 'checkmark' or 'crosshair' - return { result: image } + return {result: image} } } if (image.startsWith("<") && image.endsWith(">")) { // This is probably HTML, you're on your own here - return { result: image } + return {result: image} } if (!this._knownImagePaths.has(image)) { @@ -177,15 +178,15 @@ class ValidateTheme extends DesugaringStep { if (json["units"] !== undefined) { errors.push( "The theme " + - json.id + - " has units defined - these should be defined on the layer instead. (Hint: use overrideAll: { '+units': ... }) " + json.id + + " has units defined - these should be defined on the layer instead. (Hint: use overrideAll: { '+units': ... }) " ) } if (json["roamingRenderings"] !== undefined) { errors.push( "Theme " + - json.id + - " contains an old 'roamingRenderings'. Use an 'overrideAll' instead" + json.id + + " contains an old 'roamingRenderings'. Use an 'overrideAll' instead" ) } } @@ -197,10 +198,10 @@ class ValidateTheme extends DesugaringStep { for (const remoteImage of remoteImages) { errors.push( "Found a remote image: " + - remoteImage + - " in theme " + - json.id + - ", please download it." + remoteImage + + " in theme " + + json.id + + ", please download it." ) } for (const image of images) { @@ -227,12 +228,12 @@ class ValidateTheme extends DesugaringStep { if (theme.id !== filename) { errors.push( "Theme ids should be the same as the name.json, but we got id: " + - theme.id + - " and filename " + - filename + - " (" + - this._path + - ")" + theme.id + + " and filename " + + filename + + " (" + + this._path + + ")" ) } this._validateImage.convertJoin( @@ -312,7 +313,7 @@ class OverrideShadowingCheck extends DesugaringStep { ): { result: LayoutConfigJson; errors?: string[]; warnings?: string[] } { const overrideAll = json.overrideAll if (overrideAll === undefined) { - return { result: json } + return {result: json} } const errors = [] @@ -339,7 +340,7 @@ class OverrideShadowingCheck extends DesugaringStep { } } - return { result: json, errors } + return {result: json, errors} } } @@ -383,6 +384,51 @@ export class PrevalidateTheme extends Fuse { } } +export class DetectConflictingAddExtraTags extends DesugaringStep { + constructor() { + super("The `if`-part in a mapping might set some keys. Those key are not allowed to be set in the `addExtraTags`, as this might result in conflicting values", [], "DetectConflictingAddExtraTags"); + } + + convert(json: TagRenderingConfigJson, context: string): { + result: TagRenderingConfigJson; + errors?: string[]; + warnings?: string[]; + information?: string[] + } { + + if (!(json.mappings?.length > 0)) { + return {result: json} + } + + const tagRendering = new TagRenderingConfig(json) + + const errors = [] + for (let i = 0; i < tagRendering.mappings.length; i++) { + const mapping = tagRendering.mappings[i]; + if (!mapping.addExtraTags) { + continue + } + const keysInMapping = new Set(mapping.if.usedKeys()) + + const keysInAddExtraTags = mapping.addExtraTags.map(t => t.key) + + const duplicateKeys = keysInAddExtraTags.filter(k => keysInMapping.has(k)) + if (duplicateKeys.length > 0) { + errors.push( + "At " + context + ".mappings[" + i + "]: AddExtraTags overrides a key that is set in the `if`-clause of this mapping. Selecting this answer might thus first set one value (needed to match as answer) and then override it with a different value, resulting in an unsaveable question. The offending `addExtraTags` is " + duplicateKeys.join(", ") + ) + } + } + + + return { + result: json, + errors + }; + } +} + + export class DetectShadowedMappings extends DesugaringStep { private readonly _calculatedTagNames: string[] @@ -449,7 +495,7 @@ export class DetectShadowedMappings extends DesugaringStep { + keyValues.forEach(({k, v}) => { properties[k] = v }) for (let j = 0; j < i; j++) { @@ -492,10 +538,10 @@ export class DetectShadowedMappings extends DesugaringStep { if (json["special"] !== undefined) { errors.push( "At " + - context + - ': detected `special` on the top level. Did you mean `{"render":{ "special": ... }}`' + context + + ': detected `special` on the top level. Did you mean `{"render":{ "special": ... }}`' ) } if (json["group"]) { errors.push( "At " + - context + - ': groups are deprecated, use `"label": ["' + - json["group"] + - '"]` instead' + context + + ': groups are deprecated, use `"label": ["' + + json["group"] + + '"]` instead' ) } const freeformType = json["freeform"]?.["type"] @@ -669,6 +715,7 @@ export class ValidateTagRenderings extends Fuse { super( "Various validation on tagRenderingConfigs", new DetectShadowedMappings(layerConfig), + new DetectConflictingAddExtraTags(), new DetectMappingsWithImages(doesImageExist), new MiscTagRenderingChecks(options) ) @@ -711,9 +758,9 @@ export class ValidateLayer extends DesugaringStep { if (!Constants.priviliged_layers.find((x) => x == json.id)) { errors.push( context + - ": layer " + - json.id + - " uses 'special' as source.osmTags. However, this layer is not a priviliged layer" + ": layer " + + json.id + + " uses 'special' as source.osmTags. However, this layer is not a priviliged layer" ) } } @@ -722,13 +769,13 @@ export class ValidateLayer extends DesugaringStep { if (json.title === undefined && json.source !== "special:library") { errors.push( context + - ": this layer does not have a title defined but it does have tagRenderings. Not having a title will disable the popups, resulting in an unclickable element. Please add a title. If not having a popup is intended and the tagrenderings need to be kept (e.g. in a library layer), set `title: null` to disable this error." + ": this layer does not have a title defined but it does have tagRenderings. Not having a title will disable the popups, resulting in an unclickable element. Please add a title. If not having a popup is intended and the tagrenderings need to be kept (e.g. in a library layer), set `title: null` to disable this error." ) } if (json.title === null) { information.push( context + - ": title is `null`. This results in an element that cannot be clicked - even though tagRenderings is set." + ": title is `null`. This results in an element that cannot be clicked - even though tagRenderings is set." ) } } @@ -755,9 +802,9 @@ export class ValidateLayer extends DesugaringStep { console.log(json.tagRenderings) errors.push( "At " + - context + - ": some tagrenderings have a duplicate id: " + - duplicates.join(", ") + context + + ": some tagrenderings have a duplicate id: " + + duplicates.join(", ") ) } } @@ -775,8 +822,8 @@ export class ValidateLayer extends DesugaringStep { if (json["overpassTags"] !== undefined) { errors.push( "Layer " + - json.id + - 'still uses the old \'overpassTags\'-format. Please use "source": {"osmTags": }\' instead of "overpassTags": (note: this isn\'t your fault, the custom theme generator still spits out the old format)' + json.id + + 'still uses the old \'overpassTags\'-format. Please use "source": {"osmTags": }\' instead of "overpassTags": (note: this isn\'t your fault, the custom theme generator still spits out the old format)' ) } const forbiddenTopLevel = [ @@ -794,18 +841,18 @@ export class ValidateLayer extends DesugaringStep { if (json[forbiddenKey] !== undefined) errors.push( context + - ": layer " + - json.id + - " still has a forbidden key " + - forbiddenKey + ": layer " + + json.id + + " still has a forbidden key " + + forbiddenKey ) } if (json["hideUnderlayingFeaturesMinPercentage"] !== undefined) { errors.push( context + - ": layer " + - json.id + - " contains an old 'hideUnderlayingFeaturesMinPercentage'" + ": layer " + + json.id + + " contains an old 'hideUnderlayingFeaturesMinPercentage'" ) } @@ -822,9 +869,9 @@ export class ValidateLayer extends DesugaringStep { if (this._path != undefined && this._path.indexOf(expected) < 0) { errors.push( "Layer is in an incorrect place. The path is " + - this._path + - ", but expected " + - expected + this._path + + ", but expected " + + expected ) } } @@ -865,6 +912,13 @@ export class ValidateLayer extends DesugaringStep { } } + if (json.filter) { + const r = new On("filter", new Each( new ValidateFilter())).convert(json, context) + warnings.push(...(r.warnings ?? [])) + errors.push(...(r.errors ?? [])) + information.push(...(r.information ?? [])) + } + if (json.tagRenderings !== undefined) { const r = new On( "tagRenderings", @@ -886,9 +940,9 @@ export class ValidateLayer extends DesugaringStep { if (hasCondition?.length > 0) { errors.push( "At " + - context + - ":\n One or more icons in the mapRenderings have a condition set. Don't do this, as this will result in an invisible but clickable element. Use extra filters in the source instead. The offending mapRenderings are:\n" + - JSON.stringify(hasCondition, null, " ") + context + + ":\n One or more icons in the mapRenderings have a condition set. Don't do this, as this will result in an invisible but clickable element. Use extra filters in the source instead. The offending mapRenderings are:\n" + + JSON.stringify(hasCondition, null, " ") ) } } @@ -903,7 +957,7 @@ export class ValidateLayer extends DesugaringStep { const preset = json.presets[i] const tags: { k: string; v: string }[] = new And( preset.tags.map((t) => TagUtils.Tag(t)) - ).asChange({ id: "node/-1" }) + ).asChange({id: "node/-1"}) const properties = {} for (const tag of tags) { properties[tag.k] = tag.v @@ -912,12 +966,12 @@ export class ValidateLayer extends DesugaringStep { if (!doMatch) { errors.push( context + - ".presets[" + - i + - "]: This preset does not match the required tags of this layer. This implies that a newly added point will not show up.\n A newly created point will have properties: " + - JSON.stringify(properties) + - "\n The required tags are: " + - baseTags.asHumanString(false, false, {}) + ".presets[" + + i + + "]: This preset does not match the required tags of this layer. This implies that a newly added point will not show up.\n A newly created point will have properties: " + + JSON.stringify(properties) + + "\n The required tags are: " + + baseTags.asHumanString(false, false, {}) ) } } @@ -949,9 +1003,14 @@ export class ValidateFilter extends DesugaringStep { warnings?: string[] information?: string[] } { + if (typeof filter === "string") { + // Calling another filter, we skip + return {result: filter} + } const errors = [] for (const option of filter.options) { - for (let i = 0; i < option.fields.length; i++) { + + for (let i = 0; i < option.fields?.length ?? 0; i++) { const field = option.fields[i] const type = field.type ?? "string" if (Validators.availableTypes.find((t) => t === type) === undefined) { @@ -962,7 +1021,7 @@ export class ValidateFilter extends DesugaringStep { } } } - return { result: filter, errors } + return {result: filter, errors} } } @@ -991,7 +1050,7 @@ export class DetectDuplicateFilters extends DesugaringStep<{ const warnings: string[] = [] const information: string[] = [] - const { layers, themes } = json + const {layers, themes} = json const perOsmTag = new Map< string, { @@ -1027,7 +1086,7 @@ export class DetectDuplicateFilters extends DesugaringStep<{ return } let msg = "Possible duplicate filter: " + key - for (const { filter, layer, layout } of value) { + for (const {filter, layer, layout} of value) { let id = "" if (layout !== undefined) { id = layout.id + ":"