Merge pull request #625 from orangemug/fix/invalid-style-with-duplicate-layer-ids

Fix UI when loading invalid style with duplicate layer ids

package.json

@@ -102,6 +102,11 @@
         "experimentalObjectRestSpread": true,
         "jsx": true
       }
+    },
+    "settings": {
+      "react": {
+        "version": "detect"
+      }
     }
   },
   "devDependencies": {

src/components/App.jsx

@@ -36,6 +36,7 @@ import tokens from '../config/tokens.json'
 import isEqual from 'lodash.isequal'
 import Debug from '../libs/debug'
 import queryUtil from '../libs/query-util'
+import {formatLayerId} from './util/format';
 
 import MapboxGl from 'mapbox-gl'
 
@@ -325,7 +326,42 @@ export default class App extends React.Component {
     };
 
     const errors = validate(newStyle, latest) || [];
-    const mappedErrors = errors.map(error => {
+
+    // The validate function doesn't give us errors for duplicate error with
+    // empty string for layer.id, manually deal with that here.
+    const layerErrors = [];
+    if (newStyle && newStyle.layers) {
+      const foundLayers = new Map();
+      newStyle.layers.forEach((layer, index) => {
+        if (layer.id === "" && foundLayers.has(layer.id)) {
+          const message = `Duplicate layer: ${formatLayerId(layer.id)}`;
+          const error = new Error(
+            `layers[${index}]: duplicate layer id [empty_string], previously used`
+          );
+          layerErrors.push(error);
+        }
+        foundLayers.set(layer.id, true);
+      });
+    }
+
+    const mappedErrors = layerErrors.concat(errors).map(error => {
+      const dupMatch = error.message.match(/layers\[(\d+)\]: (duplicate layer id "?(.*)"?, previously used)/);
+      if (dupMatch) {
+        const [matchStr, index, message] = dupMatch;
+        return {
+          message: error.message,
+          parsed: {
+            type: "layer",
+            data: {
+              index,
+              key: "id",
+              message,
+            }
+          }
+        }
+      }
+
+      // duplicate layer id
       const layerMatch = error.message.match(/layers\[(\d+)\]\.(?:(\S+)\.)?(\S+): (.*)/);
       if (layerMatch) {
         const [matchStr, index, group, property, message] = layerMatch;
@@ -347,7 +383,7 @@ export default class App extends React.Component {
           message: error.message,
         };
       }
-    })
+    });
 
     let dirtyMapStyle = undefined;
     if (errors.length > 0) {
@@ -437,56 +473,50 @@ export default class App extends React.Component {
     this.onStyleChanged(changedStyle)
   }
 
-  onLayerDestroy = (layerId) => {
+  onLayerDestroy = (index) => {
     let layers = this.state.mapStyle.layers;
     const remainingLayers = layers.slice(0);
-    const idx = style.indexOfLayer(remainingLayers, layerId)
-    remainingLayers.splice(idx, 1);
+    remainingLayers.splice(index, 1);
     this.onLayersChange(remainingLayers);
   }
 
-  onLayerCopy = (layerId) => {
+  onLayerCopy = (index) => {
     let layers = this.state.mapStyle.layers;
     const changedLayers = layers.slice(0)
-    const idx = style.indexOfLayer(changedLayers, layerId)
 
-    const clonedLayer = cloneDeep(changedLayers[idx])
+    const clonedLayer = cloneDeep(changedLayers[index])
     clonedLayer.id = clonedLayer.id + "-copy"
-    changedLayers.splice(idx, 0, clonedLayer)
+    changedLayers.splice(index, 0, clonedLayer)
     this.onLayersChange(changedLayers)
   }
 
-  onLayerVisibilityToggle = (layerId) => {
+  onLayerVisibilityToggle = (index) => {
     let layers = this.state.mapStyle.layers;
     const changedLayers = layers.slice(0)
-    const idx = style.indexOfLayer(changedLayers, layerId)
 
-    const layer = { ...changedLayers[idx] }
+    const layer = { ...changedLayers[index] }
     const changedLayout = 'layout' in layer ? {...layer.layout} : {}
     changedLayout.visibility = changedLayout.visibility === 'none' ? 'visible' : 'none'
 
     layer.layout = changedLayout
-    changedLayers[idx] = layer
+    changedLayers[index] = layer
     this.onLayersChange(changedLayers)
   }
 
 
-  onLayerIdChange = (oldId, newId) => {
+  onLayerIdChange = (index, oldId, newId) => {
     const changedLayers = this.state.mapStyle.layers.slice(0)
-    const idx = style.indexOfLayer(changedLayers, oldId)
-
-    changedLayers[idx] = {
-      ...changedLayers[idx],
+    changedLayers[index] = {
+      ...changedLayers[index],
       id: newId
     }
 
     this.onLayersChange(changedLayers)
   }
 
-  onLayerChanged = (layer) => {
+  onLayerChanged = (index, layer) => {
     const changedLayers = this.state.mapStyle.layers.slice(0)
-    const idx = style.indexOfLayer(changedLayers, layer.id)
-    changedLayers[idx] = layer
+    changedLayers[index] = layer
 
     this.onLayersChange(changedLayers)
   }
@@ -645,9 +675,8 @@ export default class App extends React.Component {
     </div>
   }
 
-  onLayerSelect = (layerId) => {
-    const idx = style.indexOfLayer(this.state.mapStyle.layers, layerId)
-    this.setState({ selectedLayerIndex: idx })
+  onLayerSelect = (index) => {
+    this.setState({ selectedLayerIndex: index })
   }
 
   setModal(modalName, value) {
@@ -735,6 +764,7 @@ export default class App extends React.Component {
 
     const bottomPanel = (this.state.errors.length + this.state.infos.length) > 0 ? <MessagePanel
       currentLayer={selectedLayer}
+      selectedLayerIndex={this.state.selectedLayerIndex}
       onLayerSelect={this.onLayerSelect}
       mapStyle={this.state.mapStyle}
       errors={this.state.errors}

src/components/MessagePanel.jsx

@@ -1,5 +1,6 @@
 import React from 'react'
 import PropTypes from 'prop-types'
+import {formatLayerId} from './util/format';
 
 class MessagePanel extends React.Component {
   static propTypes = {
@@ -8,6 +9,7 @@ class MessagePanel extends React.Component {
     mapStyle: PropTypes.object,
     onLayerSelect: PropTypes.func,
     currentLayer: PropTypes.object,
+    selectedLayerIndex: PropTypes.number,
   }
 
   static defaultProps = {
@@ -15,6 +17,7 @@ class MessagePanel extends React.Component {
   }
 
   render() {
+    const {selectedLayerIndex} = this.props;
     const errors = this.props.errors.map((error, idx) => {
       let content;
       if (error.parsed && error.parsed.type === "layer") {
@@ -23,13 +26,13 @@ class MessagePanel extends React.Component {
         const layerId = mapStyle.layers[parsed.data.index].id;
         content = (
           <>
-            Layer <span>&apos;{layerId}&apos;</span>: {parsed.data.message}
-            {currentLayer.id !== layerId &&
+            Layer <span>{formatLayerId(layerId)}</span>: {parsed.data.message}
+            {selectedLayerIndex !== parsed.data.index &&
               <>
                 &nbsp;&mdash;&nbsp;
                 <button
                   className="maputnik-message-panel__switch-button"
-                  onClick={() => this.props.onLayerSelect(layerId)}
+                  onClick={() => this.props.onLayerSelect(parsed.data.index)}
                 >
                   switch to layer
                 </button>

src/components/inputs/StringInput.jsx

@@ -79,6 +79,11 @@ class StringInput extends React.Component {
           this.props.onChange(this.state.value);
         }
       },
+      onKeyDown: (e) => {
+        if (e.keyCode === 13) {
+          this.props.onChange(this.state.value);
+        }
+      },
       required: this.props.required,
     });
   }

src/components/layers/LayerEditor.jsx

@@ -19,6 +19,7 @@ import {MdMoreVert} from 'react-icons/md'
 
 import { changeType, changeProperty } from '../../libs/layer'
 import layout from '../../config/layout.json'
+import {formatLayerId} from '../util/format';
 
 
 function getLayoutForType (type) {
@@ -108,7 +109,10 @@ export default class LayerEditor extends React.Component {
   }
 
   changeProperty(group, property, newValue) {
-    this.props.onLayerChanged(changeProperty(this.props.layer, group, property, newValue))
+    this.props.onLayerChanged(
+      this.props.layerIndex,
+      changeProperty(this.props.layer, group, property, newValue)
+    )
   }
 
   onGroupToggle(groupTitle, active) {
@@ -152,13 +156,16 @@ export default class LayerEditor extends React.Component {
           value={this.props.layer.id}
           wdKey="layer-editor.layer-id"
           error={errorData.id}
-          onChange={newId => this.props.onLayerIdChange(this.props.layer.id, newId)}
+          onChange={newId => this.props.onLayerIdChange(this.props.layerIndex, this.props.layer.id, newId)}
         />
         <LayerTypeBlock
           disabled={true}
           error={errorData.type}
           value={this.props.layer.type}
-          onChange={newType => this.props.onLayerChanged(changeType(this.props.layer, newType))}
+          onChange={newType => this.props.onLayerChanged(
+            this.props.layerIndex,
+            changeType(this.props.layer, newType)
+          )}
         />
         {this.props.layer.type !== 'background' && <LayerSourceBlock
           error={errorData.sources}
@@ -210,7 +217,12 @@ export default class LayerEditor extends React.Component {
       />
       case 'jsoneditor': return <JSONEditor
         layer={this.props.layer}
-        onChange={this.props.onLayerChanged}
+        onChange={(layer) => {
+          this.props.onLayerChanged(
+            this.props.layerIndex,
+            layer
+          );
+        }}
       />
     }
   }
@@ -247,15 +259,15 @@ export default class LayerEditor extends React.Component {
     const items = {
       delete: {
         text: "Delete",
-        handler: () => this.props.onLayerDestroy(this.props.layer.id)
+        handler: () => this.props.onLayerDestroy(this.props.layerIndex)
       },
       duplicate: {
         text: "Duplicate",
-        handler: () => this.props.onLayerCopy(this.props.layer.id)
+        handler: () => this.props.onLayerCopy(this.props.layerIndex)
       },
       hide: {
         text: (layout.visibility === "none") ? "Show" : "Hide",
-        handler: () => this.props.onLayerVisibilityToggle(this.props.layer.id)
+        handler: () => this.props.onLayerVisibilityToggle(this.props.layerIndex)
       },
       moveLayerUp: {
         text: "Move layer up",
@@ -281,7 +293,7 @@ export default class LayerEditor extends React.Component {
       <header>
         <div className="layer-header">
           <h2 className="layer-header__title">
-            Layer: {this.props.layer.id}
+            Layer: {formatLayerId(this.props.layer.id)}
           </h2>
           <div className="layer-header__info">
             <Wrapper

src/components/layers/LayerIdBlock.jsx

@@ -10,11 +10,13 @@ class LayerIdBlock extends React.Component {
     value: PropTypes.string.isRequired,
     wdKey: PropTypes.string.isRequired,
     onChange: PropTypes.func.isRequired,
+    error: PropTypes.object.isRequired,
   }
 
   render() {
     return <InputBlock label={"ID"} fieldSpec={latest.layer.id}
       data-wd-key={this.props.wdKey}
+      error={this.props.error}
     >
       <StringInput
         value={this.props.value}

src/components/layers/LayerList.jsx

@@ -165,12 +165,15 @@ class LayerListContainer extends React.Component {
 
     const listItems = []
     let idx = 0
-    this.groupedLayers().forEach(layers => {
+    const layerIdCount = new Map();
+
+    const layersByGroup = this.groupedLayers();
+    layersByGroup.forEach(layers => {
       const groupPrefix = layerPrefix(layers[0].id)
       if(layers.length > 1) {
         const grp = <LayerListGroup
           data-wd-key={[groupPrefix, idx].join('-')}
-          key={[groupPrefix, idx].join('-')}
+          key={`group-${groupPrefix}`}
           title={groupPrefix}
           isActive={!this.isCollapsed(groupPrefix, idx) || idx === this.props.selectedLayerIndex}
           onActiveToggle={this.toggleLayerGroup.bind(this, groupPrefix, idx)}
@@ -189,6 +192,10 @@ class LayerListContainer extends React.Component {
           );
         });
 
+        layerIdCount.set(layer.id,
+          layerIdCount.has(layer.id) ? layerIdCount.get(layer.id) + 1 : 0
+        );
+        const key = `${layer.id}-${layerIdCount.get(layer.id)}`;
         const listItem = <LayerListItem
           className={classnames({
             'maputnik-layer-list-item-collapsed': layers.length > 1 && this.isCollapsed(groupPrefix, groupIdx) && idx !== this.props.selectedLayerIndex,
@@ -196,8 +203,9 @@ class LayerListContainer extends React.Component {
             'maputnik-layer-list-item--error': !!layerError
           })}
           index={idx}
-          key={layer.id}
+          key={key}
           layerId={layer.id}
+          layerIndex={idx}
           layerType={layer.type}
           visibility={(layer.layout || {}).visibility}
           isSelected={idx === this.props.selectedLayerIndex}

src/components/layers/LayerListItem.jsx

@@ -62,6 +62,7 @@ class IconAction extends React.Component {
 
 class LayerListItem extends React.Component {
   static propTypes = {
+    layerIndex: PropTypes.number.isRequired,
     layerId: PropTypes.string.isRequired,
     layerType: PropTypes.string.isRequired,
     isSelected: PropTypes.bool,
@@ -97,7 +98,7 @@ class LayerListItem extends React.Component {
 
     return <li
       key={this.props.layerId}
-      onClick={e => this.props.onLayerSelect(this.props.layerId)}
+      onClick={e => this.props.onLayerSelect(this.props.layerIndex)}
       data-wd-key={"layer-list-item:"+this.props.layerId}
       className={classnames({
         "maputnik-layer-list-item": true,
@@ -110,20 +111,20 @@ class LayerListItem extends React.Component {
           wdKey={"layer-list-item:"+this.props.layerId+":delete"}
           action={'delete'}
           classBlockName="delete"
-          onClick={e => this.props.onLayerDestroy(this.props.layerId)}
+          onClick={e => this.props.onLayerDestroy(this.props.layerIndex)}
         />
         <IconAction
           wdKey={"layer-list-item:"+this.props.layerId+":copy"}
           action={'duplicate'}
           classBlockName="duplicate"
-          onClick={e => this.props.onLayerCopy(this.props.layerId)}
+          onClick={e => this.props.onLayerCopy(this.props.layerIndex)}
         />
         <IconAction
           wdKey={"layer-list-item:"+this.props.layerId+":toggle-visibility"}
           action={visibilityAction}
           classBlockName="visibility"
           classBlockModifier={visibilityAction}
-          onClick={e => this.props.onLayerVisibilityToggle(this.props.layerId)}
+          onClick={e => this.props.onLayerVisibilityToggle(this.props.layerIndex)}
         />
     </li>
   }

src/components/util/format.js

@@ -0,0 +1,3 @@
+export function formatLayerId (id) {
+  return id === "" ? "[empty_string]" : `'${id}'`;
+}