IM-253 moved back addSymbolToMap

providers/maplibre/src/maplibreProvider.js

@@ -13,6 +13,7 @@ import { createMapLabelNavigator } from './utils/labels.js'
 import { updateHighlightedFeatures } from './utils/highlightFeatures.js'
 import { queryFeatures } from './utils/queryFeatures.js'
 import { setupHoverCursor } from './utils/hoverCursor.js'
+import { addSymbolsToMap } from './utils/symbolImages.js'
 import { addPatternsToMap } from './utils/patternImages.js'
 
 /**
@@ -325,7 +326,7 @@ export default class MapLibreProvider {
    */
   async addSymbolsToMap (symbolConfigs, mapStyle, symbolRegistry) {
     const pixelRatio = (this.map.getPixelRatio() || 1) * (scaleFactor[this.mapSize] || 1)
-    return symbolRegistry.addSymbolsToMap(this.map, symbolConfigs, mapStyle, pixelRatio)
+    return addSymbolsToMap(this.map, symbolConfigs, mapStyle, symbolRegistry, pixelRatio)
   }
 
   /**

providers/maplibre/src/maplibreProvider.test.js

@@ -4,10 +4,11 @@ import { attachAppEvents } from './appEvents.js'
 import { createMapLabelNavigator } from './utils/labels.js'
 import { updateHighlightedFeatures } from './utils/highlightFeatures.js'
 import { queryFeatures } from './utils/queryFeatures.js'
+import { addSymbolsToMap } from './utils/symbolImages.js'
 import { addPatternsToMap } from './utils/patternImages.js'
 import { getAreaDimensions, getCardinalMove, getResolution, getPaddedBounds, isGeometryObscured } from './utils/spatial.js'
 import { symbolRegistry } from '../../../src/services/symbolRegistry.js'
-const addSymbolsToMapSpy = jest.spyOn(symbolRegistry, 'addSymbolsToMap').mockImplementation(() => Promise.resolve())
+// const addSymbolsToMapSpy = jest.spyOn(symbolRegistry, 'addSymbolsToMap').mockImplementation(() => Promise.resolve())
 
 jest.mock('./defaults.js', () => ({
   DEFAULTS: { animationDuration: 400, coordinatePrecision: 7 },
@@ -36,6 +37,7 @@ jest.mock('./utils/labels.js', () => ({
 }))
 jest.mock('./utils/highlightFeatures.js', () => ({ updateHighlightedFeatures: jest.fn(() => []) }))
 jest.mock('./utils/queryFeatures.js', () => ({ queryFeatures: jest.fn(() => []) }))
+jest.mock('./utils/symbolImages.js', () => ({ addSymbolsToMap: jest.fn(() => Promise.resolve()) }))
 jest.mock('./utils/patternImages.js', () => ({ addPatternsToMap: jest.fn(() => Promise.resolve()) }))
 
 describe('MapLibreProvider', () => {
@@ -274,7 +276,7 @@ describe('MapLibreProvider', () => {
     const configs = [{ symbol: 'pin' }]
     const mapStyle = { id: 'test', selectedColor: '#0b0c0c' }
     await p.addSymbolsToMap(configs, mapStyle, symbolRegistry)
-    expect(addSymbolsToMapSpy).toHaveBeenCalledWith(map, configs, mapStyle, expect.any(Number))
+    expect(addSymbolsToMap).toHaveBeenCalledWith(map, configs, mapStyle, symbolRegistry, expect.any(Number))
   })
 
   test('addSymbolsToMap computes pixelRatio from getPixelRatio and mapSize scale factor', async () => {
@@ -283,7 +285,7 @@ describe('MapLibreProvider', () => {
     map.getPixelRatio.mockReturnValue(2)
     p.mapSize = 'medium' // scaleFactor['medium'] = 1.5
     await p.addSymbolsToMap([], { id: 'test' }, symbolRegistry)
-    expect(addSymbolsToMapSpy).toHaveBeenCalledWith(map, [], { id: 'test' }, 3)
+    expect(addSymbolsToMap).toHaveBeenCalledWith(map, [], { id: 'test' }, symbolRegistry, 3)
   })
 
   test('addSymbolsToMap falls back to pixelRatio 1 when getPixelRatio returns 0', async () => {
@@ -292,7 +294,7 @@ describe('MapLibreProvider', () => {
     map.getPixelRatio.mockReturnValue(0)
     p.mapSize = 'small' // scaleFactor['small'] = 1
     await p.addSymbolsToMap([], { id: 'test' }, symbolRegistry)
-    expect(addSymbolsToMapSpy).toHaveBeenCalledWith(map, [], { id: 'test' }, 1)
+    expect(addSymbolsToMap).toHaveBeenCalledWith(map, [], { id: 'test' }, symbolRegistry, 1)
   })
 
   test('addPatternsToMap delegates to utility with map instance and pixelRatio', async () => {

providers/maplibre/src/utils/symbolImages.js

@@ -35,3 +35,50 @@ export const anchorToMaplibre = ([ax, ay]) => {
   const y = yAnchor(ay)
   return (y + (x && y ? '-' : '') + x) || 'center'
 }
+
+/**
+ * Register normal, active (both rings) and selected (black ring) symbol images.
+ * Skips images that are already registered (safe to call on style change).
+ * Updates `map._activeSymbolImageMap` (normal→active) and `map._selectedSymbolImageMap` (normal→selected).
+ *
+ * Callers are responsible for resolving sublayers before calling this function
+ * (see `getSymbolConfigs` in the datasets plugin adapter).
+ *
+ * @param {Object} map - MapLibre map instance
+ * @param {Object[]} styleArray - Flat list of datasets/merged-sublayers that have a symbol config
+ * @param {Object} mapStyle - Current map style config (provides id, selectedColor, haloColor)
+ * @param {Object} symbolRegistry
+ * @param {number} [pixelRatio=2] - Device pixel ratio × map size scale factor (computed by caller)
+ * @returns {Promise<void>}
+ */
+export const addSymbolsToMap = async (map, styleArray, mapStyle, symbolRegistry, pixelRatio = 2) => {
+  if (!styleArray.length) {
+    return
+  }
+
+  map._activeSymbolImageMap = {}
+  map._selectedSymbolImageMap = {}
+
+  await Promise.all(styleArray.flatMap(config => {
+    const normalId = symbolRegistry.getSymbolImageId(config, mapStyle, false, pixelRatio)
+    const activeId = symbolRegistry.getSymbolImageId(config, mapStyle, true, pixelRatio)
+    if (normalId && activeId) {
+      map._activeSymbolImageMap[normalId] = activeId
+    }
+    return ['normal', 'active', 'selected'].map(async (variant) => {
+      const imageId = variant === 'active' ? activeId : normalId
+      if (variant !== 'selected' && (!imageId || map.hasImage(imageId))) {
+        return
+      }
+      const result = await symbolRegistry.rasteriseSymbolImage(config, mapStyle, variant, pixelRatio)
+      if (result) {
+        if (variant === 'selected' && normalId) {
+          map._selectedSymbolImageMap[normalId] = result.imageId
+        }
+        if (!map.hasImage(result.imageId)) {
+          map.addImage(result.imageId, result.imageData, { pixelRatio })
+        }
+      }
+    })
+  }))
+}

providers/maplibre/src/utils/symbolImages.test.js

@@ -1,6 +1,27 @@
-import { anchorToMaplibre } from './symbolImages.js'
+import { anchorToMaplibre, addSymbolsToMap } from './symbolImages.js'
 import { symbolRegistry } from '../../../../src/services/symbolRegistry.js'
 
+beforeAll(() => {
+  globalThis.URL.createObjectURL = jest.fn(() => 'blob:mock')
+  globalThis.URL.revokeObjectURL = jest.fn()
+
+  HTMLCanvasElement.prototype.getContext = jest.fn(() => ({
+    drawImage: jest.fn(),
+    getImageData: jest.fn((_x, _y, w, h) => ({ width: w, height: h }))
+  }))
+
+  globalThis.Image = class {
+    constructor (w, h) {
+      this.width = w
+      this.height = h
+      this._src = ''
+    }
+
+    get src () { return this._src }
+    set src (val) { this._src = val; this.onload?.() }
+  }
+})
+
 beforeEach(() => {
   symbolRegistry.setDefaults({})
 })
@@ -62,3 +83,121 @@ describe('anchorToMaplibre', () => {
     expect(anchorToMaplibre([0.5, 0.75])).toBe('bottom') // NOSONAR S109 — ANCHOR_HIGH boundary
   })
 })
+
+// ─── addSymbolsToMap ──────────────────────────────────────────────────────────
+
+const makeMap = (existingIds = []) => ({
+  _activeSymbolImageMap: {},
+  _selectedSymbolImageMap: {},
+  hasImage: jest.fn((id) => existingIds.includes(id)),
+  addImage: jest.fn()
+})
+const STYLE_ID = 'test'
+const mapStyle = { id: STYLE_ID }
+
+describe('addSymbolsToMap — registration', () => {
+  it('returns early and does not touch map for empty configs', async () => {
+    const map = makeMap()
+    await addSymbolsToMap(map, [], mapStyle, symbolRegistry)
+    expect(map.hasImage).not.toHaveBeenCalled()
+    expect(map.addImage).not.toHaveBeenCalled()
+  })
+
+  it('resets _activeSymbolImageMap and _selectedSymbolImageMap before processing', async () => {
+    const map = makeMap()
+    map._activeSymbolImageMap = { stale: 'entry' }
+    map._selectedSymbolImageMap = { stale: 'entry' }
+    await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry)
+    expect(map._activeSymbolImageMap).not.toHaveProperty('stale')
+    expect(map._selectedSymbolImageMap).not.toHaveProperty('stale')
+  })
+
+  it('calls addImage for normal, active and selected variants', async () => {
+    const map = makeMap()
+    await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry)
+    expect(map.addImage).toHaveBeenCalledTimes(3) // NOSONAR S109 — normal, active, selected
+    expect(map.addImage).toHaveBeenCalledWith(expect.stringMatching(/^symbol-[a-z0-9]+-\d+(\.\d+)?x$/), expect.any(Object), { pixelRatio: 2 })
+    expect(map.addImage).toHaveBeenCalledWith(expect.stringMatching(/^symbol-act-[a-z0-9]+-\d+(\.\d+)?x$/), expect.any(Object), { pixelRatio: 2 })
+    expect(map.addImage).toHaveBeenCalledWith(expect.stringMatching(/^symbol-sel-[a-z0-9]+-\d+(\.\d+)?x$/), expect.any(Object), { pixelRatio: 2 })
+  })
+
+  it('populates _activeSymbolImageMap and _selectedSymbolImageMap with normal → variant id pairs', async () => {
+    const map = makeMap()
+    await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry)
+    const normalId = symbolRegistry.getSymbolImageId({ symbol: 'pin' }, mapStyle, false)
+    const activeId = symbolRegistry.getSymbolImageId({ symbol: 'pin' }, mapStyle, true)
+    const selectedId = map._selectedSymbolImageMap[normalId]
+    expect(map._activeSymbolImageMap[normalId]).toBe(activeId)
+    expect(selectedId).toMatch(/^symbol-sel-[a-z0-9]+-\d+(\.\d+)?x$/)
+  })
+
+  it('skips addImage when all three variant images are already registered', async () => {
+    // Run once to discover the selected image ID (not derivable without rasterising)
+    const setupMap = makeMap()
+    await addSymbolsToMap(setupMap, [{ symbol: 'circle' }], mapStyle, symbolRegistry)
+    const normalId = symbolRegistry.getSymbolImageId({ symbol: 'circle' }, mapStyle, false)
+    const activeId = symbolRegistry.getSymbolImageId({ symbol: 'circle' }, mapStyle, true)
+    const selectedId = setupMap._selectedSymbolImageMap[normalId]
+
+    const map = makeMap([normalId, activeId, selectedId])
+    await addSymbolsToMap(map, [{ symbol: 'circle' }], mapStyle, symbolRegistry)
+    expect(map.addImage).not.toHaveBeenCalled()
+  })
+
+  it('processes multiple configs independently', async () => {
+    const map = makeMap()
+    await addSymbolsToMap(map, [{ symbol: 'pin' }, { symbol: 'circle' }], mapStyle, symbolRegistry)
+    expect(map.addImage).toHaveBeenCalledTimes(6) // NOSONAR S109 — 2 configs × 3 variants each
+    expect(Object.keys(map._activeSymbolImageMap)).toHaveLength(2)
+    expect(Object.keys(map._selectedSymbolImageMap)).toHaveLength(2)
+  })
+})
+
+describe('addSymbolsToMap — null results and caching', () => {
+  it('does not call addImage when rasteriseSymbolImage returns null', async () => {
+    // getSymbolImageId (called twice — normal + active) needs a real symbolDef to produce imageIds,
+    // but rasteriseSymbolImage must get undefined from getSymbolDef so it returns null.
+    // The registry.get call order: [1] getSymbolImageId normal, [2] getSymbolImageId active,
+    // [3] rasteriseSymbolImage normal, [4] rasteriseSymbolImage active, [5] rasteriseSymbolImage selected.
+    const pinDef = symbolRegistry.get('pin')
+    const getSpy = jest.spyOn(symbolRegistry, 'get')
+      .mockReturnValueOnce(pinDef)
+      .mockReturnValueOnce(pinDef)
+      .mockReturnValueOnce(undefined)
+      .mockReturnValueOnce(undefined)
+      .mockReturnValueOnce(undefined)
+    const map = makeMap()
+    await addSymbolsToMap(map, [{ symbol: 'pin' }], mapStyle, symbolRegistry)
+    expect(map.addImage).not.toHaveBeenCalled()
+    getSpy.mockRestore()
+  })
+
+  it('skips config when symbolDef cannot be resolved', async () => {
+    const map = makeMap()
+    await addSymbolsToMap(map, [{ symbol: 'no-such-symbol' }], mapStyle, symbolRegistry)
+    expect(map.addImage).not.toHaveBeenCalled()
+    expect(map._activeSymbolImageMap).toEqual({})
+    expect(map._selectedSymbolImageMap).toEqual({})
+  })
+
+  it('reuses cached imageData when called again with the same pixelRatio', async () => {
+    // Use an unusual ratio so this test owns its cache entries
+    const uniqueRatio = 7
+
+    const map1 = makeMap()
+    const getContextCallsBefore = HTMLCanvasElement.prototype.getContext.mock.calls.length
+    await addSymbolsToMap(map1, [{ symbol: 'pin' }], mapStyle, symbolRegistry, uniqueRatio)
+    const getContextCallsAfterFirst = HTMLCanvasElement.prototype.getContext.mock.calls.length
+    // Rasterisation ran — canvas was used
+    expect(getContextCallsAfterFirst).toBeGreaterThan(getContextCallsBefore)
+
+    // Second call with a fresh map (hasImage → false) but same ratio → cache hit
+    const map2 = makeMap()
+    await addSymbolsToMap(map2, [{ symbol: 'pin' }], mapStyle, symbolRegistry, uniqueRatio)
+    const getContextCallsAfterSecond = HTMLCanvasElement.prototype.getContext.mock.calls.length
+    // No new canvas — rasterisation was skipped via cache
+    expect(getContextCallsAfterSecond).toBe(getContextCallsAfterFirst)
+    // addImage still called because map2 has no pre-registered images
+    expect(map2.addImage).toHaveBeenCalledTimes(3) // NOSONAR S109 — normal, active, selected
+  })
+})

src/services/symbolRegistry.js

@@ -197,52 +197,6 @@ export const symbolRegistry = {
     return undefined
   },
 
-  /**
- * Register normal, active (both rings) and selected (black ring) symbol images.
- * Skips images that are already registered (safe to call on style change).
- * Updates `map._activeSymbolImageMap` (normal→active) and `map._selectedSymbolImageMap` (normal→selected).
- *
- * Callers are responsible for resolving sublayers before calling this function
- * (see `getSymbolConfigs` in the datasets plugin adapter).
- *
- * @param {Object} map - MapLibre map instance
- * @param {Object[]} styleArray - Flat list of datasets/merged-sublayers that have a symbol config
- * @param {Object} mapStyle - Current map style config (provides id, selectedColor, haloColor)
- * @param {number} [pixelRatio=2] - Device pixel ratio × map size scale factor (computed by caller)
- * @returns {Promise<void>}
- */
-  async addSymbolsToMap (map, styleArray, mapStyle, pixelRatio = 2) {
-    if (!styleArray.length) {
-      return
-    }
-
-    map._activeSymbolImageMap = {}
-    map._selectedSymbolImageMap = {}
-
-    await Promise.all(styleArray.flatMap(config => {
-      const normalId = this.getSymbolImageId(config, mapStyle, false, pixelRatio)
-      const activeId = this.getSymbolImageId(config, mapStyle, true, pixelRatio)
-      if (normalId && activeId) {
-        map._activeSymbolImageMap[normalId] = activeId
-      }
-      return ['normal', 'active', 'selected'].map(async (variant) => {
-        const imageId = variant === 'active' ? activeId : normalId
-        if (variant !== 'selected' && (!imageId || map.hasImage(imageId))) {
-          return
-        }
-        const result = await this.rasteriseSymbolImage(config, mapStyle, variant, pixelRatio)
-        if (result) {
-          if (variant === 'selected' && normalId) {
-            map._selectedSymbolImageMap[normalId] = result.imageId
-          }
-          if (!map.hasImage(result.imageId)) {
-            map.addImage(result.imageId, result.imageData, { pixelRatio })
-          }
-        }
-      })
-    }))
-  },
-
   /**
  * Rasterise one variant of a symbol to ImageData for use as a MapLibre image.
  * Results are cached by imageId so identical symbols are only rendered once.