Geomap: use name as UID (#41668)

packages/grafana-data/src/geo/layer.ts

@@ -49,7 +49,7 @@ export interface FrameGeometrySource {
  */
 export interface MapLayerOptions<TConfig = any> {
   type: string;
-  name?: string; // configured unique display name
+  name: string; // configured unique display name
 
   // Custom options depending on the type
   config?: TConfig;

public/app/plugins/panel/geomap/GeomapPanel.tsx

@@ -1,6 +1,6 @@
 import React, { Component, ReactNode } from 'react';
 import { DEFAULT_BASEMAP_CONFIG, geomapLayerRegistry } from './layers/registry';
-import { Map, MapBrowserEvent, View } from 'ol';
+import { Map as OpenLayersMap, MapBrowserEvent, View } from 'ol';
 import Attribution from 'ol/control/Attribution';
 import Zoom from 'ol/control/Zoom';
 import ScaleLine from 'ol/control/ScaleLine';
@@ -46,13 +46,13 @@ interface State extends OverlayProps {
 export interface GeomapLayerActions {
   selectLayer: (uid: string) => void;
   deleteLayer: (uid: string) => void;
-  updateLayer: (uid: string, updatedLayer: MapLayerState<any>) => void;
   addlayer: (type: string) => void;
   reorder: (src: number, dst: number) => void;
+  canRename: (v: string) => boolean;
 }
 
 export interface GeomapInstanceState {
-  map?: Map;
+  map?: OpenLayersMap;
   layers: MapLayerState[];
   selected: number;
   actions: GeomapLayerActions;
@@ -65,15 +65,15 @@ export class GeomapPanel extends Component<Props, State> {
 
   globalCSS = getGlobalStyles(config.theme2);
 
-  counter = 0;
   mouseWheelZoom?: MouseWheelZoom;
   style = getStyles(config.theme);
   hoverPayload: GeomapHoverPayload = { point: {}, pageX: -1, pageY: -1 };
   readonly hoverEvent = new DataHoverEvent(this.hoverPayload);
 
-  map?: Map;
+  map?: OpenLayersMap;
   mapDiv?: HTMLDivElement;
   layers: MapLayerState[] = [];
+  readonly byName = new Map<string, MapLayerState>();
 
   constructor(props: Props) {
     super(props);
@@ -109,6 +109,7 @@ export class GeomapPanel extends Component<Props, State> {
     return true; // always?
   }
 
+  /** This funciton will actually update the JSON model */
   private doOptionsUpdate(selected: number) {
     const { options, onOptionsChange } = this.props;
     const layers = this.layers;
@@ -129,9 +130,20 @@ export class GeomapPanel extends Component<Props, State> {
     }
   }
 
+  getNextLayerName = () => {
+    let idx = this.layers.length; // since basemap is 0, this looks right
+    while (true && idx < 100) {
+      const name = `Layer ${idx++}`;
+      if (!this.byName.has(name)) {
+        return name;
+      }
+    }
+    return `Layer ${Date.now()}`;
+  };
+
   actions: GeomapLayerActions = {
     selectLayer: (uid: string) => {
-      const selected = this.layers.findIndex((v) => v.UID === uid);
+      const selected = this.layers.findIndex((v) => v.options.name === uid);
       if (this.panelContext.onInstanceStateChange) {
         this.panelContext.onInstanceStateChange({
           map: this.map,
@@ -141,10 +153,13 @@ export class GeomapPanel extends Component<Props, State> {
         });
       }
     },
+    canRename: (v: string) => {
+      return !this.byName.has(v);
+    },
     deleteLayer: (uid: string) => {
       const layers: MapLayerState[] = [];
       for (const lyr of this.layers) {
-        if (lyr.UID === uid) {
+        if (lyr.options.name === uid) {
           this.map?.removeLayer(lyr.layer);
         } else {
           layers.push(lyr);
@@ -153,21 +168,6 @@ export class GeomapPanel extends Component<Props, State> {
       this.layers = layers;
       this.doOptionsUpdate(0);
     },
-    updateLayer: (uid: string, updatedLayer: MapLayerState<any>) => {
-      const selected = this.layers.findIndex((v) => v.UID === uid);
-      const layers: MapLayerState[] = [];
-      for (const lyr of this.layers) {
-        if (lyr.UID === uid) {
-          this.map?.removeLayer(lyr.layer);
-          this.map?.addLayer(updatedLayer.layer);
-          layers.push(updatedLayer);
-        } else {
-          layers.push(lyr);
-        }
-      }
-      this.layers = layers;
-      this.doOptionsUpdate(selected);
-    },
     addlayer: (type: string) => {
       const item = geomapLayerRegistry.getIfExists(type);
       if (!item) {
@@ -177,6 +177,7 @@ export class GeomapPanel extends Component<Props, State> {
         this.map!,
         {
           type: item.id,
+          name: this.getNextLayerName(),
           config: cloneDeep(item.defaultOptions),
         },
         false
@@ -195,6 +196,11 @@ export class GeomapPanel extends Component<Props, State> {
       this.layers = result;
 
       this.doOptionsUpdate(endIndex);
+
+      // Add the layers in the right order
+      const group = this.map?.getLayers()!;
+      group.clear();
+      this.layers.forEach((v) => group.push(v.layer));
     },
   };
 
@@ -236,12 +242,12 @@ export class GeomapPanel extends Component<Props, State> {
     }
 
     if (!div) {
-      this.map = (undefined as unknown) as Map;
+      this.map = (undefined as unknown) as OpenLayersMap;
       return;
     }
     const { options } = this.props;
 
-    const map = (this.map = new Map({
+    const map = (this.map = new OpenLayersMap({
       view: this.initMapView(options.view),
       pixelRatio: 1, // or zoom?
       layers: [], // loaded explicitly below
@@ -252,6 +258,7 @@ export class GeomapPanel extends Component<Props, State> {
       }),
     }));
 
+    this.byName.clear();
     const layers: MapLayerState[] = [];
     try {
       layers.push(await this.initLayer(map, options.basemap ?? DEFAULT_BASEMAP_CONFIG, true));
@@ -354,28 +361,46 @@ export class GeomapPanel extends Component<Props, State> {
     if (!this.map) {
       return false;
     }
-    const selected = this.layers.findIndex((v) => v.UID === uid);
-    if (selected < 0) {
+    const current = this.byName.get(uid);
+    if (!current) {
       return false;
     }
-    const layers = this.layers.slice(0);
-    try {
-      let found = false;
-      const current = this.layers[selected];
-      const info = await this.initLayer(this.map, newOptions, current.isBasemap);
-      const group = this.map?.getLayers()!;
-      for (let i = 0; i < group?.getLength(); i++) {
-        if (group.item(i) === current.layer) {
-          found = true;
-          group.setAt(i, info.layer);
-          break;
-        }
+
+    let layerIndex = -1;
+    const group = this.map?.getLayers()!;
+    for (let i = 0; i < group?.getLength(); i++) {
+      if (group.item(i) === current.layer) {
+        layerIndex = i;
+        break;
       }
-      if (!found) {
-        console.warn('ERROR not found', uid);
+    }
+
+    // Special handling for rename
+    if (newOptions.name !== uid) {
+      if (!newOptions.name) {
+        newOptions.name = uid;
+      } else if (this.byName.has(newOptions.name)) {
         return false;
       }
-      layers[selected] = info;
+      console.log('Layer name changed', uid, '>>>', newOptions.name);
+      this.byName.delete(uid);
+
+      uid = newOptions.name;
+      this.byName.set(uid, current);
+    }
+
+    // Type changed -- requires full re-initalization
+    if (current.options.type !== newOptions.type) {
+      // full init
+    } else {
+      // just update options
+    }
+
+    const layers = this.layers.slice(0);
+    try {
+      const info = await this.initLayer(this.map, newOptions, current.isBasemap);
+      layers[layerIndex] = info;
+      group.setAt(layerIndex, info.layer);
 
       // initialize with new data
       if (info.handler.update) {
@@ -385,28 +410,13 @@ export class GeomapPanel extends Component<Props, State> {
       console.warn('ERROR', err);
       return false;
     }
-    // TODO
-    // validate names, basemap etc
 
     this.layers = layers;
-    this.doOptionsUpdate(selected);
-
+    this.doOptionsUpdate(layerIndex);
     return true;
   };
 
-  private generateLayerName = (): string => {
-    let newLayerName = `Layer ${this.counter}`;
-
-    for (const otherLayer of this.layers) {
-      if (newLayerName === otherLayer.options.name) {
-        newLayerName += '-1';
-      }
-    }
-
-    return newLayerName;
-  };
-
-  async initLayer(map: Map, options: MapLayerOptions, isBasemap?: boolean): Promise<MapLayerState> {
+  async initLayer(map: OpenLayersMap, options: MapLayerOptions, isBasemap?: boolean): Promise<MapLayerState> {
     if (isBasemap && (!options?.type || config.geomapDisableCustomBaseLayer)) {
       options = DEFAULT_BASEMAP_CONFIG;
     }
@@ -415,6 +425,7 @@ export class GeomapPanel extends Component<Props, State> {
     if (!options?.type) {
       options = {
         type: MARKERS_LAYER_ID,
+        name: this.getNextLayerName(),
         config: {},
       };
     }
@@ -427,32 +438,28 @@ export class GeomapPanel extends Component<Props, State> {
     const handler = await item.create(map, options, config.theme2);
     const layer = handler.init();
 
-    // const key = layer.on('change', () => {
-    //   const state = layer.getLayerState();
-    //   console.log('LAYER', key, state);
-    // });
-
     if (handler.update) {
       handler.update(this.props.data);
     }
 
     if (!options.name) {
-      options.name = this.generateLayerName();
+      options.name = this.getNextLayerName();
     }
-    const UID = `lyr-${this.counter++}`;
-
-    return {
-      UID,
+    const UID = options.name;
+    const state = {
+      UID, // unique name when added to the map (it may change and will need special handling)
       isBasemap,
       options,
       layer,
       handler,
 
       // Used by the editors
-      onChange: (cfg) => {
+      onChange: (cfg: MapLayerOptions) => {
         this.updateLayer(UID, cfg);
       },
     };
+    this.byName.set(UID, state);
+    return state;
   }
 
   initMapView(config: MapViewConfig): View {

public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.test.tsx

@@ -11,7 +11,7 @@ describe('LayerHeader', () => {
     fireEvent.change(input, { target: { value: 'new name' } });
     fireEvent.blur(input);
 
-    expect((scenario.props.onChange as any).mock.calls[0][0].options.name).toBe('new name');
+    expect((scenario.props.onChange as any).mock.calls[0][0].name).toBe('new name');
   });
 
   it('Show error when empty name is specified', async () => {
@@ -37,21 +37,12 @@ describe('LayerHeader', () => {
   });
 
   function renderScenario(overrides: Partial<LayerHeaderProps>) {
-    const props: any = {
-      layer: {
-        UID: '1',
-        options: { name: 'Layer 1' },
+    const props: LayerHeaderProps = {
+      layer: { name: 'Layer 1', type: '?' },
+      canRename: (v: string) => {
+        const names = new Set(['Layer 1', 'Layer 2']);
+        return !names.has(v);
       },
-      layers: [
-        {
-          UID: '1',
-          options: { name: 'Layer 1' },
-        },
-        {
-          UID: '2',
-          options: { name: 'Layer 2' },
-        },
-      ],
       onChange: jest.fn(),
     };
 

public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.tsx

@@ -1,17 +1,15 @@
 import React, { useState } from 'react';
 import { css, cx } from '@emotion/css';
 import { Icon, Input, FieldValidationMessage, useStyles } from '@grafana/ui';
-import { GrafanaTheme } from '@grafana/data';
-
-import { MapLayerState } from '../../types';
+import { GrafanaTheme, MapLayerOptions } from '@grafana/data';
 
 export interface LayerHeaderProps {
-  layer: MapLayerState<any>;
-  layers: Array<MapLayerState<any>>;
-  onChange: (layer: MapLayerState<any>) => void;
+  layer: MapLayerOptions<any>;
+  canRename: (v: string) => boolean;
+  onChange: (layer: MapLayerOptions<any>) => void;
 }
 
-export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => {
+export const LayerHeader = ({ layer, canRename, onChange }: LayerHeaderProps) => {
   const styles = useStyles(getStyles);
 
   const [isEditing, setIsEditing] = useState<boolean>(false);
@@ -29,10 +27,10 @@ export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => {
       return;
     }
 
-    if (layer.options.name !== newName) {
+    if (layer.name !== newName) {
       onChange({
         ...layer,
-        options: { ...layer.options, name: newName },
+        name: newName,
       });
     }
   };
@@ -45,11 +43,9 @@ export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => {
       return;
     }
 
-    for (const otherLayer of layers) {
-      if (otherLayer.UID !== layer.UID && newName === otherLayer.options.name) {
-        setValidationError('Layer name already exists');
-        return;
-      }
+    if (!canRename(newName)) {
+      setValidationError('Layer name already exists');
+      return;
     }
 
     if (validationError) {
@@ -81,7 +77,7 @@ export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => {
             onClick={onEditLayer}
             data-testid="layer-name-div"
           >
-            <span className={styles.layerName}>{layer.options.name}</span>
+            <span className={styles.layerName}>{layer.name}</span>
             <Icon name="pen" className={styles.layerEditIcon} size="sm" />
           </button>
         )}
@@ -90,7 +86,7 @@ export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => {
           <>
             <Input
               type="text"
-              defaultValue={layer.options.name}
+              defaultValue={layer.name}
               onBlur={onEditLayerBlur}
               autoFocus
               onKeyDown={onKeyDown}

public/app/plugins/panel/geomap/editor/LayersEditor/LayerList.tsx

@@ -23,10 +23,6 @@ export const LayerList = ({ layers, onDragEnd, selected, actions }: LayerListPro
     return sel ? `${style.row} ${style.sel}` : style.row;
   };
 
-  const onLayerNameChange = (layer: MapLayerState<any>) => {
-    actions.updateLayer(layer.UID, layer);
-  };
-
   return (
     <DragDropContext onDragEnd={onDragEnd}>
       <Droppable droppableId="droppable">
@@ -37,24 +33,29 @@ export const LayerList = ({ layers, onDragEnd, selected, actions }: LayerListPro
               const rows: any = [];
               for (let i = layers.length - 1; i > 0; i--) {
                 const element = layers[i];
+                const uid = element.options.name;
                 rows.push(
-                  <Draggable key={element.UID} draggableId={element.UID} index={rows.length}>
+                  <Draggable key={uid} draggableId={uid} index={rows.length}>
                     {(provided, snapshot) => (
                       <div
                         className={getRowStyle(i === selected)}
                         ref={provided.innerRef}
                         {...provided.draggableProps}
                         {...provided.dragHandleProps}
-                        onMouseDown={() => actions!.selectLayer(element.UID)}
+                        onMouseDown={() => actions!.selectLayer(uid)}
                       >
-                        <LayerHeader layer={{ ...element }} layers={layers} onChange={onLayerNameChange} />
+                        <LayerHeader
+                          layer={element.options}
+                          canRename={actions.canRename}
+                          onChange={element.onChange}
+                        />
                         <div className={style.textWrapper}>&nbsp; {element.options.type}</div>
 
                         <IconButton
                           name="trash-alt"
                           title={'remove'}
                           className={cx(style.actionIcon, style.dragIcon)}
-                          onClick={() => actions.deleteLayer(element.UID)}
+                          onClick={() => actions.deleteLayer(uid)}
                           surface="header"
                         />
                         {layers.length > 2 && (

public/app/plugins/panel/geomap/layers/data/markersLayer.tsx

@@ -41,6 +41,7 @@ export const MARKERS_LAYER_ID = 'markers';
 // Used by default when nothing is configured
 export const defaultMarkersConfig: MapLayerOptions<MarkersConfig> = {
   type: MARKERS_LAYER_ID,
+  name: '', // will get replaced
   config: defaultOptions,
   location: {
     mode: FrameGeometrySourceMode.Auto,

public/app/plugins/panel/geomap/layers/registry.ts

@@ -7,6 +7,7 @@ import { dataLayers } from './data';
 
 export const DEFAULT_BASEMAP_CONFIG: MapLayerOptions = {
   type: 'default',
+  name: '', // will get filled in with a non-empty name
   config: {},
 };
 

public/app/plugins/panel/geomap/migrations.test.ts

@@ -47,6 +47,7 @@ describe('Worldmap Migrations', () => {
         },
         "options": Object {
           "basemap": Object {
+            "name": "Basemap",
             "type": "default",
           },
           "controls": Object {

public/app/plugins/panel/geomap/migrations.ts

@@ -40,6 +40,7 @@ export function worldmapToGeomapOptions(angular: any): { fieldConfig: FieldConfi
     },
     basemap: {
       type: 'default', // was carto
+      name: 'Basemap',
     },
     layers: [
       // TODO? depends on current configs

public/app/plugins/panel/geomap/types.ts

@@ -71,7 +71,6 @@ export interface GazetteerPathEditorConfigSettings {
 // Runtime model
 //-------------------
 export interface MapLayerState<TConfig = any> {
-  UID: string; // value changes with each initialization
   options: MapLayerOptions<TConfig>;
   handler: MapLayerHandler;
   layer: BaseLayer; // the openlayers instance