diff --git a/api/react-for-mappls.api.d.ts b/api/react-for-mappls.api.d.ts index 3fcd5f4..1867351 100644 --- a/api/react-for-mappls.api.d.ts +++ b/api/react-for-mappls.api.d.ts @@ -185,6 +185,17 @@ declare class MapplsMarkerOverlayRenderer extends AbstractMarkerOverlayRenderer< private ensureImages; private ensureImage; private loadBitmapIcon; + /** + * `map.loadImage` in the callback form the Mappls SDK has. + * + * The Mappls map is built on the mapbox-gl v1 line, where `loadImage(url, callback)` + * returns nothing and reports through the callback. Awaiting it as maplibre's promise + * form got `undefined` (so this fell back to a plain Image and fetched the icon twice), + * and the SDK then called the missing callback when its own fetch finished, which threw + * "e is not a function" as an unhandled rejection inside the SDK on every page with a + * marker. A promise, if a future SDK returns one, is honoured as well. + */ + private loadImageThroughMap; private ensureFallbackDefaultIcon; private removeUnusedImages; private syncIconOffsets; diff --git a/src/circle/MapplsCircleLayer.ts b/src/circle/MapplsCircleLayer.ts index 476800d..15e3433 100644 --- a/src/circle/MapplsCircleLayer.ts +++ b/src/circle/MapplsCircleLayer.ts @@ -3,7 +3,7 @@ import type { LayerSpecification, } from 'maplibre-gl'; import { type CircleEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type PolygonFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type PolygonFeature } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export type MapplsActualCircle = PolygonFeature & { id?: string | number }; @@ -51,7 +51,7 @@ export class MapplsCircleLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/src/helpers.ts b/src/helpers.ts index 15e60fe..48cd857 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -42,14 +42,84 @@ export function removeSourceIfExists(map: maplibregl.Map, sourceId: string): voi if (map.getSource(sourceId)) map.removeSource(sourceId); } +const MARKER_LAYER_IDS = ['mc-raster-lyr-mc-marker-tiles', 'mc-marker-layer', 'mc-marker-drag-layer']; + +// The layer order this module last produced, per map. Lets bringMarkerLayersToFront tell +// "nothing moved since I arranged it" apart from "someone added a layer on top". +const arrangedOrders = new WeakMap(); + +function currentLayerOrder(map: maplibregl.Map): string[] | null { + try { + const m = map as unknown as { + getLayersOrder?: () => string[]; + style?: { order?: string[] }; + getStyle?: () => { layers?: Array<{ id: string }> } | undefined; + }; + if (typeof m.getLayersOrder === 'function') return m.getLayersOrder(); + if (Array.isArray(m.style?.order)) return [...m.style!.order!]; + const layers = m.getStyle?.()?.layers; + return layers ? layers.map((layer) => layer.id) : null; + } catch { + return null; + } +} + +/** + * Keep the marker layers above every other layer. + * + * **Moves nothing when they already are.** `moveLayer` marks the style as changed even when + * the layer stays where it was, and every change fires `styledata`. The provider treats a + * `styledata` that arrives while sources are still loading as "style not ready", and the + * `idle` that follows resyncs every overlay — which called this again. The style flipped + * between ready and not ready about every 0.3 s, forever, with nothing on the map changing. + */ export function bringMarkerLayersToFront(map: maplibregl.Map): void { - for (const layerId of ['mc-raster-lyr-mc-marker-tiles', 'mc-marker-layer', 'mc-marker-drag-layer']) { + const order = currentLayerOrder(map); + if (order) { + const present = MARKER_LAYER_IDS.filter((layerId) => order.includes(layerId)); + if (present.length === 0) return; + const tail = order.slice(order.length - present.length); + if (tail.every((layerId, i) => layerId === present[i])) return; + if (arrangedOrders.get(map) === order.join('\n')) return; + } + for (const layerId of MARKER_LAYER_IDS) { try { if (map.getLayer(layerId)) map.moveLayer(layerId); } catch { continue; } } + const arranged = currentLayerOrder(map); + if (arranged) arrangedOrders.set(map, arranged.join('\n')); +} + +// What each GeoJSON source was last given, keyed by the source object itself. A style swap +// replaces the source objects, so a lost source never matches and is written again. +const writtenFeatures = new WeakMap(); + +/** + * `source.setData` that skips the write when the features are the same objects, in the same + * order, as the last write to this very source. + * + * Renderers build a new feature object whenever an overlay changes, so identity is enough to + * say "nothing changed". Skipping matters for the same reason as in + * [bringMarkerLayersToFront]: a redundant setData puts the source back into "loading", the + * next `styledata` then reads as "style not ready", and the resync on `idle` wrote it again. + */ +export function setSourceFeatures( + source: { setData(data: GeoJSONSourceData): unknown }, + features: FeatureCollection['features'], +): void { + const previous = writtenFeatures.get(source); + if ( + previous && + previous.length === features.length && + previous.every((feature, i) => feature === features[i]) + ) { + return; + } + source.setData({ type: 'FeatureCollection', features } as GeoJSONSourceData); + writtenFeatures.set(source, [...features]); } export function metersToPixels(meters: number, latitude: number): number { diff --git a/src/marker/MapplsMarkerOverlayRenderer.ts b/src/marker/MapplsMarkerOverlayRenderer.ts index aff247a..483dd9d 100644 --- a/src/marker/MapplsMarkerOverlayRenderer.ts +++ b/src/marker/MapplsMarkerOverlayRenderer.ts @@ -228,8 +228,7 @@ export class MapplsMarkerOverlayRenderer extends AbstractMarkerOverlayRenderer< url: string, ): Promise { try { - const image = await this.holder.map.loadImage(url); - return image.data; + return await this.loadImageThroughMap(url); } catch { const image = new Image(); image.decoding = 'async'; @@ -242,6 +241,36 @@ export class MapplsMarkerOverlayRenderer extends AbstractMarkerOverlayRenderer< } } + /** + * `map.loadImage` in the callback form the Mappls SDK has. + * + * The Mappls map is built on the mapbox-gl v1 line, where `loadImage(url, callback)` + * returns nothing and reports through the callback. Awaiting it as maplibre's promise + * form got `undefined` (so this fell back to a plain Image and fetched the icon twice), + * and the SDK then called the missing callback when its own fetch finished, which threw + * "e is not a function" as an unhandled rejection inside the SDK on every page with a + * marker. A promise, if a future SDK returns one, is honoured as well. + */ + private loadImageThroughMap(url: string): Promise { + type Loaded = HTMLImageElement | ImageBitmap | ImageData; + const map = this.holder.map as unknown as { + loadImage( + url: string, + callback: (error?: Error | null, image?: Loaded | null) => void, + ): void | Promise<{ data: Loaded }>; + }; + return new Promise((resolve, reject) => { + const returned = map.loadImage(url, (error, image) => { + if (error) reject(error); + else if (image) resolve(image); + else reject(new Error(`Failed to load marker icon: ${url}`)); + }); + if (returned && typeof (returned as Promise<{ data: Loaded }>).then === 'function') { + (returned as Promise<{ data: Loaded }>).then((response) => resolve(response.data), reject); + } + }); + } + private ensureFallbackDefaultIcon(): boolean { if (this.holder.map.hasImage(DEFAULT_ICON_ID)) return true; const size = 48; diff --git a/src/marker/MarkerLayer.ts b/src/marker/MarkerLayer.ts index 464bc02..530df96 100644 --- a/src/marker/MarkerLayer.ts +++ b/src/marker/MarkerLayer.ts @@ -4,7 +4,7 @@ import type { LayerSpecification, } from 'maplibre-gl'; import type { MarkerEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type PointFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type PointFeature } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export type MapplsActualMarker = PointFeature; @@ -93,7 +93,7 @@ export class MarkerLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/src/polygon/MapplsPolygonLayer.ts b/src/polygon/MapplsPolygonLayer.ts index 60337d5..31ef258 100644 --- a/src/polygon/MapplsPolygonLayer.ts +++ b/src/polygon/MapplsPolygonLayer.ts @@ -5,7 +5,7 @@ import type { LineFeature, PolygonFeature, } from '../helpers'; -import { bringMarkerLayersToFront } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export interface MapplsActualPolygon { @@ -69,8 +69,8 @@ export class MapplsPolygonLayer { const outlineSource = this.holder.map.getSource(this.outlineSourceId) as GeoJSONSource | undefined; if (!fillSource || !outlineSource) return false; - fillSource.setData(fillData); - outlineSource.setData(outlineData); + setSourceFeatures(fillSource, fillData.features); + setSourceFeatures(outlineSource, outlineData.features); return true; } catch { return false; diff --git a/src/polyline/MapplsPolylineLayer.ts b/src/polyline/MapplsPolylineLayer.ts index 3b2fe93..8be9363 100644 --- a/src/polyline/MapplsPolylineLayer.ts +++ b/src/polyline/MapplsPolylineLayer.ts @@ -1,6 +1,6 @@ import type { GeoJSONSource, LayerSpecification } from 'maplibre-gl'; import type { PolylineEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type LineFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type LineFeature } from '../helpers'; import { MapplsMapViewHolder } from '../MapplsMapViewHolder'; export type MapplsActualPolyline = LineFeature[]; @@ -45,7 +45,7 @@ export class MapplsPolylineLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/test/MarkerIconLoad.test.mjs b/test/MarkerIconLoad.test.mjs new file mode 100644 index 0000000..0556a09 --- /dev/null +++ b/test/MarkerIconLoad.test.mjs @@ -0,0 +1,55 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { after, test } from 'node:test'; +import { pathToFileURL } from 'node:url'; +import { resolve } from 'node:path'; +import { build } from 'tsup'; +import { createGeoPoint, createMarkerState } from '@mapconductor/js-sdk-core'; + +// The Mappls map is on the mapbox-gl v1 line: loadImage(url, callback) returns nothing. +// The renderer awaited it in maplibre's promise form, never passed a callback, and the SDK +// threw "e is not a function" when its fetch finished (a pageerror on every page with a +// marker). This fake has exactly that callback-only signature. +mkdirSync(new URL('../build/', import.meta.url), { recursive: true }); +const outDir = mkdtempSync(new URL('../build/marker-icon-tests-', import.meta.url)); +after(() => rmSync(outDir, { recursive: true, force: true })); +await build({ + entry: { renderer: 'src/marker/MapplsMarkerOverlayRenderer.ts' }, + outDir, format: ['esm'], platform: 'node', splitting: false, + dts: false, sourcemap: false, config: false, silent: true, +}); +const { MapplsMarkerOverlayRenderer } = await import(pathToFileURL(resolve(outDir, 'renderer.mjs'))); + +function fixture() { + const images = new Map(); + const calls = []; + const map = { + hasImage: (id) => images.has(id), + addImage: (id, image) => { images.set(id, image); }, + loadImage(url, callback) { + calls.push({ url, callbackType: typeof callback }); + setTimeout(() => { + if (typeof callback !== 'function') return; // the real SDK throws here + callback(null, { fakeImage: url }); + }, 0); + return undefined; + }, + }; + const noLayer = { draw() { return true; }, drawSelected() { return true; }, setIconOffsets() {} }; + const renderer = new MapplsMarkerOverlayRenderer({ + holder: { map }, + markerManager: { allEntities: () => [] }, + markerLayer: noLayer, + dragLayer: noLayer, + }); + return { renderer, images, calls }; +} + +test('marker icons are loaded through the callback form of map.loadImage', async () => { + const f = fixture(); + const state = createMarkerState({ id: 'M', position: createGeoPoint({ latitude: 35, longitude: 139 }) }); + await f.renderer.onAdd([{ state, bitmapIcon: state.icon?.toBitmapIcon?.() ?? { url: 'data:image/png;base64,' } }]); + assert.ok(f.calls.length > 0, 'loadImage was used'); + assert.ok(f.calls.every((c) => c.callbackType === 'function'), 'every loadImage call passes a callback'); + assert.deepEqual(f.images.get('mc-default'), { fakeImage: f.calls[0].url }, 'the loaded image is registered'); +}); diff --git a/test/ResyncWritesNothing.test.mjs b/test/ResyncWritesNothing.test.mjs new file mode 100644 index 0000000..ccb2904 --- /dev/null +++ b/test/ResyncWritesNothing.test.mjs @@ -0,0 +1,129 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { after, test } from 'node:test'; +import { pathToFileURL } from 'node:url'; +import { resolve } from 'node:path'; +import { build } from 'tsup'; +import { createGeoPoint, createPolylineState, PolylineManager } from '@mapconductor/js-sdk-core'; + +// The provider marks the style "not ready" on a styledata that arrives while a source is +// still loading, and resyncs every overlay on the idle that follows. A resync that rewrote +// unchanged sources (setData) or re-moved layers that were already on top (moveLayer) fired +// another styledata, so the style flipped between ready and not ready every ~0.3 s forever. +// These tests pin "a resync with nothing to do writes nothing", and that a lost style is +// still rebuilt. +mkdirSync(new URL('../build/', import.meta.url), { recursive: true }); +const outDir = mkdtempSync(new URL('../build/resync-tests-', import.meta.url)); +after(() => rmSync(outDir, { recursive: true, force: true })); +await build({ + entry: { + helpers: 'src/helpers.ts', + controller: 'src/polyline/MapplsPolylineController.ts', + renderer: 'src/polyline/MapplsPolylineOverlayRenderer.ts', + layer: 'src/polyline/MapplsPolylineLayer.ts', + }, + outDir, format: ['esm'], platform: 'node', splitting: false, + dts: false, sourcemap: false, config: false, silent: true, +}); +const { bringMarkerLayersToFront, setSourceFeatures } = await import(pathToFileURL(resolve(outDir, 'helpers.mjs'))); +const { MapplsPolylineController } = await import(pathToFileURL(resolve(outDir, 'controller.mjs'))); +const { MapplsPolylineOverlayRenderer } = await import(pathToFileURL(resolve(outDir, 'renderer.mjs'))); +const { MapplsPolylineLayer } = await import(pathToFileURL(resolve(outDir, 'layer.mjs'))); + +function fakeMap(initialLayers = ['background', 'road-label']) { + const writes = { setData: 0, moveLayer: 0, addSource: 0, addLayer: 0 }; + let sources = new Map(); + let order = [...initialLayers]; + const layers = new Map(initialLayers.map((id) => [id, { id }])); + const map = { + writes, + get order() { return [...order]; }, + getLayersOrder: () => [...order], + style: { get order() { return [...order]; } }, + getStyle: () => ({ layers: order.map((id) => layers.get(id)) }), + getSource: (id) => sources.get(id), + getLayer: (id) => layers.get(id), + addSource(id) { + writes.addSource++; + sources.set(id, { setData() { writes.setData++; } }); + }, + addLayer(layer, before) { + writes.addLayer++; + layers.set(layer.id, layer); + const at = before ? order.indexOf(before) : -1; + if (at >= 0) order.splice(at, 0, layer.id); else order.push(layer.id); + }, + moveLayer(id, before) { + writes.moveLayer++; + order = order.filter((x) => x !== id); + const at = before ? order.indexOf(before) : -1; + if (at >= 0) order.splice(at, 0, id); else order.push(id); + }, + // A style swap drops every source and layer the SDK added. + reloadStyle() { + sources = new Map(); + order = order.filter((id) => !id.startsWith('mc-') && !id.startsWith('polyline')); + for (const id of [...layers.keys()]) if (!order.includes(id)) layers.delete(id); + }, + }; + return map; +} + +test('bringMarkerLayersToFront moves nothing when the marker layers are already on top', () => { + const map = fakeMap(['background', 'mc-marker-layer', 'mc-marker-drag-layer']); + bringMarkerLayersToFront(map); + assert.equal(map.writes.moveLayer, 0); +}); + +test('bringMarkerLayersToFront moves the marker layers once, then leaves them alone', () => { + const map = fakeMap(['background', 'mc-marker-layer', 'mc-marker-drag-layer']); + map.addLayer({ id: 'polyline-layer' }); + bringMarkerLayersToFront(map); + assert.deepEqual(map.order.slice(-2), ['mc-marker-layer', 'mc-marker-drag-layer']); + const moved = map.writes.moveLayer; + assert.ok(moved > 0); + bringMarkerLayersToFront(map); + bringMarkerLayersToFront(map); + assert.equal(map.writes.moveLayer, moved); +}); + +test('setSourceFeatures skips identical features but writes to a replaced source', () => { + let calls = 0; + const source = { setData() { calls++; } }; + const a = { type: 'Feature' }; + const b = { type: 'Feature' }; + setSourceFeatures(source, [a, b]); + setSourceFeatures(source, [a, b]); + assert.equal(calls, 1); + setSourceFeatures(source, [b, a]); + assert.equal(calls, 2, 'a different order is a change'); + setSourceFeatures(source, [b]); + assert.equal(calls, 3, 'a removed feature is a change'); + const replaced = { setData() { calls++; } }; + setSourceFeatures(replaced, [b]); + assert.equal(calls, 4, 'a new source object (style swap) is always written'); +}); + +test('resync with nothing changed writes nothing; after a style swap it rebuilds', async () => { + const map = fakeMap(['background', 'road-label', 'mc-marker-layer', 'mc-marker-drag-layer']); + const holder = { map }; + const layer = new MapplsPolylineLayer({ holder, canEditStyle: () => true }); + const renderer = new MapplsPolylineOverlayRenderer({ layer, polylineManager: new PolylineManager(), holder }); + const controller = new MapplsPolylineController(renderer); + await controller.add([createPolylineState({ + id: 'PL', + points: [createGeoPoint({ latitude: 35, longitude: 139 }), createGeoPoint({ latitude: 36, longitude: 140 })], + })]); + const afterAdd = { ...map.writes }; + assert.ok(afterAdd.setData > 0); + + await controller.resync(); + await controller.resync(); + assert.deepEqual(map.writes, afterAdd, 'a resync with nothing to do must not touch the style'); + + map.reloadStyle(); + await controller.resync(); + assert.ok(map.getSource('polyline-source'), 'the lost source is added again'); + assert.ok(map.writes.setData > afterAdd.setData, 'and its data written again'); + assert.ok(map.order.includes('polyline-layer'), 'and its layer'); +});