From 466eaeb4f0f240a2165180cdaf007bb92eb5aecd Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Mon, 15 Nov 2021 11:18:08 -0800 Subject: [PATCH] Geomap: use name as UID (#41668) --- packages/grafana-data/src/geo/layer.ts | 2 +- .../app/plugins/panel/geomap/GeomapPanel.tsx | 145 +++++++++--------- .../editor/LayersEditor/LayerHeader.test.tsx | 21 +-- .../editor/LayersEditor/LayerHeader.tsx | 28 ++-- .../geomap/editor/LayersEditor/LayerList.tsx | 17 +- .../panel/geomap/layers/data/markersLayer.tsx | 1 + .../plugins/panel/geomap/layers/registry.ts | 1 + .../plugins/panel/geomap/migrations.test.ts | 1 + public/app/plugins/panel/geomap/migrations.ts | 1 + public/app/plugins/panel/geomap/types.ts | 1 - 10 files changed, 108 insertions(+), 110 deletions(-) diff --git a/packages/grafana-data/src/geo/layer.ts b/packages/grafana-data/src/geo/layer.ts index 4d25e3a622a..9909d62ed0f 100644 --- a/packages/grafana-data/src/geo/layer.ts +++ b/packages/grafana-data/src/geo/layer.ts @@ -49,7 +49,7 @@ export interface FrameGeometrySource { */ export interface MapLayerOptions { type: string; - name?: string; // configured unique display name + name: string; // configured unique display name // Custom options depending on the type config?: TConfig; diff --git a/public/app/plugins/panel/geomap/GeomapPanel.tsx b/public/app/plugins/panel/geomap/GeomapPanel.tsx index 46bde5c1c00..4df51e4d1cd 100644 --- a/public/app/plugins/panel/geomap/GeomapPanel.tsx +++ b/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) => 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 { 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(); constructor(props: Props) { super(props); @@ -109,6 +109,7 @@ export class GeomapPanel extends Component { 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 { } } + 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 { }); } }, + 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 { this.layers = layers; this.doOptionsUpdate(0); }, - updateLayer: (uid: string, updatedLayer: MapLayerState) => { - 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 { this.map!, { type: item.id, + name: this.getNextLayerName(), config: cloneDeep(item.defaultOptions), }, false @@ -195,6 +196,11 @@ export class GeomapPanel extends Component { 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 { } 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 { }), })); + 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 { 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 { 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 { + async initLayer(map: OpenLayersMap, options: MapLayerOptions, isBasemap?: boolean): Promise { if (isBasemap && (!options?.type || config.geomapDisableCustomBaseLayer)) { options = DEFAULT_BASEMAP_CONFIG; } @@ -415,6 +425,7 @@ export class GeomapPanel extends Component { if (!options?.type) { options = { type: MARKERS_LAYER_ID, + name: this.getNextLayerName(), config: {}, }; } @@ -427,32 +438,28 @@ export class GeomapPanel extends Component { 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 { diff --git a/public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.test.tsx b/public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.test.tsx index 9bb670e88cb..6528a82e4d0 100644 --- a/public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.test.tsx +++ b/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) { - 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(), }; diff --git a/public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.tsx b/public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.tsx index 5a321e7bba3..5994cf91056 100644 --- a/public/app/plugins/panel/geomap/editor/LayersEditor/LayerHeader.tsx +++ b/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; - layers: Array>; - onChange: (layer: MapLayerState) => void; + layer: MapLayerOptions; + canRename: (v: string) => boolean; + onChange: (layer: MapLayerOptions) => void; } -export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => { +export const LayerHeader = ({ layer, canRename, onChange }: LayerHeaderProps) => { const styles = useStyles(getStyles); const [isEditing, setIsEditing] = useState(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" > - {layer.options.name} + {layer.name} )} @@ -90,7 +86,7 @@ export const LayerHeader = ({ layer, layers, onChange }: LayerHeaderProps) => { <> ) => { - actions.updateLayer(layer.UID, layer); - }; - return ( @@ -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( - + {(provided, snapshot) => (
actions!.selectLayer(element.UID)} + onMouseDown={() => actions!.selectLayer(uid)} > - +
  {element.options.type}
actions.deleteLayer(element.UID)} + onClick={() => actions.deleteLayer(uid)} surface="header" /> {layers.length > 2 && ( diff --git a/public/app/plugins/panel/geomap/layers/data/markersLayer.tsx b/public/app/plugins/panel/geomap/layers/data/markersLayer.tsx index 12a403718d7..f5c0bbaa7b0 100644 --- a/public/app/plugins/panel/geomap/layers/data/markersLayer.tsx +++ b/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 = { type: MARKERS_LAYER_ID, + name: '', // will get replaced config: defaultOptions, location: { mode: FrameGeometrySourceMode.Auto, diff --git a/public/app/plugins/panel/geomap/layers/registry.ts b/public/app/plugins/panel/geomap/layers/registry.ts index 6dc22bd92b9..de99df94e5f 100644 --- a/public/app/plugins/panel/geomap/layers/registry.ts +++ b/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: {}, }; diff --git a/public/app/plugins/panel/geomap/migrations.test.ts b/public/app/plugins/panel/geomap/migrations.test.ts index ce6291708af..b9ecd98786e 100644 --- a/public/app/plugins/panel/geomap/migrations.test.ts +++ b/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 { diff --git a/public/app/plugins/panel/geomap/migrations.ts b/public/app/plugins/panel/geomap/migrations.ts index f3e7d93dc81..07a8a871cfa 100644 --- a/public/app/plugins/panel/geomap/migrations.ts +++ b/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 diff --git a/public/app/plugins/panel/geomap/types.ts b/public/app/plugins/panel/geomap/types.ts index 412989be952..765a6928a1d 100644 --- a/public/app/plugins/panel/geomap/types.ts +++ b/public/app/plugins/panel/geomap/types.ts @@ -71,7 +71,6 @@ export interface GazetteerPathEditorConfigSettings { // Runtime model //------------------- export interface MapLayerState { - UID: string; // value changes with each initialization options: MapLayerOptions; handler: MapLayerHandler; layer: BaseLayer; // the openlayers instance