From 2e8a3311e76c9065338ffce1388658269a05e261 Mon Sep 17 00:00:00 2001 From: Pieter Vander Vennet Date: Sun, 24 Sep 2023 00:25:10 +0200 Subject: [PATCH] Themes: add validation for duplicate presets, fix #1582 --- assets/themes/pets/pets.json | 11 +-- .../vending_machine/vending_machine.json | 3 +- .../ThemeConfig/Conversion/Validation.ts | 83 +++++++++++++++++-- src/Models/ThemeConfig/LayerConfig.ts | 2 +- src/Utils.ts | 2 +- 5 files changed, 83 insertions(+), 18 deletions(-) diff --git a/assets/themes/pets/pets.json b/assets/themes/pets/pets.json index e63b216c6c..7f7b7338f6 100644 --- a/assets/themes/pets/pets.json +++ b/assets/themes/pets/pets.json @@ -144,14 +144,7 @@ "width": 5 } ], - "presets": [ - { - "tags": [ - "shop=yes", - "dog=yes" - ] - } - ], + "=presets": [ ], "source": { "=osmTags": { "and": [ @@ -227,4 +220,4 @@ } ], "credits": "Niels Elgaard Larsen" -} \ No newline at end of file +} diff --git a/assets/themes/vending_machine/vending_machine.json b/assets/themes/vending_machine/vending_machine.json index 7f053f2d2c..66637bcb1f 100644 --- a/assets/themes/vending_machine/vending_machine.json +++ b/assets/themes/vending_machine/vending_machine.json @@ -29,6 +29,7 @@ "sameAs": "vending_machine" }, "minzoom": 18, + "=presets": [], "source": { "osmTags": { "and": [ @@ -61,4 +62,4 @@ } } ] -} \ No newline at end of file +} diff --git a/src/Models/ThemeConfig/Conversion/Validation.ts b/src/Models/ThemeConfig/Conversion/Validation.ts index ad7a30b57e..33e35f3ee9 100644 --- a/src/Models/ThemeConfig/Conversion/Validation.ts +++ b/src/Models/ThemeConfig/Conversion/Validation.ts @@ -18,6 +18,8 @@ import { QuestionableTagRenderingConfigJson } from "../Json/QuestionableTagRende import Validators from "../../../UI/InputElement/Validators" import TagRenderingConfig from "../TagRenderingConfig" import { parse as parse_html } from "node-html-parser" +import PresetConfig from "../PresetConfig" +import { TagsFilter } from "../../../Logic/Tags/TagsFilter" class ValidateLanguageCompleteness extends DesugaringStep { private readonly _languages: string[] @@ -167,9 +169,9 @@ class ValidateTheme extends DesugaringStep { json: LayoutConfigJson, context: string ): { result: LayoutConfigJson; errors: string[]; warnings: string[]; information: string[] } { - const errors = [] - const warnings = [] - const information = [] + const errors: string[] = [] + const warnings: string[] = [] + const information: string[] = [] const theme = new LayoutConfig(json, this._isBuiltin) @@ -245,7 +247,7 @@ class ValidateTheme extends DesugaringStep { information ) } - const dups = Utils.Dupiclates(json.layers.map((layer) => layer["id"])) + const dups = Utils.Duplicates(json.layers.map((layer) => layer["id"])) if (dups.length > 0) { errors.push( `The theme ${json.id} defines multiple layers with id ${dups.join(", ")}` @@ -275,6 +277,10 @@ class ValidateTheme extends DesugaringStep { errors.push(e) } + if (theme.id !== "personal") { + new DetectDuplicatePresets().convertJoin(theme, context, errors, warnings, information) + } + return { result: json, errors, @@ -889,7 +895,7 @@ export class ValidateLayer extends DesugaringStep { { // duplicate ids in tagrenderings check const duplicates = Utils.Dedup( - Utils.Dupiclates(Utils.NoNull((json.tagRenderings ?? []).map((tr) => tr["id"]))) + Utils.Duplicates(Utils.NoNull((json.tagRenderings ?? []).map((tr) => tr["id"]))) ) if (duplicates.length > 0) { console.log(json.tagRenderings) @@ -985,7 +991,7 @@ export class ValidateLayer extends DesugaringStep { ) } - const duplicateIds = Utils.Dupiclates( + const duplicateIds = Utils.Duplicates( (json.tagRenderings ?? []) ?.map((f) => f["id"]) .filter((id) => id !== "questions") @@ -1243,3 +1249,68 @@ export class DetectDuplicateFilters extends DesugaringStep<{ } } } + +export class DetectDuplicatePresets extends DesugaringStep { + constructor() { + super( + "Detects mappings which have identical (english) names or identical mappings.", + ["presets"], + "DetectDuplicatePresets" + ) + } + convert( + json: LayoutConfig, + context: string + ): { + result: LayoutConfig + errors?: string[] + warnings?: string[] + information?: string[] + } { + const presets: PresetConfig[] = [].concat(...json.layers.map((l) => l.presets)) + + const errors = [] + const enNames = presets.map((p) => p.title.textFor("en")) + if (new Set(enNames).size != enNames.length) { + const dups = Utils.Duplicates(enNames) + const layersWithDup = json.layers.filter((l) => + l.presets.some((p) => dups.indexOf(p.title.textFor("en")) >= 0) + ) + const layerIds = layersWithDup.map((l) => l.id) + errors.push( + `At ${context}: this themes has multiple presets which are named:${dups}, namely layers ${layerIds.join( + ", " + )} this is confusing for contributors and is probably the result of reusing the same layer multiple times. Use \`{"override": {"=presets": []}}\` to remove some presets` + ) + } + + const optimizedTags = presets.map((p) => new And(p.tags).optimize()) + for (let i = 0; i < presets.length; i++) { + const presetATags = optimizedTags[i] + const presetA = presets[i] + for (let j = i + 1; j < presets.length; j++) { + const presetBTags = optimizedTags[j] + const presetB = presets[j] + if ( + Utils.SameObject(presetATags, presetBTags) && + Utils.sameList( + presetA.preciseInput.snapToLayers, + presetB.preciseInput.snapToLayers + ) + ) { + errors.push( + `At ${context}: this themes has multiple presets with the same tags: ${presetATags.asHumanString( + false, + false, + {} + )}, namely the preset '${presets[i].title.textFor("en")}' and '${presets[ + j + ].title.textFor("en")}'` + ) + } + } + } + + return { errors, result: json } + } +} diff --git a/src/Models/ThemeConfig/LayerConfig.ts b/src/Models/ThemeConfig/LayerConfig.ts index 06d87f7a9e..13e0b84ef2 100644 --- a/src/Models/ThemeConfig/LayerConfig.ts +++ b/src/Models/ThemeConfig/LayerConfig.ts @@ -359,7 +359,7 @@ export default class LayerConfig extends WithContextLoader { } { - const duplicateIds = Utils.Dupiclates(this.filters.map((f) => f.id)) + const duplicateIds = Utils.Duplicates(this.filters.map((f) => f.id)) if (duplicateIds.length > 0) { throw `Some filters have a duplicate id: ${duplicateIds} (at ${context}.filters)` } diff --git a/src/Utils.ts b/src/Utils.ts index 415f81d205..85ab51f361 100644 --- a/src/Utils.ts +++ b/src/Utils.ts @@ -387,7 +387,7 @@ In the case that MapComplete is pointed to the testing grounds, the edit will be return newArr } - public static Dupiclates(arr: string[]): string[] { + public static Duplicates(arr: string[]): string[] { if (arr === undefined) { return undefined }