From 85908a95b8ec20d2789cf2914b17a610de20302a Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Sat, 7 Nov 2026 22:13:36 -0800 Subject: [PATCH] Clear StrategyMarkerController in the same order as the other controllers clear() called onRemove and emptied the strategy but never onPostProcess. On the web every provider hands strategies a CollectorMarkerOverlayRenderer, which stages adds and removals and commits them to the marker collector in onPostProcess, so the removals were never committed and the markers stayed on the map (checked in the browser on maplibre, mapbox, mappls, tomtom and longdo). The GL renderers on android and iOS draw from the manager in onPostProcess, which has the same effect. clear() now runs onRemove(everything), strategy.clear() (so a strategy's own bookkeeping, such as the cluster state, is still reset), then onPostProcess, under the same lock as add/update. Markers queued before the first camera are dropped too; they would otherwise appear on the first camera event after clear(). Nothing in the SDK calls this clear() today (clearOverlays() clears the providers' own controllers only), so the visible effect is for code that drives a StrategyMarkerController directly. StrategyMarkerClearOrder.test.mjs, matching the android-sdk-core and ios-sdk-core tests. --- src/marker/StrategyMarkerController.ts | 25 ++++--- test/StrategyMarkerClearOrder.test.mjs | 90 ++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 10 deletions(-) create mode 100644 test/StrategyMarkerClearOrder.test.mjs diff --git a/src/marker/StrategyMarkerController.ts b/src/marker/StrategyMarkerController.ts index 883881f..acb7f9d 100644 --- a/src/marker/StrategyMarkerController.ts +++ b/src/marker/StrategyMarkerController.ts @@ -122,16 +122,21 @@ export class StrategyMarkerController } async clear(): Promise { - // Release the renderer's per-marker resources (icon images / sprites) - // before dropping the entities. strategy.clear() only clears the manager's - // bookkeeping and never calls onRemove, so without this the GL providers' - // reference-counted icon images and Azure's sprite images would stay - // registered on the map after an explicit clear() while the map is alive. - const entities = this.strategy.markerManager.allEntities(); - if (entities.length > 0) { - await this.renderer.onRemove(entities); - } - this.strategy.clear(); + // Same order as the other controllers' clear(): onRemove(everything) → empty → onPostProcess. + // onRemove releases the renderer's per-marker resources (the GL providers' reference-counted + // icon images, Azure's sprites); strategy.clear() only drops the bookkeeping. The GL renderers + // draw from the manager in onPostProcess, so without it the cleared markers stayed on the map. + // Under the same lock as add/update so a clear cannot interleave with a strategy pass. + await this.semaphore.withLock(async () => { + const entities = this.strategy.markerManager.allEntities(); + if (entities.length > 0) { + await this.renderer.onRemove(entities); + } + this.strategy.clear(); + // Markers queued before the first camera fix were cleared too; don't add them later. + this.pendingStates = null; + await this.renderer.onPostProcess(); + }); } getEntity(id: string): MarkerEntity | null { diff --git a/test/StrategyMarkerClearOrder.test.mjs b/test/StrategyMarkerClearOrder.test.mjs new file mode 100644 index 0000000..3a99a01 --- /dev/null +++ b/test/StrategyMarkerClearOrder.test.mjs @@ -0,0 +1,90 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; + +import { + MarkerManager, + StrategyMarkerController, + createGeoPoint, + createGeoRectBounds, + createMarkerEntity, + createMarkerState, +} from '../dist/index.mjs'; + +/** + * StrategyMarkerController.clear() must leave nothing drawn, like the other controllers' + * clear() (OverlayClearOrder.test.mjs): onRemove(everything) → empty the manager → + * onPostProcess. It used to call onRemove and empty the manager but never onPostProcess, + * so a GL renderer, which redraws from the manager in onPostProcess, kept showing the + * markers. Same test on android-sdk-core and ios-sdk-core. + */ + +const bounds = createGeoRectBounds({ + southWest: createGeoPoint({ latitude: 34, longitude: 138 }), + northEast: createGeoPoint({ latitude: 36, longitude: 140 }), +}); +const camera = { position: createGeoPoint({ latitude: 35, longitude: 139 }), zoom: 10, visibleRegion: { bounds } }; + +/** GL-style renderer: onRemove touches nothing, onPostProcess redraws the manager. */ +class GlRenderer { + constructor(manager) { this.manager = manager; this.drawn = []; this.removed = []; this.holder = null; } + async onAdd(data) { return data.map((p) => p.state.id); } + async onChange(data) { return data.map((p) => p.current.state.id); } + async onRemove(data) { this.removed.push(...data.map((e) => e.state.id)); } + async onAnimate() {} + async onPostProcess() { this.drawn = this.manager.allEntities().map((e) => e.state.id).sort(); } +} + +/** Draws every marker it is given, like the default strategies. */ +class DrawAllStrategy { + constructor() { this.markerManager = MarkerManager.defaultManager(); this.cleared = 0; } + clear() { this.cleared++; this.markerManager.clear(); } + async onAdd({ data, renderer }) { + const markers = await renderer.onAdd(data.map((state) => ({ state, bitmapIcon: null }))); + data.forEach((state, i) => this.markerManager.registerEntity(createMarkerEntity({ state, marker: markers[i] }))); + await renderer.onPostProcess(); + return true; + } + async onUpdate() { return true; } + async onCameraChanged() {} +} + +function fixture() { + const strategy = new DrawAllStrategy(); + const renderer = new GlRenderer(strategy.markerManager); + const controller = new StrategyMarkerController({ strategy, renderer }); + return { strategy, renderer, controller }; +} + +const markers = () => [ + createMarkerState({ id: 'm1', position: createGeoPoint({ latitude: 35, longitude: 139 }) }), + createMarkerState({ id: 'm2', position: createGeoPoint({ latitude: 35.1, longitude: 139.1 }) }), +]; + +test('clear() removes, empties the strategy, then post-processes, so nothing stays drawn', async () => { + const f = fixture(); + await f.controller.onCameraChanged(camera); + await f.controller.add(markers()); + assert.deepEqual(f.renderer.drawn, ['m1', 'm2']); + + await f.controller.clear(); + assert.deepEqual(f.renderer.removed.sort(), ['m1', 'm2'], 'onRemove gets every entity'); + assert.equal(f.strategy.cleared, 1, "the strategy's own clear() still runs"); + assert.deepEqual(f.renderer.drawn, [], 'nothing is drawn after clear()'); +}); + +test('clear() drops markers still waiting for the first camera', async () => { + const f = fixture(); + await f.controller.add(markers()); // no camera yet: queued + await f.controller.clear(); + await f.controller.onCameraChanged(camera); + assert.deepEqual(f.renderer.drawn, [], 'the queued markers must not appear after clear()'); +}); + +test('markers sent again after clear() are drawn again', async () => { + const f = fixture(); + await f.controller.onCameraChanged(camera); + await f.controller.add(markers()); + await f.controller.clear(); + await f.controller.add(markers()); + assert.deepEqual(f.renderer.drawn, ['m1', 'm2']); +});