From 079a3f86940ac51c3533ecd4568c2edf81b48515 Mon Sep 17 00:00:00 2001 From: Pieter Vander Vennet Date: Fri, 19 Jan 2024 17:31:35 +0100 Subject: [PATCH] Studio: improve error handling, fix renumbering --- .../ThemeConfig/Conversion/Conversion.ts | 15 ++-- .../Conversion/ConversionContext.ts | 26 +++++++ .../ThemeConfig/Conversion/Validation.ts | 68 +++++++++++++------ src/UI/Studio/EditLayer.svelte | 2 +- src/UI/Studio/EditLayerState.ts | 67 ++++++++++++++++-- src/UI/Studio/ErrorIndicatorForRegion.svelte | 2 +- src/UI/Studio/MappingInput.svelte | 9 ++- src/UI/Studio/StudioServer.ts | 5 +- src/UI/Studio/TagRenderingInput.svelte | 15 ++-- src/UI/StudioGUI.svelte | 40 ++++++----- 10 files changed, 187 insertions(+), 62 deletions(-) diff --git a/src/Models/ThemeConfig/Conversion/Conversion.ts b/src/Models/ThemeConfig/Conversion/Conversion.ts index d74028731..867a3d8f8 100644 --- a/src/Models/ThemeConfig/Conversion/Conversion.ts +++ b/src/Models/ThemeConfig/Conversion/Conversion.ts @@ -2,7 +2,6 @@ import { LayerConfigJson } from "../Json/LayerConfigJson" import { Utils } from "../../../Utils" import { QuestionableTagRenderingConfigJson } from "../Json/QuestionableTagRenderingConfigJson" import { ConversionContext } from "./ConversionContext" -import { T } from "vitest/dist/types-aac763a5" export interface DesugaringContext { tagRenderings: Map @@ -11,10 +10,11 @@ export interface DesugaringContext { } export type ConversionMsgLevel = "debug" | "information" | "warning" | "error" + export interface ConversionMessage { - context: ConversionContext - message: string - level: ConversionMsgLevel + readonly context: ConversionContext + readonly message: string + readonly level: ConversionMsgLevel } export abstract class Conversion { @@ -85,6 +85,7 @@ export class Pure extends Conversion { export class Bypass extends DesugaringStep { private readonly _applyIf: (t: T) => boolean private readonly _step: DesugaringStep + constructor(applyIf: (t: T) => boolean, step: DesugaringStep) { super("Applies the step on the object, if the object satisfies the predicate", [], "Bypass") this._applyIf = applyIf @@ -102,7 +103,6 @@ export class Bypass extends DesugaringStep { export class Each extends Conversion { private readonly _step: Conversion private readonly _msg: string - private readonly _filter: (x: X) => boolean constructor(step: Conversion, options?: { msg?: string }) { super( @@ -224,6 +224,7 @@ export class FirstOf extends Conversion { export class Cached extends Conversion { private _step: Conversion private readonly key: string + constructor(step: Conversion) { super("Secretly caches the output for the given input", [], "cached") this._step = step @@ -242,9 +243,11 @@ export class Cached extends Conversion { return converted } } + export class Fuse extends DesugaringStep { - private readonly steps: DesugaringStep[] protected debug = false + private readonly steps: DesugaringStep[] + constructor(doc: string, ...steps: DesugaringStep[]) { super( (doc ?? "") + diff --git a/src/Models/ThemeConfig/Conversion/ConversionContext.ts b/src/Models/ThemeConfig/Conversion/ConversionContext.ts index 2a0e5e848..db4bed4fd 100644 --- a/src/Models/ThemeConfig/Conversion/ConversionContext.ts +++ b/src/Models/ThemeConfig/Conversion/ConversionContext.ts @@ -1,4 +1,5 @@ import { ConversionMessage, ConversionMsgLevel } from "./Conversion" +import { Context } from "maplibre-gl" export class ConversionContext { /** @@ -42,6 +43,31 @@ export class ConversionContext { return new ConversionContext([], msg ? [msg] : [], ["test"]) } + /** + * Does an inline edit of the messages for which a new path is defined + * This is a slight hack + * @param rewritePath + */ + public rewriteMessages( + rewritePath: ( + p: ReadonlyArray + ) => undefined | ReadonlyArray + ): void { + for (let i = 0; i < this.messages.length; i++) { + const m = this.messages[i] + const newPath = rewritePath(m.context.path) + if (!newPath) { + continue + } + const rewrittenContext = new ConversionContext( + this.messages, + newPath, + m.context.operation + ) + this.messages[i] = { ...m, context: rewrittenContext } + } + } + static print(msg: ConversionMessage) { const noString = msg.context.path.filter( (p) => typeof p !== "string" && typeof p !== "number" diff --git a/src/Models/ThemeConfig/Conversion/Validation.ts b/src/Models/ThemeConfig/Conversion/Validation.ts index ed4c79098..2adf0f023 100644 --- a/src/Models/ThemeConfig/Conversion/Validation.ts +++ b/src/Models/ThemeConfig/Conversion/Validation.ts @@ -13,7 +13,10 @@ import { And } from "../../../Logic/Tags/And" import Translations from "../../../UI/i18n/Translations" import FilterConfigJson from "../Json/FilterConfigJson" import DeleteConfig from "../DeleteConfig" -import { QuestionableTagRenderingConfigJson } from "../Json/QuestionableTagRenderingConfigJson" +import { + MappingConfigJson, + QuestionableTagRenderingConfigJson, +} from "../Json/QuestionableTagRenderingConfigJson" import Validators from "../../../UI/InputElement/Validators" import TagRenderingConfig from "../TagRenderingConfig" import { parse as parse_html } from "node-html-parser" @@ -21,9 +24,7 @@ import PresetConfig from "../PresetConfig" import { TagsFilter } from "../../../Logic/Tags/TagsFilter" import { Translatable } from "../Json/Translatable" import { ConversionContext } from "./ConversionContext" -import * as eli from "../../../assets/editor-layer-index.json" import { AvailableRasterLayers } from "../../RasterLayers" -import Back from "../../../assets/svg/Back.svelte" import PointRenderingConfigJson from "../Json/PointRenderingConfigJson" class ValidateLanguageCompleteness extends DesugaringStep { @@ -178,7 +179,7 @@ export class ValidateTheme extends DesugaringStep { if (!json.title) { context.enter("title").err(`The theme ${json.id} does not have a title defined.`) } - if(!json.icon){ + if (!json.icon) { context.enter("icon").err("A theme should have an icon") } if (this._isBuiltin && this._extractImages !== undefined) { @@ -831,6 +832,7 @@ class MiscTagRenderingChecks extends DesugaringStep { json: TagRenderingConfigJson | QuestionableTagRenderingConfigJson, context: ConversionContext ): TagRenderingConfigJson { + console.log(">>> Validating TR", context.path.join("."), json) if (json["special"] !== undefined) { context.err( 'Detected `special` on the top level. Did you mean `{"render":{ "special": ... }}`' @@ -848,13 +850,32 @@ class MiscTagRenderingChecks extends DesugaringStep { CheckTranslation.allowUndefined.convert(json[key], context.enter(key)) } for (let i = 0; i < json.mappings?.length ?? 0; i++) { - const mapping = json.mappings[i] + const mapping: MappingConfigJson = json.mappings[i] CheckTranslation.noUndefined.convert( mapping.then, context.enters("mappings", i, "then") ) if (!mapping.if) { - context.enters("mappings", i).err("No `if` is defined") + console.log( + "Checking mappings", + i, + "if", + mapping.if, + context.path.join("."), + mapping.then + ) + context.enters("mappings", i, "if").err("No `if` is defined") + } + if (mapping.addExtraTags) { + for (let j = 0; j < mapping.addExtraTags.length; j++) { + if (!mapping.addExtraTags[j]) { + context + .enters("mappings", i, "addExtraTags", j) + .err( + "Detected a 'null' or 'undefined' value. Either specify a tag or delete this item" + ) + } + } } const en = mapping?.then?.["en"] if (en && this.detectYesOrNo(en)) { @@ -977,6 +998,9 @@ class MiscTagRenderingChecks extends DesugaringStep { } } + if (context.hasErrors()) { + return undefined + } return json } @@ -996,6 +1020,7 @@ export class ValidateTagRenderings extends Fuse { constructor(layerConfig?: LayerConfigJson, doesImageExist?: DoesImageExist) { super( "Various validation on tagRenderingConfigs", + new MiscTagRenderingChecks(), new DetectShadowedMappings(layerConfig), new DetectConflictingAddExtraTags(), // TODO enable new DetectNonErasedKeysInMappings(), @@ -1003,8 +1028,7 @@ export class ValidateTagRenderings extends Fuse { new On("render", new ValidatePossibleLinks()), new On("question", new ValidatePossibleLinks()), new On("questionHint", new ValidatePossibleLinks()), - new On("mappings", new Each(new On("then", new ValidatePossibleLinks()))), - new MiscTagRenderingChecks() + new On("mappings", new Each(new On("then", new ValidatePossibleLinks()))) ) } } @@ -1107,7 +1131,9 @@ export class PrevalidateLayer extends DesugaringStep { context.enter("pointRendering").err("There are no pointRenderings at all...") } - json.pointRendering?.forEach((pr,i) => this._validatePointRendering.convert(pr, context.enters("pointeRendering", i))) + json.pointRendering?.forEach((pr, i) => + this._validatePointRendering.convert(pr, context.enters("pointeRendering", i)) + ) if (json["mapRendering"]) { context.enter("mapRendering").err("This layer has a legacy 'mapRendering'") @@ -1134,7 +1160,7 @@ export class PrevalidateLayer extends DesugaringStep { } if (json.tagRenderings !== undefined && json.tagRenderings.length > 0) { - new On("tagRendering", new Each(new ValidateTagRenderings(json))) + new On("tagRenderings", new Each(new ValidateTagRenderings(json))) if (json.title === undefined && json.source !== "special:library") { context .enter("title") @@ -1424,29 +1450,33 @@ class ValidatePointRendering extends DesugaringStep { } if (json["markers"]) { - context.enter("markers").err(`Detected a field 'markerS' in pointRendering. It is written as a singular case`) + context + .enter("markers") + .err( + `Detected a field 'markerS' in pointRendering. It is written as a singular case` + ) } if (json.marker && !Array.isArray(json.marker)) { - context.enter("marker").err( - "The marker in a pointRendering should be an array" - ) + context.enter("marker").err("The marker in a pointRendering should be an array") } if (json.location.length == 0) { - context.enter("location").err ( - "A pointRendering should have at least one 'location' to defined where it should be rendered. " - ) + context + .enter("location") + .err( + "A pointRendering should have at least one 'location' to defined where it should be rendered. " + ) } return json - - } } + export class ValidateLayer extends Conversion< LayerConfigJson, { parsed: LayerConfig; raw: LayerConfigJson } > { private readonly _skipDefaultLayers: boolean private readonly _prevalidation: PrevalidateLayer + constructor( path: string, isBuiltin: boolean, diff --git a/src/UI/Studio/EditLayer.svelte b/src/UI/Studio/EditLayer.svelte index 5d9aedbaf..fc74eb3ab 100644 --- a/src/UI/Studio/EditLayer.svelte +++ b/src/UI/Studio/EditLayer.svelte @@ -180,7 +180,7 @@
Advanced functionality - +
diff --git a/src/UI/Studio/EditLayerState.ts b/src/UI/Studio/EditLayerState.ts index 557ec2109..b52e6e85a 100644 --- a/src/UI/Studio/EditLayerState.ts +++ b/src/UI/Studio/EditLayerState.ts @@ -22,6 +22,7 @@ import { LayoutConfigJson } from "../../Models/ThemeConfig/Json/LayoutConfigJson import { PrepareTheme } from "../../Models/ThemeConfig/Conversion/PrepareTheme" import { ConversionContext } from "../../Models/ThemeConfig/Conversion/ConversionContext" import { LocalStorageSource } from "../../Logic/Web/LocalStorageSource" +import { TagRenderingConfigJson } from "../../Models/ThemeConfig/Json/TagRenderingConfigJson" export interface HighlightedTagRendering { path: ReadonlyArray @@ -66,7 +67,6 @@ export abstract class EditJsonState { this.messages = this.setupErrorsForLayers() const layerId = this.getId() - this.highlightedItem.addCallbackD((hl) => console.log("Highlighted item is", hl)) this.configuration .mapD((config) => { if (!this.sendingUpdates) { @@ -110,6 +110,7 @@ export abstract class EditJsonState { public async delete() { await this.server.delete(this.getId().data, this.category) } + public getStoreFor(path: ReadonlyArray): UIEventSource { const key = path.join(".") @@ -172,7 +173,6 @@ export abstract class EditJsonState { public setValueAt(path: ReadonlyArray, v: any) { let entry = this.configuration.data - console.trace("Setting value at", path,"to",v) const isUndefined = v === undefined || v === null || @@ -249,6 +249,62 @@ export abstract class EditJsonState { } } +class ContextRewritingStep extends Conversion { + private readonly _step: Conversion + private readonly _state: DesugaringContext + private readonly _getTagRenderings: (t: T) => TagRenderingConfigJson[] + + constructor( + state: DesugaringContext, + step: Conversion, + getTagRenderings: (t: T) => TagRenderingConfigJson[] + ) { + super( + "When validating a layer, the tagRenderings are first expanded. Some builtin tagRendering-calls (e.g. `contact`) will introduce _multiple_ tagRenderings, causing the count to be off. This class rewrites the error messages to fix this", + [], + "ContextRewritingStep" + ) + this._state = state + this._step = step + this._getTagRenderings = getTagRenderings + } + + convert(json: LayerConfigJson, context: ConversionContext): T { + const converted = this._step.convert(json, context) + const originalIds = json.tagRenderings?.map( + (tr) => (tr)["id"] + ) + if (!originalIds) { + return converted + } + + let newTagRenderings: TagRenderingConfigJson[] + if (converted === undefined) { + const prepared = new PrepareLayer(this._state) + newTagRenderings = ( + prepared.convert(json, context).tagRenderings + ) + } else { + newTagRenderings = this._getTagRenderings(converted) + } + context.rewriteMessages((path) => { + if (path[0] !== "tagRenderings") { + return undefined + } + const newPath = [...path] + const idToSearch = newTagRenderings[newPath[1]].id + const oldIndex = originalIds.indexOf(idToSearch) + if (oldIndex < 0) { + console.warn("Original ID was not found: ", idToSearch) + return undefined // We don't modify the message + } + newPath[1] = oldIndex + return newPath + }) + return converted + } +} + export default class EditLayerState extends EditJsonState { // Needed for the special visualisations public readonly osmConnection: OsmConnection @@ -334,9 +390,10 @@ export default class EditLayerState extends EditJsonState { } protected buildValidation(state: DesugaringContext) { - return new Pipe( - new PrepareLayer(state), - new ValidateLayer("dynamic", false, undefined, true) + return new ContextRewritingStep( + state, + new Pipe(new PrepareLayer(state), new ValidateLayer("dynamic", false, undefined, true)), + (t) => t.raw.tagRenderings ) } diff --git a/src/UI/Studio/ErrorIndicatorForRegion.svelte b/src/UI/Studio/ErrorIndicatorForRegion.svelte index e3fd4c084..b1983db5d 100644 --- a/src/UI/Studio/ErrorIndicatorForRegion.svelte +++ b/src/UI/Studio/ErrorIndicatorForRegion.svelte @@ -2,7 +2,7 @@ import EditLayerState from "./EditLayerState" import { ExclamationIcon } from "@rgossiaux/svelte-heroicons/solid" - export let firstPaths: Set + export let firstPaths: Set export let state: EditLayerState let messagesCount = state.messages.map( (msgs) => diff --git a/src/UI/Studio/MappingInput.svelte b/src/UI/Studio/MappingInput.svelte index f36984085..d210811d4 100644 --- a/src/UI/Studio/MappingInput.svelte +++ b/src/UI/Studio/MappingInput.svelte @@ -11,9 +11,11 @@ import { Utils } from "../../Utils" import ToSvelte from "../Base/ToSvelte.svelte" import { VariableUiElement } from "../Base/VariableUIElement" + import { ExclamationTriangle } from "@babeard/svelte-heroicons/solid/ExclamationTriangle" export let state: EditLayerState export let path: (string | number)[] + let messages = state.messagesFor(path) let tag: UIEventSource = state.getStoreFor([...path, "if"]) let parsedTag = tag.map((t) => (t ? TagUtils.Tag(t) : undefined)) let exampleTags = parsedTag.map((pt) => { @@ -27,7 +29,6 @@ } return o }) - let uploadableOnly: boolean = true let thenText: UIEventSource> = state.getStoreFor([...path, "then"]) let thenTextEn = thenText.mapD((translation) => @@ -71,5 +72,11 @@ No then is set {/if} + {#if $messages.length > 0} +
+ + {$messages.length} errors +
+ {/if}
{/if} diff --git a/src/UI/Studio/StudioServer.ts b/src/UI/Studio/StudioServer.ts index 2c438f01b..1256e0895 100644 --- a/src/UI/Studio/StudioServer.ts +++ b/src/UI/Studio/StudioServer.ts @@ -2,6 +2,7 @@ import { Utils } from "../../Utils" import Constants from "../../Models/Constants" import { LayerConfigJson } from "../../Models/ThemeConfig/Json/LayerConfigJson" import { Store } from "../../Logic/UIEventSource" +import { LayoutConfigJson } from "../../Models/ThemeConfig/Json/LayoutConfigJson" export default class StudioServer { private readonly url: string @@ -47,11 +48,13 @@ export default class StudioServer { return layerOverview } + async fetch(layerId: string, category: "layers", uid?: number): Promise + async fetch(layerId: string, category: "themes", uid?: number): Promise async fetch( layerId: string, category: "layers" | "themes", uid?: number - ): Promise { + ): Promise { try { return await Utils.downloadJson(this.urlFor(layerId, category, uid)) } catch (e) { diff --git a/src/UI/Studio/TagRenderingInput.svelte b/src/UI/Studio/TagRenderingInput.svelte index 41e01a220..ee43f20d1 100644 --- a/src/UI/Studio/TagRenderingInput.svelte +++ b/src/UI/Studio/TagRenderingInput.svelte @@ -24,13 +24,13 @@ import { onMount } from "svelte" export let state: EditLayerState - export let schema: ConfigMeta - export let path: (string | number)[] + export let path: ReadonlyArray + let messages = state.messagesFor(path) let expertMode = state.expertMode const store = state.getStoreFor(path) let value = store.data let hasSeenIntro = UIEventSource.asBoolean( - LocalStorageSource.Get("studio-seen-tagrendering-tutorial", "false") + LocalStorageSource.Get("studio-seen-tagrendering-tutorial", "false"), ) onMount(() => { if (!hasSeenIntro.data) { @@ -43,7 +43,7 @@ * Should only be enabled for 'tagrenderings' in the theme, if the source is OSM */ let allowQuestions: Store = state.configuration.mapD( - (config) => path.at(0) === "tagRenderings" && config.source?.geoJson === undefined + (config) => path.at(0) === "tagRenderings" && config.source?.["geoJson"] === undefined, ) let mappingsBuiltin: MappingConfigJson[] = [] @@ -119,7 +119,7 @@ const freeformSchemaAll = ( questionableTagRenderingSchemaRaw.filter( - (schema) => schema.path.length == 2 && schema.path[0] === "freeform" && $allowQuestions + (schema) => schema.path.length == 2 && schema.path[0] === "freeform" && $allowQuestions, ) ) let freeformSchema = $expertMode @@ -128,7 +128,7 @@ const missing: string[] = questionableTagRenderingSchemaRaw .filter( (schema) => - schema.path.length >= 1 && !items.has(schema.path[0]) && !ignored.has(schema.path[0]) + schema.path.length >= 1 && !items.has(schema.path[0]) && !ignored.has(schema.path[0]), ) .map((schema) => schema.path.join(".")) console.log({ state }) @@ -164,7 +164,7 @@ {/if} {#each $mappings ?? [] as mapping, i (mapping)}
- +
+ {/each}