From 446865ea0e7d8c945f8c180063009390099adb50 Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Sat, 7 Nov 2026 19:07:15 -0800 Subject: [PATCH 1/2] Drop the clear() overrides on polyline and circle They redrew after super.clear() because js-sdk-core's clear() called onPostProcess before emptying the manager (polyline) or not at all (circle), so this provider's redraw-from-manager renderer kept drawing what clear() removed. js-sdk-core now runs onRemove, manager.clear(), onPostProcess, which redraws an empty layer. clear() is still there, inherited from the core controller with the same signature; the API snapshot only stops listing the override. Requires the js-sdk-core release with that change (MapConductor/js-sdk-core fix/overlay-clear-order). Against an older core, clearOverlays() leaves polylines and circles drawn until something else redraws them. --- api/react-for-mapbox.api.d.ts | 2 -- src/circle/MapboxCircleController.ts | 5 ----- src/polyline/MapboxPolylineController.ts | 5 ----- 3 files changed, 12 deletions(-) diff --git a/api/react-for-mapbox.api.d.ts b/api/react-for-mapbox.api.d.ts index b22d9c2..b74d19f 100644 --- a/api/react-for-mapbox.api.d.ts +++ b/api/react-for-mapbox.api.d.ts @@ -245,7 +245,6 @@ declare class MapboxCircleController extends CircleController; resync(): Promise; - clear(): Promise; /** * Hit-test a map click (its lat/lng) against the circles geometrically (inside * the fill radius) and dispatch the click on the matching circle. Does NOT use @@ -300,7 +299,6 @@ declare class MapboxPolylineController extends PolylineController; - clear(): Promise; /** * Hit-test a map click (its lat/lng) against the polylines geometrically and, * if the click lands within the tap tolerance of a line, dispatch the click on diff --git a/src/circle/MapboxCircleController.ts b/src/circle/MapboxCircleController.ts index 7a13c22..69240c7 100644 --- a/src/circle/MapboxCircleController.ts +++ b/src/circle/MapboxCircleController.ts @@ -24,11 +24,6 @@ export class MapboxCircleController extends CircleController await this.renderer.redraw(); } - override async clear(): Promise { - await super.clear(); - await this.renderer.redraw(); - } - /** * Hit-test a map click (its lat/lng) against the circles geometrically (inside * the fill radius) and dispatch the click on the matching circle. Does NOT use diff --git a/src/polyline/MapboxPolylineController.ts b/src/polyline/MapboxPolylineController.ts index b28bac0..9766a72 100644 --- a/src/polyline/MapboxPolylineController.ts +++ b/src/polyline/MapboxPolylineController.ts @@ -19,11 +19,6 @@ export class MapboxPolylineController extends PolylineController { - await super.clear(); - await this.renderer.redraw(); - } - /** * Hit-test a map click (its lat/lng) against the polylines geometrically and, * if the click lands within the tap tolerance of a line, dispatch the click on From 29fa8a466058bb7c2dc50f52d23ff48862dba6a9 Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Sat, 7 Nov 2026 20:03:39 -0800 Subject: [PATCH 2/2] Hold ground image removals until the style can be edited removeGroundImage returned early while canEditStyle() was false, and the controller had already forgotten the state, so the image stayed on the map for good. canEditStyle follows isStyleLoaded(), which turns false right after other overlays rewrite their sources; clearOverlays() clears those first, so the ground image survived every clearOverlays(). The renderer now keeps such removals and applies them from the controller's resync(), which runs when the style becomes editable again. Adding the same id again before then cancels the held removal. GroundImageRemoval.test.mjs covers both. --- api/react-for-mapbox.api.d.ts | 2 + .../MapboxGroundImageController.ts | 2 + .../MapboxGroundImageOverlayRenderer.ts | 33 ++++- test/GroundImageRemoval.test.mjs | 127 ++++++++++++++++++ 4 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 test/GroundImageRemoval.test.mjs diff --git a/api/react-for-mapbox.api.d.ts b/api/react-for-mapbox.api.d.ts index b74d19f..48dd38c 100644 --- a/api/react-for-mapbox.api.d.ts +++ b/api/react-for-mapbox.api.d.ts @@ -396,6 +396,7 @@ declare class MapboxGroundImageOverlayRenderer extends AbstractGroundImageOverla private readonly canEditStyle; /** Last values applied to the map style, keyed by state id. */ private readonly applied; + private readonly pendingRemovals; constructor({ holder, canEditStyle, }: { holder: MapboxMapViewHolder; canEditStyle: () => boolean; @@ -411,6 +412,7 @@ declare class MapboxGroundImageOverlayRenderer extends AbstractGroundImageOverla /** Sync an already-created image source+layer to the current state (diffed). */ private applyToExisting; removeGroundImage(entity: GroundImageEntity): Promise; + private removeFromStyle; } declare class MapboxGroundImageController implements SlottedOverlayController { diff --git a/src/groundimage/MapboxGroundImageController.ts b/src/groundimage/MapboxGroundImageController.ts index a5adea7..b587005 100644 --- a/src/groundimage/MapboxGroundImageController.ts +++ b/src/groundimage/MapboxGroundImageController.ts @@ -68,6 +68,8 @@ export class MapboxGroundImageController implements SlottedOverlayController { resync(): void { this.cancelPendingUpdates(); + // Called when the style becomes editable again: apply removals held back meanwhile. + this.renderer.flushPendingRemovals(); this.groundImageIds.clear(); for (const state of this.groundImageStates.values()) void this.upsert(state); } diff --git a/src/groundimage/MapboxGroundImageOverlayRenderer.ts b/src/groundimage/MapboxGroundImageOverlayRenderer.ts index f8c1603..3a35280 100644 --- a/src/groundimage/MapboxGroundImageOverlayRenderer.ts +++ b/src/groundimage/MapboxGroundImageOverlayRenderer.ts @@ -18,6 +18,13 @@ export class MapboxGroundImageOverlayRenderer extends AbstractGroundImageOverlay private readonly canEditStyle: () => boolean; /** Last values applied to the map style, keyed by state id. */ private readonly applied = new Map(); + // Removals that arrived while the style could not be edited, applied by + // flushPendingRemovals() once it can. Dropping them instead left the image on + // the map for good: the controller forgets the state as soon as it asks for + // the removal, so nothing would ask again. canEditStyle turns false whenever + // isStyleLoaded() does, which includes the moments right after other overlays + // rewrite their sources, so clearOverlays() hit this every time. + private readonly pendingRemovals = new Set(); constructor({ holder, @@ -34,6 +41,8 @@ export class MapboxGroundImageOverlayRenderer extends AbstractGroundImageOverlay layerId(id: string): string { return `mc-gimg-lyr-${id}`; } async createGroundImage(state: GroundImageState): Promise { + // Added again before a held-back removal ran: the caller wants it on the map. + this.pendingRemovals.delete(state.id); const coordinates = groundImageCoordinates(state); if (!coordinates) return null; @@ -149,8 +158,26 @@ export class MapboxGroundImageOverlayRenderer extends AbstractGroundImageOverlay async removeGroundImage(entity: GroundImageEntity): Promise { this.applied.delete(entity.groundImage); - if (!this.canEditStyle()) return; - removeLayerIfExists(this.holder.map, this.layerId(entity.groundImage)); - removeSourceIfExists(this.holder.map, this.sourceId(entity.groundImage)); + if (!this.canEditStyle()) { + this.pendingRemovals.add(entity.groundImage); + return; + } + this.pendingRemovals.delete(entity.groundImage); + this.removeFromStyle(entity.groundImage); + } + + /** + * Applies the removals held back while the style could not be edited. + * @internal + */ + flushPendingRemovals(): void { + if (this.pendingRemovals.size === 0 || !this.canEditStyle()) return; + for (const id of this.pendingRemovals) this.removeFromStyle(id); + this.pendingRemovals.clear(); + } + + private removeFromStyle(id: string): void { + removeLayerIfExists(this.holder.map, this.layerId(id)); + removeSourceIfExists(this.holder.map, this.sourceId(id)); } } diff --git a/test/GroundImageRemoval.test.mjs b/test/GroundImageRemoval.test.mjs new file mode 100644 index 0000000..1d99cc7 --- /dev/null +++ b/test/GroundImageRemoval.test.mjs @@ -0,0 +1,127 @@ +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, createGeoRectBounds, createGroundImageState } from '@mapconductor/js-sdk-core'; + +// Exercise the actual internal classes without exporting them as public API. +// Only the map is replaced; no DOM, WebGL, or network is needed. +mkdirSync(new URL('../build/', import.meta.url), { recursive: true }); +const outDir = mkdtempSync(new URL('../build/groundimage-tests-', import.meta.url)); +after(() => rmSync(outDir, { recursive: true, force: true })); +await build({ + entry: { + controller: 'src/groundimage/MapboxGroundImageController.ts', + renderer: 'src/groundimage/MapboxGroundImageOverlayRenderer.ts', + }, + outDir, format: ['esm'], platform: 'node', splitting: false, + dts: false, sourcemap: false, config: false, silent: true, +}); +const { MapboxGroundImageController } = await import(pathToFileURL(resolve(outDir, 'controller.mjs'))); +const { MapboxGroundImageOverlayRenderer } = await import(pathToFileURL(resolve(outDir, 'renderer.mjs'))); + +const sourceId = 'mc-gimg-src-GI'; +const layerId = 'mc-gimg-lyr-GI'; + +function state(opacity = 0.9) { + return createGroundImageState({ + id: 'GI', + bounds: createGeoRectBounds({ + southWest: createGeoPoint({ latitude: 35.64, longitude: 139.74 }), + northEast: createGeoPoint({ latitude: 35.665, longitude: 139.78 }), + }), + imageUrl: '/image.png', + opacity, + }); +} + +function fixture() { + const sources = new Map(); + const layers = new Map(); + let ready = true; + const map = { + getSource: (id) => sources.get(id), + getLayer: (id) => layers.get(id), + addSource(id, source) { + assert.ok(ready, 'cannot add a source while the style is unavailable'); + assert.ok(!sources.has(id)); + sources.set(id, { ...source, updateImage() {}, setCoordinates() {} }); + }, + addLayer(layer) { + assert.ok(ready, 'cannot add a layer while the style is unavailable'); + assert.ok(sources.has(layer.source)); + assert.ok(!layers.has(layer.id)); + layers.set(layer.id, layer); + }, + removeLayer(id) { + assert.ok(ready, 'cannot remove a layer while the style is unavailable'); + layers.delete(id); + }, + removeSource(id) { + assert.ok(ready, 'cannot remove a source while the style is unavailable'); + assert.ok(![...layers.values()].some((layer) => layer.source === id)); + sources.delete(id); + }, + setPaintProperty(id, property, value) { + layers.get(id).paint[property] = value; + }, + moveLayer() {}, + }; + const renderer = new MapboxGroundImageOverlayRenderer({ holder: { map }, canEditStyle: () => ready }); + const controller = new MapboxGroundImageController(renderer); + return { + controller, + setReady(value) { ready = value; }, + onMap: () => sources.has(sourceId) || layers.has(layerId), + }; +} + +const settle = () => new Promise((r) => setTimeout(r, 0)); + +test('a removal while the style is not editable is applied once it is', async () => { + const f = fixture(); + f.controller.composition([state()]); + await settle(); + assert.ok(f.onMap()); + + // clearOverlays() runs right after other overlays rewrote their sources, + // when isStyleLoaded() -- and so canEditStyle -- is briefly false. + f.setReady(false); + f.controller.clear(); + await settle(); + assert.equal(f.controller.hasId('GI'), false); + + f.setReady(true); + f.controller.resync(); + await settle(); + assert.equal(f.onMap(), false, 'the cleared ground image is still on the map'); +}); + +test('adding the same id again before the style is editable keeps it', async () => { + const f = fixture(); + f.controller.composition([state()]); + await settle(); + + f.setReady(false); + f.controller.clear(); + await settle(); + f.controller.composition([state(0.4)]); + await settle(); + + f.setReady(true); + f.controller.resync(); + await settle(); + assert.ok(f.onMap(), 'the re-added ground image was removed'); + assert.equal(f.controller.hasId('GI'), true); +}); + +test('a removal while the style is editable is applied at once', async () => { + const f = fixture(); + f.controller.composition([state()]); + await settle(); + f.controller.composition([]); + await settle(); + assert.equal(f.onMap(), false); +});