From b43d9760582665e1bb6a02cc42a79115ba0c77c2 Mon Sep 17 00:00:00 2001 From: pietervdvn Date: Wed, 13 Apr 2022 00:31:13 +0200 Subject: [PATCH] Fix bug: override preserves null's again --- .../Conversion/AddContextToTranslations.ts | 29 +++- Models/ThemeConfig/Conversion/Conversion.ts | 14 ++ Models/ThemeConfig/Conversion/PrepareTheme.ts | 127 +++++++++--------- Utils.ts | 28 +++- .../mapcomplete-changes.json | 33 ++--- .../Conversion/PrepareTheme.spec.ts | 61 ++++++++- 6 files changed, 205 insertions(+), 87 deletions(-) diff --git a/Models/ThemeConfig/Conversion/AddContextToTranslations.ts b/Models/ThemeConfig/Conversion/AddContextToTranslations.ts index 9fc3cd1e3a..8d9a953905 100644 --- a/Models/ThemeConfig/Conversion/AddContextToTranslations.ts +++ b/Models/ThemeConfig/Conversion/AddContextToTranslations.ts @@ -38,16 +38,43 @@ export class AddContextToTranslations extends DesugaringStep { * ] * } * rewritten // => expected + * + * // should preserve nulls + * const theme = { + * layers: [ + * { + * builtin: ["abc"], + * override: { + * name:null + * } + * } + * ] + * } + * const rewritten = new AddContextToTranslations("prefix:").convert(theme, "context").result + * const expected = { + * layers: [ + * { + * builtin: ["abc"], + * override: { + * name: null + * } + * } + * ] + * } + * rewritten // => expected */ convert(json: T, context: string): { result: T; errors?: string[]; warnings?: string[]; information?: string[] } { const result = Utils.WalkJson(json, (leaf, path) => { + if(leaf === undefined || leaf === null){ + return leaf + } if (typeof leaf === "object") { return {...leaf, _context: this._prefix + context + "." + path.join(".")} } else { return leaf } - }, obj => obj !== undefined && obj !== null && Translations.isProbablyATranslation(obj)) + }, obj => obj === undefined || obj === null || Translations.isProbablyATranslation(obj)) return { result diff --git a/Models/ThemeConfig/Conversion/Conversion.ts b/Models/ThemeConfig/Conversion/Conversion.ts index f947ed916e..3d9aa1183a 100644 --- a/Models/ThemeConfig/Conversion/Conversion.ts +++ b/Models/ThemeConfig/Conversion/Conversion.ts @@ -158,6 +158,20 @@ export class On extends DesugaringStep { } } +export class Pass extends Conversion { + constructor(message?: string) { + super(message??"Does nothing, often to swap out steps in testing", [], "Pass"); + } + + + convert(json: T, context: string): { result: T; errors?: string[]; warnings?: string[]; information?: string[] } { + return { + result: json + }; + } + +} + export class Concat extends Conversion { private readonly _step: Conversion; diff --git a/Models/ThemeConfig/Conversion/PrepareTheme.ts b/Models/ThemeConfig/Conversion/PrepareTheme.ts index d2cf44bcae..7c1aa17211 100644 --- a/Models/ThemeConfig/Conversion/PrepareTheme.ts +++ b/Models/ThemeConfig/Conversion/PrepareTheme.ts @@ -1,4 +1,4 @@ -import {Concat, Conversion, DesugaringContext, DesugaringStep, Each, Fuse, On, SetDefault} from "./Conversion"; +import {Concat, Conversion, DesugaringContext, DesugaringStep, Each, Fuse, On, Pass, SetDefault} from "./Conversion"; import {LayoutConfigJson} from "../Json/LayoutConfigJson"; import {PrepareLayer} from "./PrepareLayer"; import {LayerConfigJson} from "../Json/LayerConfigJson"; @@ -13,28 +13,30 @@ import {AddContextToTranslations} from "./AddContextToTranslations"; class SubstituteLayer extends Conversion<(string | LayerConfigJson), LayerConfigJson[]> { private readonly _state: DesugaringContext; + constructor( state: DesugaringContext, ) { - super("Converts the identifier of a builtin layer into the actual layer, or converts a 'builtin' syntax with override in the fully expanded form", [],"SubstituteLayer"); + super("Converts the identifier of a builtin layer into the actual layer, or converts a 'builtin' syntax with override in the fully expanded form", [], "SubstituteLayer"); this._state = state; } - + convert(json: string | LayerConfigJson, context: string): { result: LayerConfigJson[]; errors: string[], information?: string[] } { const errors = [] const information = [] - const state= this._state - function reportNotFound(name: string){ + const state = this._state + + function reportNotFound(name: string) { const knownLayers = Array.from(state.sharedLayers.keys()) - const withDistance = knownLayers.map(lname => [lname, Utils.levenshteinDistance(name, lname)]) + const withDistance = knownLayers.map(lname => [lname, Utils.levenshteinDistance(name, lname)]) withDistance.sort((a, b) => a[1] - b[1]) const ids = withDistance.map(n => n[0]) - // Known builtin layers are "+.join(",")+"\n For more information, see " + // Known builtin layers are "+.join(",")+"\n For more information, see " errors.push(`${context}: The layer with name ${name} was not found as a builtin layer. Perhaps you meant ${ids[0]}, ${ids[1]} or ${ids[2]}? For an overview of all available layers, refer to https://github.com/pietervdvn/MapComplete/blob/develop/Docs/BuiltinLayers.md`) } - - + + if (typeof json === "string") { const found = state.sharedLayers.get(json) if (found === undefined) { @@ -72,40 +74,40 @@ class SubstituteLayer extends Conversion<(string | LayerConfigJson), LayerConfig } catch (e) { errors.push(`At ${context}: could not apply an override due to: ${e}.\nThe override is: ${JSON.stringify(json["override"],)}`) } - - if(json["hideTagRenderingsWithLabels"]){ + + if (json["hideTagRenderingsWithLabels"]) { const hideLabels: Set = new Set(json["hideTagRenderingsWithLabels"]) // These labels caused at least one deletion - const usedLabels : Set = new Set(); + const usedLabels: Set = new Set(); const filtered = [] for (const tr of found.tagRenderings) { const labels = tr["labels"] - if(labels !== undefined){ + if (labels !== undefined) { const forbiddenLabel = labels.findIndex(l => hideLabels.has(l)) - if(forbiddenLabel >= 0){ + if (forbiddenLabel >= 0) { usedLabels.add(labels[forbiddenLabel]) - information.push(context+": Dropping tagRendering "+tr["id"]+" as it has a forbidden label: "+labels[forbiddenLabel]) + information.push(context + ": Dropping tagRendering " + tr["id"] + " as it has a forbidden label: " + labels[forbiddenLabel]) continue } } - - if(hideLabels.has(tr["id"])){ + + if (hideLabels.has(tr["id"])) { usedLabels.add(tr["id"]) - information.push(context+": Dropping tagRendering "+tr["id"]+" as its id is a forbidden label") + information.push(context + ": Dropping tagRendering " + tr["id"] + " as its id is a forbidden label") continue } - if(hideLabels.has(tr["group"])){ + if (hideLabels.has(tr["group"])) { usedLabels.add(tr["group"]) - information.push(context+": Dropping tagRendering "+tr["id"]+" as its group `"+tr["group"]+"` is a forbidden label") + information.push(context + ": Dropping tagRendering " + tr["id"] + " as its group `" + tr["group"] + "` is a forbidden label") continue } filtered.push(tr) } const unused = Array.from(hideLabels).filter(l => !usedLabels.has(l)) - if(unused.length > 0){ - errors.push("This theme specifies that certain tagrenderings have to be removed based on forbidden layers. One or more of these layers did not match any tagRenderings and caused no deletions: "+unused.join(", ")+"\n This means that this label can be removed or that the original tagRendering that should be deleted does not have this label anymore") + if (unused.length > 0) { + errors.push("This theme specifies that certain tagrenderings have to be removed based on forbidden layers. One or more of these layers did not match any tagRenderings and caused no deletions: " + unused.join(", ") + "\n This means that this label can be removed or that the original tagRendering that should be deleted does not have this label anymore") } found.tagRenderings = filtered } @@ -130,7 +132,7 @@ class AddDefaultLayers extends DesugaringStep { private _state: DesugaringContext; constructor(state: DesugaringContext) { - super("Adds the default layers, namely: " + Constants.added_by_default.join(", "), ["layers"],"AddDefaultLayers"); + super("Adds the default layers, namely: " + Constants.added_by_default.join(", "), ["layers"], "AddDefaultLayers"); this._state = state; } @@ -147,8 +149,8 @@ class AddDefaultLayers extends DesugaringStep { errors.push("Default layer " + layerName + " not found") continue } - if(alreadyLoaded.has(v.id)){ - warnings.push("Layout "+context+" already has a layer with name "+v.id+"; skipping inclusion of this builtin layer") + if (alreadyLoaded.has(v.id)) { + warnings.push("Layout " + context + " already has a layer with name " + v.id + "; skipping inclusion of this builtin layer") continue } json.layers.push(v) @@ -165,7 +167,7 @@ class AddDefaultLayers extends DesugaringStep { class AddImportLayers extends DesugaringStep { constructor() { - super("For every layer in the 'layers'-list, create a new layer which'll import notes. (Note that priviliged layers and layers which have a geojson-source set are ignored)", ["layers"],"AddImportLayers"); + super("For every layer in the 'layers'-list, create a new layer which'll import notes. (Note that priviliged layers and layers which have a geojson-source set are ignored)", ["layers"], "AddImportLayers"); } convert(json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors: string[] } { @@ -176,7 +178,7 @@ class AddImportLayers extends DesugaringStep { json.layers = [...json.layers] - if(json.enableNoteImports ?? true) { + if (json.enableNoteImports ?? true) { const creator = new CreateNoteImportLayer() for (let i1 = 0; i1 < allLayers.length; i1++) { const layer = allLayers[i1]; @@ -222,15 +224,16 @@ class AddImportLayers extends DesugaringStep { export class AddMiniMap extends DesugaringStep { private readonly _state: DesugaringContext; - constructor(state: DesugaringContext, ) { - super("Adds a default 'minimap'-element to the tagrenderings if none of the elements define such a minimap", ["tagRenderings"],"AddMiniMap"); + + constructor(state: DesugaringContext,) { + super("Adds a default 'minimap'-element to the tagrenderings if none of the elements define such a minimap", ["tagRenderings"], "AddMiniMap"); this._state = state; } /** * Returns true if this tag rendering has a minimap in some language. * Note: this minimap can be hidden by conditions - * + * * AddMiniMap.hasMinimap({render: "{minimap()}"}) // => true * AddMiniMap.hasMinimap({render: {en: "{minimap()}"}}) // => true * AddMiniMap.hasMinimap({render: {en: "{minimap()}", nl: "{minimap()}"}}) // => true @@ -280,23 +283,23 @@ export class AddMiniMap extends DesugaringStep { } } -class AddContextToTransltionsInLayout extends DesugaringStep { - +class AddContextToTransltionsInLayout extends DesugaringStep { + constructor() { - super("Adds context to translations, including the prefix 'themes:json.id'; this is to make sure terms in an 'overrides' or inline layer are linkable too",["_context"], "AddContextToTranlationsInLayout"); + super("Adds context to translations, including the prefix 'themes:json.id'; this is to make sure terms in an 'overrides' or inline layer are linkable too", ["_context"], "AddContextToTranlationsInLayout"); } - + convert(json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors?: string[]; warnings?: string[]; information?: string[] } { const conversion = new AddContextToTranslations("themes:") return conversion.convert(json, json.id); } - + } class ApplyOverrideAll extends DesugaringStep { constructor() { - super("Applies 'overrideAll' onto every 'layer'. The 'overrideAll'-field is removed afterwards", ["overrideAll", "layers"],"ApplyOverrideAll"); + super("Applies 'overrideAll' onto every 'layer'. The 'overrideAll'-field is removed afterwards", ["overrideAll", "layers"], "ApplyOverrideAll"); } convert(json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors: string[]; warnings: string[] } { @@ -325,8 +328,9 @@ class ApplyOverrideAll extends DesugaringStep { class AddDependencyLayersToTheme extends DesugaringStep { private readonly _state: DesugaringContext; - constructor(state: DesugaringContext, ) { - super("If a layer has a dependency on another layer, these layers are added automatically on the theme. (For example: defibrillator depends on 'walls_and_buildings' to snap onto. This layer is added automatically)", ["layers"],"AddDependencyLayersToTheme"); + + constructor(state: DesugaringContext,) { + super("If a layer has a dependency on another layer, these layers are added automatically on the theme. (For example: defibrillator depends on 'walls_and_buildings' to snap onto. This layer is added automatically)", ["layers"], "AddDependencyLayersToTheme"); this._state = state; } @@ -340,17 +344,17 @@ class AddDependencyLayersToTheme extends DesugaringStep { const dependencies: { neededLayer: string, reason: string, context?: string, neededBy: string }[] = [] for (const layerConfig of alreadyLoaded) { - try{ + try { const layerDeps = DependencyCalculator.getLayerDependencies(new LayerConfig(layerConfig)) dependencies.push(...layerDeps) - }catch(e){ + } catch (e) { console.error(e) - throw "Detecting layer dependencies for "+layerConfig.id+" failed due to "+e + throw "Detecting layer dependencies for " + layerConfig.id + " failed due to " + e } } for (const dependency of dependencies) { - if(loadedLayerIds.has(dependency.neededLayer)){ + if (loadedLayerIds.has(dependency.neededLayer)) { // We mark the needed layer as 'mustLoad' alreadyLoaded.find(l => l.id === dependency.neededLayer).forceLoad = true } @@ -380,7 +384,7 @@ class AddDependencyLayersToTheme extends DesugaringStep { } } while (unmetDependencies.length > 0) - + return dependenciesToAdd.map(dep => { dep = Utils.Clone(dep); dep.forceLoad = true @@ -418,46 +422,47 @@ class AddDependencyLayersToTheme extends DesugaringStep { class PreparePersonalTheme extends DesugaringStep { private readonly _state: DesugaringContext; + constructor(state: DesugaringContext) { - super("Adds every public layer to the personal theme",["layers"],"PreparePersonalTheme"); + super("Adds every public layer to the personal theme", ["layers"], "PreparePersonalTheme"); this._state = state; } convert(json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors?: string[]; warnings?: string[]; information?: string[] } { - if(json.id !== "personal"){ + if (json.id !== "personal") { return {result: json} } - + json.layers = Array.from(this._state.sharedLayers.keys()).filter(l => Constants.priviliged_layers.indexOf(l) < 0) return {result: json}; } - + } -class WarnForUnsubstitutedLayersInTheme extends DesugaringStep{ - +class WarnForUnsubstitutedLayersInTheme extends DesugaringStep { + constructor() { - super("Generates a warning if a theme uses an unsubstituted layer", ["layers"],"WarnForUnsubstitutedLayersInTheme"); + super("Generates a warning if a theme uses an unsubstituted layer", ["layers"], "WarnForUnsubstitutedLayersInTheme"); } - + convert(json: LayoutConfigJson, context: string): { result: LayoutConfigJson; errors?: string[]; warnings?: string[]; information?: string[] } { - if(json.hideFromOverview === true){ + if (json.hideFromOverview === true) { return {result: json} } const warnings = [] for (const layer of json.layers) { - if(typeof layer === "string"){ + if (typeof layer === "string") { continue } - if(layer["builtin"] !== undefined){ + if (layer["builtin"] !== undefined) { continue } - if(layer["source"]["geojson"] !== undefined){ + if (layer["source"]["geojson"] !== undefined) { // We turn a blind eye for import layers continue } - - const wrn = "The theme "+json.id+" has an inline layer: "+layer["id"]+". This is discouraged." + + const wrn = "The theme " + json.id + " has an inline layer: " + layer["id"] + ". This is discouraged." warnings.push(wrn) } return { @@ -465,11 +470,13 @@ class WarnForUnsubstitutedLayersInTheme extends DesugaringStep warnings }; } - + } export class PrepareTheme extends Fuse { - constructor(state: DesugaringContext) { + constructor(state: DesugaringContext, options?: { + skipDefaultLayers: false | boolean + }) { super( "Fully prepares and expands a theme", new AddContextToTransltionsInLayout(), @@ -483,7 +490,7 @@ export class PrepareTheme extends Fuse { new ApplyOverrideAll(), // And then we prepare all the layers _again_ in case that an override all contained unexpanded tagrenderings! new On("layers", new Each(new PrepareLayer(state))), - new AddDefaultLayers(state), + options?.skipDefaultLayers ? new Pass("AddDefaultLayers is disabled due to the set flag") : new AddDefaultLayers(state), new AddDependencyLayersToTheme(state), new AddImportLayers(), new On("layers", new Each(new AddMiniMap(state))) diff --git a/Utils.ts b/Utils.ts index 7ee06e1744..c8a7da1c9b 100644 --- a/Utils.ts +++ b/Utils.ts @@ -518,11 +518,35 @@ In the case that MapComplete is pointed to the testing grounds, the edit will be * Apply a function on every leaf of the JSON; used to rewrite parts of the JSON. * Returns a modified copy of the original object. * + * 'null' and 'undefined' are _always_ considered a leaf, even if 'isLeaf' says it isn't + * * Hangs if the object contains a loop + * + * // should walk a json + * const walked = Utils.WalkJson({ + * key: "value" + * }, (x: string) => x + "!") + * walked // => {key: "value!"} + * + * // should preserve undefined and null: + * const walked = Utils.WalkJson({ + * u: undefined, + * n: null, + * v: "value" + * }, (x) => {if(x !== undefined && x !== null){return x+"!}; return x}) + * walked // => {v: "value!", u: undefined, n: null} + * + * // should preserve undefined and null, also with a negative isLeaf: + * const walked = Utils.WalkJson({ + * u: undefined, + * n: null, + * v: "value" + * }, (x) => return x}, _ => false) + * walked // => {v: "value", u: undefined, n: null} */ static WalkJson(json: any, f: (v: object | number | string | boolean | undefined, path: string[]) => any, isLeaf: (object) => boolean = undefined, path: string[] = []) { - if (json === undefined) { - return f(undefined, path) + if (json === undefined || json === null) { + return f(json, path) } const jtp = typeof json if (isLeaf !== undefined) { diff --git a/assets/themes/mapcomplete-changes/mapcomplete-changes.json b/assets/themes/mapcomplete-changes/mapcomplete-changes.json index 4f2598a1a9..c90abd4c4d 100644 --- a/assets/themes/mapcomplete-changes/mapcomplete-changes.json +++ b/assets/themes/mapcomplete-changes/mapcomplete-changes.json @@ -1,16 +1,13 @@ { "id": "mapcomplete-changes", "title": { - "en": "Changes made with MapComplete", - "de": "Änderungen mit MapComplete" + "en": "Changes made with MapComplete" }, "shortDescription": { - "en": "Shows changes made by MapComplete", - "de": "Zeigt Änderungen von MapComplete" + "en": "Shows changes made by MapComplete" }, "description": { - "en": "This maps shows all the changes made with MapComplete", - "de": "Diese Karte zeigt alle Änderungen die mit MapComplete gemacht wurden" + "en": "This maps shows all the changes made with MapComplete" }, "maintainer": "", "icon": "./assets/svg/logo.svg", @@ -39,27 +36,23 @@ ], "title": { "render": { - "en": "Changeset for {theme}", - "de": "Änderungen für {theme}" + "en": "Changeset for {theme}" } }, "description": { - "en": "Shows all MapComplete changes", - "de": "Zeigt alle MapComplete Änderungen" + "en": "Shows all MapComplete changes" }, "tagRenderings": [ { "id": "render_id", "render": { - "en": "Changeset {id}", - "de": "Änderung {id}" + "en": "Changeset {id}" } }, { "id": "contributor", "render": { - "en": "Change made by {_last_edit:contributor}", - "de": "Änderung wurde von {_last_edit:contributor} gemacht" + "en": "Change made by {_last_edit:contributor}" } }, { @@ -335,8 +328,7 @@ } ], "question": { - "en": "Themename contains {search}", - "de": "Themenname enthält {search}" + "en": "Themename contains {search}" } } ] @@ -352,8 +344,7 @@ } ], "question": { - "en": "Made by contributor {search}", - "de": "Erstellt von {search}" + "en": "Made by contributor {search}" } } ] @@ -369,8 +360,7 @@ } ], "question": { - "en": "Not made by contributor {search}", - "de": "Nicht erstellt von {search}" + "en": "Not made by contributor {search}" } } ] @@ -385,8 +375,7 @@ { "id": "link_to_more", "render": { - "en": "More statistics can be found here", - "de": "Weitere Statistiken finden Sie hier" + "en": "More statistics can be found here" } }, { diff --git a/test/Models/ThemeConfig/Conversion/PrepareTheme.spec.ts b/test/Models/ThemeConfig/Conversion/PrepareTheme.spec.ts index b18863ce79..2a06069ca0 100644 --- a/test/Models/ThemeConfig/Conversion/PrepareTheme.spec.ts +++ b/test/Models/ThemeConfig/Conversion/PrepareTheme.spec.ts @@ -10,6 +10,7 @@ import LayerConfig from "../../../../Models/ThemeConfig/LayerConfig"; import {ExtractImages} from "../../../../Models/ThemeConfig/Conversion/FixImages"; import * as cyclofix from "../../../../assets/generated/themes/cyclofix.json" import {Tag} from "../../../../Logic/Tags/Tag"; +import {DesugaringContext} from "../../../../Models/ThemeConfig/Conversion/Conversion"; const themeConfigJson: LayoutConfigJson = { @@ -69,7 +70,6 @@ describe("PrepareTheme", () => { }) - it("should apply override", () => { const sharedLayers = new Map() @@ -82,8 +82,65 @@ describe("PrepareTheme", () => { const layerUnderTest = themeConfig.layers.find(l => l.id === "public_bookcase") expect(layerUnderTest.source.geojsonSource).eq("https://example.com/data.geojson") }) -}) + + it("should remove names which are overriden with null", () => { + const testLayer: LayerConfigJson = { + source: { + osmTags: "x=y" + }, + id: "layer-example", + name: { + en: "Test layer - please ignore" + }, + titleIcons : [], + mapRendering: null + } + const ctx: DesugaringContext = { + sharedLayers: new Map([[ + "layer-example", testLayer + ]]), + tagRenderings: new Map() + } + const layout : LayoutConfigJson= + { + description: "A testing theme", + icon: "", + id: "", + layers: [ + "layer-example", + { + "builtin": "layer-example", + "override": { + "name": null, + "minzoom": 18 + } + } + ], + maintainer: "Me", + startLat: 0, + startLon: 0, + startZoom: 0, + title: "Test theme", + version: "" + + } + const rewritten = new PrepareTheme(ctx, { + skipDefaultLayers: true + }).convertStrict(layout, "test") + expect(rewritten.layers[0]).deep.eq(testLayer) + expect(rewritten.layers[1]).deep.eq({ + source: { + osmTags: "x=y" + }, + id: "layer-example", + name:null, + minzoom: 18, + mapRendering: null, + titleIcons: [] + }) + }) +}) describe("ExtractImages", () => { it("should find all images in a themefile", () => {