From e35df654d7f2a0ef7c12c0ceef27cb3a1a7a1027 Mon Sep 17 00:00:00 2001 From: Pieter Vander Vennet Date: Sat, 12 Oct 2024 12:57:09 +0200 Subject: [PATCH] Feat: remove 'noteImportLayer', they are not used enough for the big performance and maintenance impact they have --- scripts/generateDocs.ts | 1 - scripts/generateLayerOverview.ts | 3 - src/Logic/Actors/NoElementsInViewDetector.ts | 1 - .../TiledFeatureSource/SummaryTileSource.ts | 3 +- .../Conversion/CreateNoteImportLayer.ts | 209 ------------------ .../ThemeConfig/Conversion/PrepareTheme.ts | 79 +------ src/Models/ThemeConfig/LayoutConfig.ts | 14 +- src/Models/ThemeViewState.ts | 6 +- src/UI/BigComponents/ShareScreen.svelte | 3 - .../Conversion/CreateNoteImportLayer.spec.ts | 41 ---- 10 files changed, 9 insertions(+), 351 deletions(-) delete mode 100644 src/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts delete mode 100644 test/Models/ThemeConfig/Conversion/CreateNoteImportLayer.spec.ts diff --git a/scripts/generateDocs.ts b/scripts/generateDocs.ts index 8e5330dd0..c06a3c449 100644 --- a/scripts/generateDocs.ts +++ b/scripts/generateDocs.ts @@ -459,7 +459,6 @@ export class GenerateDocs extends Script { const allLayers = AllSharedLayers.getSharedLayersConfigs() const layersToShow = theme.layers.filter( (l) => - !l.id.startsWith("note_import_") && l.id !== "favourite" && Constants.added_by_default.indexOf(l.id) < 0 ) diff --git a/scripts/generateLayerOverview.ts b/scripts/generateLayerOverview.ts index fdc973b46..3924ec560 100644 --- a/scripts/generateLayerOverview.ts +++ b/scripts/generateLayerOverview.ts @@ -304,9 +304,6 @@ class LayerOverviewUtils extends Script { if(sharedLayers.has(l.id)){ continue } - if(l.id.startsWith("note_import")){ - continue - } LayerOverviewUtils.mergeKeywords(keywords, this.layerKeywords(l)) } diff --git a/src/Logic/Actors/NoElementsInViewDetector.ts b/src/Logic/Actors/NoElementsInViewDetector.ts index 475a9b4d2..bc5148335 100644 --- a/src/Logic/Actors/NoElementsInViewDetector.ts +++ b/src/Logic/Actors/NoElementsInViewDetector.ts @@ -16,7 +16,6 @@ export default class NoElementsInViewDetector { const minZoom = Math.min( ...themeViewState.layout.layers .filter((l) => Constants.priviliged_layers.indexOf(l.id) < 0) - .filter((l) => !l.id.startsWith("note_import")) .map((l) => l.minzoom) ) const mapProperties = themeViewState.mapProperties diff --git a/src/Logic/FeatureSource/TiledFeatureSource/SummaryTileSource.ts b/src/Logic/FeatureSource/TiledFeatureSource/SummaryTileSource.ts index fcdf7e819..41c08ff74 100644 --- a/src/Logic/FeatureSource/TiledFeatureSource/SummaryTileSource.ts +++ b/src/Logic/FeatureSource/TiledFeatureSource/SummaryTileSource.ts @@ -24,8 +24,7 @@ export class SummaryTileSourceRewriter implements FeatureSource { ) { this.filteredLayers = Array.from(filteredLayers.values()).filter( (l) => - Constants.priviliged_layers.indexOf(l.layerDef.id) < 0 && - !l.layerDef.id.startsWith("note_import") + Constants.priviliged_layers.indexOf(l.layerDef.id) < 0 ) this._summarySource = summarySource filteredLayers.forEach((v) => { diff --git a/src/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts b/src/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts deleted file mode 100644 index f36b21f56..000000000 --- a/src/Models/ThemeConfig/Conversion/CreateNoteImportLayer.ts +++ /dev/null @@ -1,209 +0,0 @@ -import { Conversion } from "./Conversion" -import LayerConfig from "../LayerConfig" -import { LayerConfigJson } from "../Json/LayerConfigJson" -import Translations from "../../../UI/i18n/Translations" -import { Translation, TypedTranslation } from "../../../UI/i18n/Translation" -import { ConversionContext } from "./ConversionContext" - -export default class CreateNoteImportLayer extends Conversion { - /** - * A closed note is included if it is less then 'n'-days closed - * @private - */ - private readonly _includeClosedNotesDays: number - - constructor(includeClosedNotesDays = 0) { - super( - [ - "Advanced conversion which deducts a layer showing all notes that are 'importable' (i.e. a note that contains a link to some MapComplete theme, with hash '#import').", - "The import buttons and matches will be based on the presets of the given theme", - ].join("\n\n"), - [], - "CreateNoteImportLayer" - ) - this._includeClosedNotesDays = includeClosedNotesDays - } - - convert(layerJson: LayerConfigJson, _: ConversionContext): LayerConfigJson { - const t = Translations.t.importLayer - - /** - * The note itself will contain `tags=k=v;k=v;k=v;... - * This must be matched with a regex. - * This is a simple JSON-object as how it'll be put into the layerConfigJson directly - */ - const isShownIfAny: any[] = [] - const layer = new LayerConfig(layerJson, "while constructing a note-import layer") - for (const preset of layer.presets) { - const mustMatchAll = [] - for (const tag of preset.tags) { - const key = tag.key - const value = tag.value - const condition = "_tags~(^|.*;)" + key + "=" + value + "($|;.*)" - mustMatchAll.push(condition) - } - isShownIfAny.push({ and: mustMatchAll }) - } - - const title = layer.presets[0].title - - const importButton = {} - { - const translations = trs(t.importButton, { - layerId: layer.id, - title: layer.presets[0].title, - }) - for (const key in translations) { - if (key !== "_context") { - importButton[key] = "{" + translations[key] + "}" - } else { - importButton[key] = translations[key] - } - } - } - - function embed(prefix, translation: Translation, postfix) { - const result = {} - for (const language in translation.translations) { - result[language] = prefix + translation.translations[language] + postfix - } - result["_context"] = translation.context - return result - } - - function tr(translation: Translation) { - return { ...translation.translations, _context: translation.context } - } - - function trs(translation: TypedTranslation, subs: T): Record { - return { ...translation.Subs(subs).translations, _context: translation.context } - } - - return { - id: "note_import_" + layer.id, - // By disabling the name, the import-layers won't pollute the filter view "name": t.layerName.Subs({title: layer.title.render}).translations, - description: trs(t.description, { title: layer.title.render }), - source: { - osmTags: { - and: ["id~[0-9]+", "comment_url~.*notes/[0-9]*/comment.json"], - }, - geoJson: - "https://api.openstreetmap.org/api/0.6/notes.json?limit=10000&closed=" + - this._includeClosedNotesDays + - "&bbox={x_min},{y_min},{x_max},{y_max}", - geoJsonZoomLevel: 10, - }, - /* We need to set 'pass_all_features' - There are probably many note_import-layers, and we don't want the first one to gobble up all notes and then discard them... - */ - passAllFeatures: true, - minzoom: Math.min(12, layerJson.minzoom - 2), - title: { - render: trs(t.popupTitle, { title }), - }, - calculatedTags: [ - "_first_comment=get(feat)('comments')[0]?.text?.toLowerCase() ?? ''", - "_trigger_index=(() => {const lines = feat.properties['_first_comment']?.split('\\n') ?? []; const matchesMapCompleteURL = lines.map(l => l.match(\".*https://mapcomplete.\\(org|osm.be\\)/\\([a-zA-Z_-]+\\)\\(.html\\)?.*#import\")); const matchedIndexes = matchesMapCompleteURL.map((doesMatch, i) => [doesMatch !== null, i]).filter(v => v[0]).map(v => v[1]); return matchedIndexes[0] })()", - "_comments_count=get(feat)('comments').length", - "_intro=(() => {const lines = get(feat)('comments')[0].text.split('\\n'); lines.splice(get(feat)('_trigger_index')-1, lines.length); return lines.filter(l => l !== '').join('
');})()", - "_tags=(() => {let lines = get(feat)('comments')[0].text.split('\\n').map(l => l.trim()); lines.splice(0, get(feat)('_trigger_index') + 1); lines = lines.filter(l => l != ''); return lines.join(';');})()", - ], - isShown: { - and: ["_trigger_index~*", { or: isShownIfAny }], - }, - titleIcons: [ - { - render: "", - }, - ], - tagRenderings: [ - { - id: "Intro", - render: "{_intro}", - }, - { - id: "conversation", - render: "{visualize_note_comments(comments,1)}", - condition: "_comments_count>1", - }, - { - id: "import", - render: importButton, - condition: "closed_at=", - }, - { - id: "close_note_", - render: embed( - "{close_note(", - t.notFound.Subs({ title }), - ", ./assets/svg/close.svg, id, This feature does not exist, 18)}" - ), - condition: "closed_at=", - }, - { - id: "close_note_mapped", - render: embed( - "{close_note(", - t.alreadyMapped.Subs({ title }), - ", ./assets/svg/duplicate.svg, id, Already mapped, 18)}" - ), - condition: "closed_at=", - }, - { - id: "handled", - render: tr(t.importHandled), - condition: "closed_at~*", - }, - { - id: "comment", - render: "{add_note_comment()}", - }, - { - id: "add_image", - render: "{add_image_to_note()}", - }, - { - id: "nearby_images_note", - render: tr(t.nearbyImagesIntro), - }, - { - id: "all_tags_note", - render: "{all_tags()}", - metacondition: { - or: [ - "__featureSwitchIsDebugging=true", - "mapcomplete-show_tags=full", - "mapcomplete-show_debug=yes", - ], - }, - }, - ], - pointRendering: [ - { - location: ["point"], - marker: [ - { - icon: "circle", - color: "#fff", - }, - { - icon: { - render: "help", - mappings: [ - { - if: { or: ["closed_at~*", "_imported=yes"] }, - then: "checkmark", - }, - ], - }, - color: "#00", - }, - ], - iconSize: "40,40", - anchor: "center", - }, - ], - allowMove: false, - } - } -} diff --git a/src/Models/ThemeConfig/Conversion/PrepareTheme.ts b/src/Models/ThemeConfig/Conversion/PrepareTheme.ts index ea380de88..825f8c3db 100644 --- a/src/Models/ThemeConfig/Conversion/PrepareTheme.ts +++ b/src/Models/ThemeConfig/Conversion/PrepareTheme.ts @@ -1,27 +1,15 @@ -import { - Concat, - Conversion, - DesugaringContext, - DesugaringStep, - Each, - Fuse, - On, - Pass, - 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" import { Utils } from "../../../Utils" import Constants from "../../Constants" -import CreateNoteImportLayer from "./CreateNoteImportLayer" import LayerConfig from "../LayerConfig" import { TagRenderingConfigJson } from "../Json/TagRenderingConfigJson" import DependencyCalculator from "../DependencyCalculator" import { AddContextToTranslations } from "./AddContextToTranslations" import ValidationUtils from "./ValidationUtils" import { ConversionContext } from "./ConversionContext" -import { PrevalidateTheme } from "./Validation" class SubstituteLayer extends Conversion { private readonly _state: DesugaringContext @@ -40,7 +28,7 @@ class SubstituteLayer extends Conversion [ + const withDistance:[string,number][] = knownLayers.map((lname) => [ lname, Utils.levenshteinDistance(name, lname), ]) @@ -221,68 +209,6 @@ 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", - ) - } - - convert(json: LayoutConfigJson, context: ConversionContext): LayoutConfigJson { - if (!(json.enableNoteImports ?? true)) { - context.info( - "Not creating a note import layers for theme " + json.id + " as they are disabled", - ) - return json - } - - json = { ...json } - const allLayers: LayerConfigJson[] = json.layers - json.layers = [...json.layers] - - const creator = new CreateNoteImportLayer() - for (let i1 = 0; i1 < allLayers.length; i1++) { - const layer = allLayers[i1] - if (layer.source === undefined) { - // Priviliged layers are skipped - continue - } - - if (layer.source["geoJson"] !== undefined) { - // Layer which don't get their data from OSM are skipped - continue - } - - if (layer.title === undefined || layer.name === undefined) { - // Anonymous layers and layers without popup are skipped - continue - } - - if (layer.presets === undefined || layer.presets.length == 0) { - // A preset is needed to be able to generate a new point - continue - } - - try { - const importLayerResult = creator.convert( - layer, - context.inOperation(this.name).enter(i1), - ) - if (importLayerResult !== undefined) { - json.layers.push(importLayerResult) - } - } catch (e) { - console.error("Error", e) - context.err("Could not generate an import-layer for " + layer.id + " due to " + e) - } - } - - return json - } -} - class AddContextToTranslationsInLayout extends DesugaringStep { constructor() { super( @@ -589,7 +515,6 @@ class PostvalidateTheme extends DesugaringStep { for (const l of json.layers) { const layer = l const basedOn = layer["_basedOn"] - const basedOnDef = this._state.sharedLayers.get(basedOn) if (!basedOn) { continue } diff --git a/src/Models/ThemeConfig/LayoutConfig.ts b/src/Models/ThemeConfig/LayoutConfig.ts index e9239b200..2680a62af 100644 --- a/src/Models/ThemeConfig/LayoutConfig.ts +++ b/src/Models/ThemeConfig/LayoutConfig.ts @@ -23,6 +23,7 @@ export class MinimalLayoutInformation { keywords?: Record layers: string[] } + /** * Minimal information about a theme **/ @@ -38,7 +39,6 @@ export class LayoutInformation { } - export default class LayoutConfig implements LayoutInformation { public static readonly defaultSocialImage = "assets/SocialImage.png" public readonly id: string @@ -195,7 +195,7 @@ export default class LayoutConfig implements LayoutInformation { icon: "./assets/svg/pop-out.svg", href: "https://{basepath}/{theme}.html?lat={lat}&lon={lon}&z={zoom}&language={language}", newTab: true, - requirements: ["iframe", "no-welcome-message"], + requirements: ["iframe", "no-welcome-message"] }, context + ".extraLink" ) @@ -296,12 +296,7 @@ export default class LayoutConfig implements LayoutInformation { } untranslated .get(ln) - .push( - translation.context.replace( - /^note_import_[a-zA-Z0-9_]*/, - "note_import" - ) - ) + .push(translation.context) } }) }, @@ -315,6 +310,7 @@ export default class LayoutConfig implements LayoutInformation { return { untranslated, total } } + public getMatchingLayer(tags: Record): LayerConfig | undefined { if (tags === undefined) { return undefined @@ -348,7 +344,7 @@ export default class LayoutConfig implements LayoutInformation { // The 'favourite'-layer contains pretty much all images as it bundles all layers, so we exclude it const jsonNoFavourites = { ...json, - layers: json.layers.filter((l) => l["id"] !== "favourite"), + layers: json.layers.filter((l) => l["id"] !== "favourite") } const usedImages = jsonNoFavourites._usedImages usedImages.sort() diff --git a/src/Models/ThemeViewState.ts b/src/Models/ThemeViewState.ts index 596060f12..1b947ff0a 100644 --- a/src/Models/ThemeViewState.ts +++ b/src/Models/ThemeViewState.ts @@ -737,11 +737,7 @@ export default class ThemeViewState implements SpecialVisualizationState { /** * MaxZoom for the summary layer */ - const normalLayers = this.layout.layers.filter( - (l) => - Constants.priviliged_layers.indexOf(l.id) < 0 && - !l.id.startsWith("note_import"), - ) + const normalLayers = this.layout.layers.filter(l => l.isNormal()) const maxzoom = Math.min(...normalLayers.map((l) => l.minzoom)) diff --git a/src/UI/BigComponents/ShareScreen.svelte b/src/UI/BigComponents/ShareScreen.svelte index 49e90ba7f..8c3160e66 100644 --- a/src/UI/BigComponents/ShareScreen.svelte +++ b/src/UI/BigComponents/ShareScreen.svelte @@ -60,9 +60,6 @@ if (flayer.layerDef.filterIsSameAs) { continue } - if (id.indexOf("note_import") >= 0) { - continue - } if (Constants.added_by_default.indexOf(id) >= 0) { continue } diff --git a/test/Models/ThemeConfig/Conversion/CreateNoteImportLayer.spec.ts b/test/Models/ThemeConfig/Conversion/CreateNoteImportLayer.spec.ts deleted file mode 100644 index dd4adaded..000000000 --- a/test/Models/ThemeConfig/Conversion/CreateNoteImportLayer.spec.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { Utils } from "../../../../src/Utils" -import { DesugaringContext } from "../../../../src/Models/ThemeConfig/Conversion/Conversion" -import { LayerConfigJson } from "../../../../src/Models/ThemeConfig/Json/LayerConfigJson" -import { TagRenderingConfigJson } from "../../../../src/Models/ThemeConfig/Json/TagRenderingConfigJson" -import { PrepareLayer } from "../../../../src/Models/ThemeConfig/Conversion/PrepareLayer" -import * as bookcases from "../../../../assets/layers/public_bookcase/public_bookcase.json" -import CreateNoteImportLayer from "../../../../src/Models/ThemeConfig/Conversion/CreateNoteImportLayer" -import { describe, expect, it } from "vitest" -import { QuestionableTagRenderingConfigJson } from "../../../../src/Models/ThemeConfig/Json/QuestionableTagRenderingConfigJson" -import { ConversionContext } from "../../../../src/Models/ThemeConfig/Conversion/ConversionContext" - -describe("CreateNoteImportLayer", () => { - it("should generate a layerconfig", () => { - const desugaringState: DesugaringContext = { - sharedLayers: new Map(), - tagRenderings: new Map(), - } - const layerPrepare = new PrepareLayer(desugaringState) - const layer = layerPrepare.convertStrict( - bookcases, - ConversionContext.test("parse bookcases") - ) - const generator = new CreateNoteImportLayer() - const generatedLayer: LayerConfigJson = generator.convertStrict( - layer, - ConversionContext.test("convert") - ) - expect(generatedLayer.isShown["and"][1].or[0].and[0]).toEqual( - "_tags~(^|.*;)amenity=public_bookcase($|;.*)" - ) - // "Zoomlevel is to high" - expect(generatedLayer.minzoom <= layer.minzoom).toBe(true) - let renderings = Utils.NoNull( - Utils.NoNull( - generatedLayer.tagRenderings.map((tr) => (tr).render) - ).map((render) => render["en"]) - ) - // "no import button found" - expect(renderings.some((r) => r.indexOf("import_button") > 0)).toBe(true) - }) -})