From 7090a5ceb804d417c9ad1a190ba1841fd685fdda Mon Sep 17 00:00:00 2001 From: pietervdvn Date: Thu, 6 Jan 2022 18:51:52 +0100 Subject: [PATCH] Performance hacks --- Logic/Actors/SelectedFeatureHandler.ts | 14 --- .../TiledFeatureSource/TiledFeatureSource.ts | 5 +- Logic/MetaTagging.ts | 40 ++++--- Logic/SimpleMetaTagger.ts | 9 +- UI/Base/Combine.ts | 8 +- UI/Base/MinimapImplementation.ts | 18 +++- UI/Base/ScrollableFullScreen.ts | 29 +++-- UI/Base/VariableUIElement.ts | 9 ++ UI/BaseUIElement.ts | 5 + UI/BigComponents/LeftControls.ts | 2 +- UI/Popup/FeatureInfoBox.ts | 1 + UI/ShowDataLayer/ShowDataLayer.ts | 102 +++++++++++------- UI/i18n/Translation.ts | 11 +- assets/layers/etymology/etymology.json | 5 +- assets/themes/grb_import/grb.json | 2 +- 15 files changed, 167 insertions(+), 93 deletions(-) diff --git a/Logic/Actors/SelectedFeatureHandler.ts b/Logic/Actors/SelectedFeatureHandler.ts index 204950ed9..c5598f346 100644 --- a/Logic/Actors/SelectedFeatureHandler.ts +++ b/Logic/Actors/SelectedFeatureHandler.ts @@ -39,20 +39,6 @@ export default class SelectedFeatureHandler { hash.addCallback(() => self.setSelectedElementFromHash()) - // IF the selected element changes, set the hash correctly - state.selectedElement.addCallback(feature => { - if (feature === undefined) { - if (!SelectedFeatureHandler._no_trigger_on.has(hash.data)) { - hash.setData("") - } - } - - const h = feature?.properties?.id; - if (h !== undefined) { - hash.setData(h) - } - }) - state.featurePipeline?.newDataLoadedSignal?.addCallbackAndRunD(_ => { // New data was loaded. In initial startup, the hash might be set (via the URL) but might not be selected yet if (hash.data === undefined || SelectedFeatureHandler._no_trigger_on.has(hash.data)) { diff --git a/Logic/FeatureSource/TiledFeatureSource/TiledFeatureSource.ts b/Logic/FeatureSource/TiledFeatureSource/TiledFeatureSource.ts index 60918fbfa..98e904746 100644 --- a/Logic/FeatureSource/TiledFeatureSource/TiledFeatureSource.ts +++ b/Logic/FeatureSource/TiledFeatureSource/TiledFeatureSource.ts @@ -201,9 +201,10 @@ export interface TiledFeatureSourceOptions { readonly minZoomLevel?: number, /** * IF minZoomLevel is set, and if a feature runs through a tile boundary, it would normally be duplicated. - * Setting 'dontEnforceMinZoomLevel' will still allow bigger zoom levels for those features + * Setting 'dontEnforceMinZoomLevel' will still allow bigger zoom levels for those features. + * If 'pick_first' is set, the feature will not be duplicated but set to some tile */ - readonly dontEnforceMinZoom?: boolean, + readonly dontEnforceMinZoom?: boolean | "pick_first", readonly registerTile?: (tile: TiledFeatureSource & FeatureSourceForLayer & Tiled) => void, readonly layer?: FilteredLayer } \ No newline at end of file diff --git a/Logic/MetaTagging.ts b/Logic/MetaTagging.ts index d1501109c..302762110 100644 --- a/Logic/MetaTagging.ts +++ b/Logic/MetaTagging.ts @@ -56,6 +56,7 @@ export default class MetaTagging { const feature = ff.feature const freshness = ff.freshness let somethingChanged = false + let definedTags = new Set(Object.getOwnPropertyNames( feature.properties )) for (const metatag of metatagsToApply) { try { if (!metatag.keys.some(key => feature.properties[key] === undefined)) { @@ -64,8 +65,11 @@ export default class MetaTagging { } if (metatag.isLazy) { + if(!metatag.keys.some(key => !definedTags.has(key))) { + // All keys are defined - lets skip! + continue + } somethingChanged = true; - metatag.applyMetaTagsOnFeature(feature, freshness, layer, state) } else { const newValueAdded = metatag.applyMetaTagsOnFeature(feature, freshness, layer, state) @@ -84,12 +88,13 @@ export default class MetaTagging { } if (layerFuncs !== undefined) { + let retaggingChanged = false; try { - layerFuncs(params, feature) + retaggingChanged = layerFuncs(params, feature) } catch (e) { console.error(e) } - somethingChanged = true + somethingChanged = somethingChanged || retaggingChanged } if (somethingChanged) { @@ -99,8 +104,8 @@ export default class MetaTagging { } return atLeastOneFeatureChanged } - public static createFunctionsForFeature(layerId: string, calculatedTags: [string, string, boolean][]): ((feature: any) => void)[] { - const functions: ((feature: any) => void)[] = []; + public static createFunctionsForFeature(layerId: string, calculatedTags: [string, string, boolean][]): ((feature: any) => boolean)[] { + const functions: ((feature: any) => boolean)[] = []; for (const entry of calculatedTags) { const key = entry[0] @@ -110,10 +115,9 @@ export default class MetaTagging { continue; } - const calculateAndAssign = (feat) => { - - + const calculateAndAssign: ((feat: any) => boolean) = (feat) => { try { + let oldValue = isStrict ? feat.properties[key] : undefined let result = new Function("feat", "return " + code + ";")(feat); if (result === "") { result === undefined @@ -124,7 +128,7 @@ export default class MetaTagging { } delete feat.properties[key] feat.properties[key] = result; - return result; + return result === oldValue; }catch(e){ if (MetaTagging.errorPrintCount < MetaTagging.stopErrorOutputAt) { console.warn("Could not calculate a " + (isStrict ? "strict " : "") + " calculated tag for key " + key + " defined by " + code + " (in layer" + layerId + ") due to \n" + e + "\n. Are you the theme creator? Doublecheck your code. Note that the metatags might not be stable on new features", e, e.stack) @@ -133,6 +137,7 @@ export default class MetaTagging { console.error("Got ", MetaTagging.stopErrorOutputAt, " errors calculating this metatagging - stopping output now") } } + return false; } } @@ -149,9 +154,11 @@ export default class MetaTagging { configurable: true, enumerable: false, // By setting this as not enumerable, the localTileSaver will _not_ calculate this get: function () { - return calculateAndAssign(feature) + calculateAndAssign(feature) + return feature.properties[key] } }) + return true } @@ -160,17 +167,23 @@ export default class MetaTagging { return functions; } - private static retaggingFuncCache = new Map void)[]>() + private static retaggingFuncCache = new Map boolean)[]>() + /** + * Creates the function which adds all the calculated tags to a feature. Called once per layer + * @param layer + * @param state + * @private + */ private static createRetaggingFunc(layer: LayerConfig, state): - ((params: ExtraFuncParams, feature: any) => void) { + ((params: ExtraFuncParams, feature: any) => boolean) { const calculatedTags: [string, string, boolean][] = layer.calculatedTags; if (calculatedTags === undefined || calculatedTags.length === 0) { return undefined; } - let functions = MetaTagging.retaggingFuncCache.get(layer.id); + let functions :((feature: any) => boolean)[] = MetaTagging.retaggingFuncCache.get(layer.id); if (functions === undefined) { functions = MetaTagging.createFunctionsForFeature(layer.id, calculatedTags) MetaTagging.retaggingFuncCache.set(layer.id, functions) @@ -192,6 +205,7 @@ export default class MetaTagging { } catch (e) { console.error("Invalid syntax in calculated tags or some other error: ", e) } + return true; // Something changed } } diff --git a/Logic/SimpleMetaTagger.ts b/Logic/SimpleMetaTagger.ts index db422827f..67be6a5af 100644 --- a/Logic/SimpleMetaTagger.ts +++ b/Logic/SimpleMetaTagger.ts @@ -272,7 +272,7 @@ export default class SimpleMetaTaggers { public static country = new CountryTagger() private static isOpen = new SimpleMetaTagger( { - keys: ["_isOpen", "_isOpen:description"], + keys: ["_isOpen"], doc: "If 'opening_hours' is present, it will add the current state of the feature (being 'yes' or 'no')", includesDates: true, isLazy: true @@ -283,7 +283,7 @@ export default class SimpleMetaTaggers { // isOpen is irrelevant return false } - + Object.defineProperty(feature.properties, "_isOpen", { enumerable: false, configurable: true, @@ -291,7 +291,8 @@ export default class SimpleMetaTaggers { delete feature.properties._isOpen feature.properties._isOpen = undefined const tagsSource = state.allElements.getEventSourceById(feature.properties.id); - tagsSource.addCallbackAndRunD(tags => { + tagsSource + .addCallbackAndRunD(tags => { if (tags.opening_hours === undefined || tags._country === undefined) { return; } @@ -341,7 +342,6 @@ export default class SimpleMetaTaggers { } } updateTags(); - return true; // Our job is done, lets unregister! } catch (e) { console.warn("Error while parsing opening hours of ", tags.id, e); delete tags._isOpen @@ -352,6 +352,7 @@ export default class SimpleMetaTaggers { return undefined } }) + return true; }) ) diff --git a/UI/Base/Combine.ts b/UI/Base/Combine.ts index 434644b6a..3c3cb0c7f 100644 --- a/UI/Base/Combine.ts +++ b/UI/Base/Combine.ts @@ -1,7 +1,6 @@ import {FixedUiElement} from "./FixedUiElement"; import {Utils} from "../../Utils"; import BaseUIElement from "../BaseUIElement"; -import Title from "./Title"; export default class Combine extends BaseUIElement { private readonly uiElements: BaseUIElement[]; @@ -21,6 +20,13 @@ export default class Combine extends BaseUIElement { return this.uiElements.map(el => el.AsMarkdown()).join(this.HasClass("flex-col") ? "\n\n" : " "); } + Destroy() { + super.Destroy(); + for (const uiElement of this.uiElements) { + uiElement.Destroy() + } + } + protected InnerConstructElement(): HTMLElement { const el = document.createElement("span") try { diff --git a/UI/Base/MinimapImplementation.ts b/UI/Base/MinimapImplementation.ts index 4d93c4fae..e5aa7f8ef 100644 --- a/UI/Base/MinimapImplementation.ts +++ b/UI/Base/MinimapImplementation.ts @@ -48,7 +48,7 @@ export default class MinimapImplementation extends BaseUIElement implements Mini public static initialize() { Minimap.createMiniMap = options => new MinimapImplementation(options) } - + public installBounds(factor: number | BBox, showRange?: boolean) { this.leafletMap.addCallbackD(leaflet => { let bounds; @@ -105,6 +105,15 @@ export default class MinimapImplementation extends BaseUIElement implements Mini } }) } + + Destroy() { + super.Destroy(); + console.warn("Decomissioning minimap", this._id) + const mp = this.leafletMap.data + this.leafletMap.setData(null) + mp.off() + mp.remove() + } public async TakeScreenshot() { const screenshotter = new SimpleMapScreenshoter(); @@ -125,6 +134,13 @@ export default class MinimapImplementation extends BaseUIElement implements Mini const self = this; // @ts-ignore const resizeObserver = new ResizeObserver(_ => { + if(wrapper.clientHeight === 0 || wrapper.clientWidth === 0){ + return; + } + if(wrapper.offsetParent === null || window.getComputedStyle(wrapper).display === 'none'){ + // Not visible + return; + } try { self.InitMap(); self.leafletMap?.data?.invalidateSize() diff --git a/UI/Base/ScrollableFullScreen.ts b/UI/Base/ScrollableFullScreen.ts index 686649ed8..c3dd39caa 100644 --- a/UI/Base/ScrollableFullScreen.ts +++ b/UI/Base/ScrollableFullScreen.ts @@ -49,24 +49,26 @@ export default class ScrollableFullScreen extends UIElement { Hash.hash.setData(hashToShow) self.Activate(); } else { - self.clear(); + // Some cleanup... + ScrollableFullScreen.empty.AttachTo("fullscreen") + const fs = document.getElementById("fullscreen"); + ScrollableFullScreen._currentlyOpen?.isShown?.setData(false); + fs.classList.add("hidden") } }) - Hash.hash.addCallback(hash => { - if (!isShown.data) { - return; - } - if (hash === undefined || hash === "" || hash !== hashToShow) { - isShown.setData(false) - } - }) } InnerRender(): BaseUIElement { return this._component; } + Destroy() { + super.Destroy(); + this._component.Destroy() + this._fullscreencomponent.Destroy() + } + Activate(): void { this.isShown.setData(true) this._fullscreencomponent.AttachTo("fullscreen"); @@ -74,14 +76,6 @@ export default class ScrollableFullScreen extends UIElement { ScrollableFullScreen._currentlyOpen = this; fs.classList.remove("hidden") } - - private clear() { - ScrollableFullScreen.empty.AttachTo("fullscreen") - const fs = document.getElementById("fullscreen"); - ScrollableFullScreen._currentlyOpen?.isShown?.setData(false); - fs.classList.add("hidden") - } - private BuildComponent(title: BaseUIElement, content: BaseUIElement, isShown: UIEventSource) { const returnToTheMap = new Combine([ @@ -93,6 +87,7 @@ export default class ScrollableFullScreen extends UIElement { returnToTheMap.onClick(() => { isShown.setData(false) + Hash.hash.setData(undefined) }) title.SetClass("block text-l sm:text-xl md:text-2xl w-full font-bold p-0 max-h-20vh overflow-y-auto") diff --git a/UI/Base/VariableUIElement.ts b/UI/Base/VariableUIElement.ts index 6ee720627..6798a11f2 100644 --- a/UI/Base/VariableUIElement.ts +++ b/UI/Base/VariableUIElement.ts @@ -8,10 +8,19 @@ export class VariableUiElement extends BaseUIElement { super(); this._contents = contents; } + + Destroy() { + super.Destroy(); + this.isDestroyed = true; + } protected InnerConstructElement(): HTMLElement { const el = document.createElement("span"); + const self = this; this._contents.addCallbackAndRun((contents) => { + if(self.isDestroyed){ + return true; + } while (el.firstChild) { el.removeChild(el.lastChild); } diff --git a/UI/BaseUIElement.ts b/UI/BaseUIElement.ts index eaac95b4b..eb33637a8 100644 --- a/UI/BaseUIElement.ts +++ b/UI/BaseUIElement.ts @@ -11,6 +11,7 @@ export default abstract class BaseUIElement { private clss: Set = new Set(); private style: string; private _onClick: () => void; + protected isDestroyed = false; public onClick(f: (() => void)) { this._onClick = f; @@ -149,6 +150,10 @@ export default abstract class BaseUIElement { public AsMarkdown(): string { throw "AsMarkdown is not implemented by " + this.constructor.name+"; implement it in the subclass" } + + public Destroy(){ + this.isDestroyed = true; + } protected abstract InnerConstructElement(): HTMLElement; } diff --git a/UI/BigComponents/LeftControls.ts b/UI/BigComponents/LeftControls.ts index a669b856c..a996416f5 100644 --- a/UI/BigComponents/LeftControls.ts +++ b/UI/BigComponents/LeftControls.ts @@ -121,7 +121,7 @@ export default class LeftControls extends Combine { "filters", guiState.filterViewIsOpened ).SetClass("rounded-lg md:floating-element-width"), - new MapControlButton(Svg.filter_svg()) + new MapControlButton(Svg.layers_svg()) .onClick(() => guiState.filterViewIsOpened.setData(true)), guiState.filterViewIsOpened ) diff --git a/UI/Popup/FeatureInfoBox.ts b/UI/Popup/FeatureInfoBox.ts index 9d637f7da..5d1d5baf1 100644 --- a/UI/Popup/FeatureInfoBox.ts +++ b/UI/Popup/FeatureInfoBox.ts @@ -235,4 +235,5 @@ export default class FeatureInfoBox extends ScrollableFullScreen { return false; } + } diff --git a/UI/ShowDataLayer/ShowDataLayer.ts b/UI/ShowDataLayer/ShowDataLayer.ts index d1af72001..7e372e7a7 100644 --- a/UI/ShowDataLayer/ShowDataLayer.ts +++ b/UI/ShowDataLayer/ShowDataLayer.ts @@ -29,6 +29,11 @@ export default class ShowDataLayer { // Used to generate a fresh ID when needed private _cleanCount = 0; private geoLayer = undefined; + + /** + * A collection of functions to call when the current geolayer is unregistered + */ + private unregister: (() => void)[] = []; private isDirty = false; /** * If the selected element triggers, this is used to lookup the correct layer and to open the popup @@ -56,25 +61,32 @@ export default class ShowDataLayer { const self = this; options.leafletMap.addCallback(_ => { - self.update(options) + return self.update(options) } ); this._features.features.addCallback(_ => self.update(options)); options.doShowLayer?.addCallback(doShow => { const mp = options.leafletMap.data; + if(mp === null){ + self.Destroy() + return true; + } if (mp == undefined) { return; } + if (doShow) { if (self.isDirty) { - self.update(options) + return self.update(options) } else { mp.addLayer(this.geoLayer) } } else { if (this.geoLayer !== undefined) { mp.removeLayer(this.geoLayer) + this.unregister.forEach(f => f()) + this.unregister = [] } } @@ -82,40 +94,50 @@ export default class ShowDataLayer { this._selectedElement?.addCallbackAndRunD(selected => { - if (self._leafletMap.data === undefined) { - return; - } - const v = self.leafletLayersPerId.get(selected.properties.id + selected.geometry.type) - if (v === undefined) { - return; - } - const leafletLayer = v.leafletlayer - const feature = v.feature - if (leafletLayer.getPopup().isOpen()) { - return; - } - if (selected.properties.id !== feature.properties.id) { - return; - } - - if (feature.id !== feature.properties.id) { - // Probably a feature which has renamed - // the feature might have as id 'node/-1' and as 'feature.properties.id' = 'the newly assigned id'. That is no good too - console.log("Not opening the popup for", feature, "as probably renamed") - return; - } - if (selected.geometry.type === feature.geometry.type // If a feature is rendered both as way and as point, opening one popup might trigger the other to open, which might trigger the one to open again - ) { - console.log("Opening popup of feature", feature) - leafletLayer.openPopup() - } + self.openPopupOfSelectedElement(selected) }) this.update(options) } - private update(options: ShowDataLayerOptions) { + private Destroy() { + this.unregister.forEach(f => f()) + } + + private openPopupOfSelectedElement(selected) { + if (selected === undefined) { + return + } + if (this._leafletMap.data === undefined) { + return; + } + const v = this.leafletLayersPerId.get(selected.properties.id + selected.geometry.type) + if (v === undefined) { + return; + } + const leafletLayer = v.leafletlayer + const feature = v.feature + if (leafletLayer.getPopup().isOpen()) { + return; + } + if (selected.properties.id !== feature.properties.id) { + return; + } + + if (feature.id !== feature.properties.id) { + // Probably a feature which has renamed + // the feature might have as id 'node/-1' and as 'feature.properties.id' = 'the newly assigned id'. That is no good too + console.log("Not opening the popup for", feature, "as probably renamed") + return; + } + if (selected.geometry.type === feature.geometry.type // If a feature is rendered both as way and as point, opening one popup might trigger the other to open, which might trigger the one to open again + ) { + leafletLayer.openPopup() + } + } + + private update(options: ShowDataLayerOptions) : boolean{ if (this._features.features.data === undefined) { return; } @@ -125,9 +147,13 @@ export default class ShowDataLayer { } const mp = options.leafletMap.data; + if(mp === null){ + return true; // Unregister as the map is destroyed + } if (mp === undefined) { return; } + console.trace("Updating... " + mp["_container"]?.id +" for layer "+this._layerToShow.id) this._cleanCount++ // clean all the old stuff away, if any if (this.geoLayer !== undefined) { @@ -145,7 +171,7 @@ export default class ShowDataLayer { pointToLayer: (feature, latLng) => self.pointToLayer(feature, latLng), onEachFeature: (feature, leafletLayer) => self.postProcessFeature(feature, leafletLayer) }); - + const selfLayer = this.geoLayer; const allFeats = this._features.features.data; for (const feat of allFeats) { @@ -176,20 +202,20 @@ export default class ShowDataLayer { offsettedLine = L.polyline(coords, lineStyle); this.postProcessFeature(feat, offsettedLine) offsettedLine.addTo(this.geoLayer) - + // If 'self.geoLayer' is not the same as the layer the feature is added to, we can safely remove this callback return self.geoLayer !== selfLayer }) } else { this.geoLayer.addData(feat); - } + } } catch (e) { console.error("Could not add ", feat, "to the geojson layer in leaflet due to", e, e.stack) } } if (options.zoomToFeatures ?? false) { - if(this.geoLayer.getLayers().length > 0){ + if (this.geoLayer.getLayers().length > 0) { try { const bounds = this.geoLayer.getBounds() mp.fitBounds(bounds, {animate: false}) @@ -203,6 +229,7 @@ export default class ShowDataLayer { mp.addLayer(this.geoLayer) } this.isDirty = false; + this.openPopupOfSelectedElement(this._selectedElement?.data) } @@ -250,7 +277,7 @@ export default class ShowDataLayer { } /** - * POst processing - basically adding the popup + * Post processing - basically adding the popup * @param feature * @param leafletLayer * @private @@ -290,6 +317,10 @@ export default class ShowDataLayer { } infobox.AttachTo(id) infobox.Activate(); + this.unregister.push(() => { + console.log("Destroying infobox") + infobox.Destroy(); + }) if (this._selectedElement?.data?.properties?.id !== feature.properties.id) { this._selectedElement?.setData(feature) } @@ -303,7 +334,6 @@ export default class ShowDataLayer { leafletlayer: leafletLayer }) - } } \ No newline at end of file diff --git a/UI/i18n/Translation.ts b/UI/i18n/Translation.ts index c11bc4b0a..d914425a2 100644 --- a/UI/i18n/Translation.ts +++ b/UI/i18n/Translation.ts @@ -7,7 +7,7 @@ export class Translation extends BaseUIElement { public static forcedLanguage = undefined; public readonly translations: object - + constructor(translations: object, context?: string) { super() if (translations === undefined) { @@ -33,6 +33,11 @@ export class Translation extends BaseUIElement { get txt(): string { return this.textFor(Translation.forcedLanguage ?? Locale.language.data) } + + Destroy() { + super.Destroy(); + this.isDestroyed = true; + } static ExtractAllTranslationsFrom(object: any, context = ""): { context: string, tr: Translation }[] { const allTranslations: { context: string, tr: Translation }[] = [] @@ -90,7 +95,11 @@ export class Translation extends BaseUIElement { InnerConstructElement(): HTMLElement { const el = document.createElement("span") + const self = this Locale.language.addCallbackAndRun(_ => { + if(self.isDestroyed){ + return true + } el.innerHTML = this.txt }) return el; diff --git a/assets/layers/etymology/etymology.json b/assets/layers/etymology/etymology.json index b0087cf8d..1c0f96c41 100644 --- a/assets/layers/etymology/etymology.json +++ b/assets/layers/etymology/etymology.json @@ -140,6 +140,7 @@ }, { "id": "wikipedia", + "#": "Note that this is a _read_only_ option, to prevent people entering a 'wikidata'-link instead of 'name:etymology:wikidata'", "render": { "en": "A Wikipedia article about this street exists:
{wikipedia():max-height:25rem}" }, @@ -149,7 +150,7 @@ "mapRendering": [ { "icon": { - "render": "pin:#05d7fcaa;./assets/layers/etymology/logo.svg", + "render": "pin:#05d7fcaa", "mappings": [ { "if": { @@ -158,7 +159,7 @@ "name:etymology:wikidata=" ] }, - "then": "pin:#fcca05aa;./assets/layers/etymology/logo.svg" + "then": "pin:#fcca05aa" } ] }, diff --git a/assets/themes/grb_import/grb.json b/assets/themes/grb_import/grb.json index 2b9bbfac6..77bda9e99 100644 --- a/assets/themes/grb_import/grb.json +++ b/assets/themes/grb_import/grb.json @@ -16,7 +16,7 @@ "de" ], "maintainer": "", - "icon": "./assets/svg/bug.svg", + "icon": "./assets/themes/grb_import/grb.svg", "version": "0", "startLat": 51.0249, "startLon": 4.026489,