Fix `insertBefore` and `replace` relying on object key order

src/JSONEditor.svelte

@@ -33,7 +33,7 @@
   import jump from './assets/jump.js/src/jump.js'
   import { syncState } from './utils/syncState.js'
   import { isObject } from './utils/typeUtils.js'
-  import { patchProps } from './utils/updateProps.js'
+  import { getNextKeys, patchProps } from './utils/updateProps.js'
 
   let divContents
 
@@ -89,16 +89,14 @@
   export function patch(operations) {
     const prevState = state
 
+    console.log('operations', operations)
+
     const documentPatchResult = immutableJSONPatch(doc, operations)
     const statePatchResult = immutableJSONPatch(state, operations)
     // TODO: only apply operations to state for relevant operations: move, copy, delete? Figure out
 
     doc = documentPatchResult.json
-    state = statePatchResult.json
+    state = patchProps(statePatchResult.json, operations)
-
-    // if a property is renamed (move operation), rename it in the object's props
-    // so it maintains its identity and hence its index
-    state = patchProps(state, operations)
 
     history.add({
       undo: documentPatchResult.revert,
@@ -151,7 +149,11 @@
       console.log('paste', { clipboard, selection })
 
       if (selection.beforePath) {
-        const operations = insertBefore(doc, selection.beforePath, clipboard)
+        const parentPath = initial(selection.beforePath)
+        const beforeKey = last(selection.beforePath)
+        const props = getIn(state, parentPath.concat(STATE_PROPS))
+        const nextKeys = getNextKeys(props, parentPath, beforeKey, true)
+        const operations = insertBefore(doc, selection.beforePath, clipboard, nextKeys)
         console.log('patch', operations)
         handlePatch(operations)
 
@@ -163,7 +165,12 @@
 
         // FIXME: must adjust STATE_PROPS of the object where we inserted the clipboard
       } else if (selection.paths) {
-        const operations = replace(doc, selection.paths, clipboard)
+        const lastPath = last(selection.paths) // FIXME: here we assume selection.paths is sorted correctly
+        const parentPath = initial(lastPath)
+        const beforeKey = last(lastPath)
+        const props = getIn(state, parentPath.concat(STATE_PROPS))
+        const nextKeys = getNextKeys(props, parentPath, beforeKey, true)
+        const operations = replace(doc, selection.paths, clipboard, nextKeys)
         console.log('patch', operations)
         handlePatch(operations)
 

src/JSONNode.svelte

@@ -19,6 +19,7 @@
   import { findUniqueName } from './utils/stringUtils.js'
   import { isUrl, stringConvert, valueType } from './utils/typeUtils'
   import { compileJSONPointer } from './utils/jsonPointer'
+  import { getNextKeys } from './utils/updateProps.js'
 
   export let key = undefined // only applicable for object properties
   export let value
@@ -109,11 +110,7 @@
 
   function handleUpdateKey (oldKey, newKey) {
     const newKeyUnique = findUniqueName(newKey, value)
-
+    const nextKeys = getNextKeys(props, path, key, false)
-    const index = props.findIndex(prop => prop.key === oldKey)
-    const nextKeys = (index !== -1)
-      ? props.slice(index + 1).map(prop => prop.key)
-      : []
 
     onPatch(rename(path, oldKey, newKeyUnique, nextKeys))
   }

src/actions.js

@@ -15,9 +15,12 @@ import { findUniqueName } from './utils/stringUtils'
  * @param {JSON} json
  * @param {Path} path
  * @param {Array.<{key?: string, value: JSON}>} values
+ * @param {string[]} nextKeys   A list with all keys *after* the renamed key,
+ *                              these keys will be moved down, so the renamed
+ *                              key will maintain it's position above these keys
  * @return {Array}
  */
-export function insertBefore (json, path, values) {  // TODO: find a better name and define datastructure for values
+export function insertBefore (json, path, values, nextKeys) {  // TODO: find a better name and define datastructure for values
   const parentPath = initial(path)
   const parent = getIn(json, parentPath)
 
@@ -30,34 +33,20 @@ export function insertBefore (json, path, values) {  // TODO: find a better name
     }))
   }
   else { // 'object'
-    const addActions = values.map(entry => {
+    return [
-      const newProp = findUniqueName(entry.key, parent)
+      // insert new values
-      return {
+      ...values.map(entry => {
-        op: 'add',
+        const newProp = findUniqueName(entry.key, parent)
-        path: compileJSONPointer(parentPath.concat(newProp)),
+        return {
-        value: entry.value
+          op: 'add',
-      }
+          path: compileJSONPointer(parentPath.concat(newProp)),
-    })
+          value: entry.value
+        }
+      }),
 
-    // we move all lower down properties to the same parent again,
+      // move all lower down keys so the inserted key will maintain it's position
-    // to force them to move under the inserted properties instead of the
+      ...nextKeys.map(key => moveDown(parentPath, key))
-    // new properties appearing at the bottom of the object
+    ]
-    const keys = Object.keys(parent)
-    const beforeKey = last(path)
-    const beforeIndex = keys.indexOf(beforeKey)
-    const keysToMove = (beforeIndex !== -1)
-      ? keys.slice(beforeIndex)
-      : []
-    const moveActions = keysToMove.map(key => {
-      const movePath = compileJSONPointer(parentPath.concat(key))
-      return {
-        op: 'move',
-        from: movePath,
-        path: movePath
-      }
-    })
-
-    return addActions.concat(moveActions)
   }
 }
 
@@ -105,9 +94,8 @@ export function append (json, path, values) {  // TODO: find a better name and d
  * @param {string} oldKey
  * @param {string} newKey
  * @param {string[]} nextKeys   A list with all keys *after* the renamed key,
- *                              used to maintain the position of the renamed key.
+ *                              these keys will be moved down, so the renamed
- *                              If not provided, the renamed key will be moved
+ *                              key will maintain it's position above these keys
- *                              to the bottom of the list with keys.
  * @returns {Array}
  */
 export function rename(parentPath, oldKey, newKey, nextKeys) {
@@ -120,13 +108,7 @@ export function rename(parentPath, oldKey, newKey, nextKeys) {
     },
 
     // move all lower down keys so the renamed key will maintain it's position
-    ...nextKeys.map(key => {
+    ...nextKeys.map(key => moveDown(parentPath, key))
-      return {
-        op: 'move',
-        from: compileJSONPointer(parentPath.concat(key)),
-        path: compileJSONPointer(parentPath.concat(key))
-      }
-    })
   ]
 }
 
@@ -140,9 +122,12 @@ export function rename(parentPath, oldKey, newKey, nextKeys) {
  * @param {JSON} json
  * @param {Path[]} paths
  * @param {Array.<{key?: string, value: JSON}>} values
+ * @param {string[]} nextKeys   A list with all keys *after* the renamed key,
+ *                              these keys will be moved down, so the renamed
+ *                              key will maintain it's position above these keys
  * @return {Array}
  */
-export function replace (json, paths, values) {  // TODO: find a better name and define datastructure for values
+export function replace (json, paths, values, nextKeys) {  // TODO: find a better name and define datastructure for values
   const firstPath = first(paths)
   const parentPath = initial(firstPath)
   const parent = getIn(json, parentPath)
@@ -161,35 +146,24 @@ export function replace (json, paths, values) {  // TODO: find a better name and
     return removeActions.concat(insertActions)
   }
   else { // parent is Object
-    const removeActions = removeAll(paths)
+    return [
-    const insertActions = values.map(entry => {
+      // remove operations
-      const newProp = findUniqueName(entry.key, parent)
+      ...removeAll(paths),
-      return {
-        op: 'add',
-        path: compileJSONPointer(parentPath.concat(newProp)),
-        value: entry.value
-      }
-    })
 
-    // we move all lower down properties to the same parent again,
+      // insert operations
-    // to force them to move under the inserted properties instead of the
+      ...values.map(entry => {
-    // new properties appearing at the bottom of the object
+        const newProp = findUniqueName(entry.key, parent)
-    const keys = Object.keys(parent)
+        return {
-    const beforeKey = last(firstPath)
+          op: 'add',
-    const beforeIndex = keys.indexOf(beforeKey)
+          path: compileJSONPointer(parentPath.concat(newProp)),
-    const keysToMove = (beforeIndex !== -1)
+          value: entry.value
-      ? keys.slice(beforeIndex)
+        }
-      : []
+      }),
-    const moveActions = keysToMove.map(key => {
-      const movePath = compileJSONPointer(parentPath.concat(key))
-      return {
-        op: 'move',
-        from: movePath,
-        path: movePath
-      }
-    })
 
-    return removeActions.concat(insertActions, moveActions)
+      // move down operations
+      // move all lower down keys so the renamed key will maintain it's position
+      ...nextKeys.map(key => moveDown(parentPath, key))
+    ]
   }
 }
 
@@ -218,3 +192,13 @@ export function removeAll (paths) {
       }))
       .reverse() // reverse is needed for arrays: delete the last index first
 }
+
+// helper function to move a key down in an object,
+// so another key can get positioned before the moved down keys
+function moveDown(parentPath, key) {
+  return {
+    op: 'move',
+    from: compileJSONPointer(parentPath.concat(key)),
+    path: compileJSONPointer(parentPath.concat(key))
+  }
+}

src/utils/updateProps.js

@@ -49,18 +49,26 @@ export function patchProps (state, operations) {
         if (props) {
           const oldKey = last(pathFrom)
           const newKey = last(pathTo)
-          const index = props.findIndex(item => item.key === oldKey)
+          const oldIndex = props.findIndex(item => item.key === oldKey)
 
-          if (index !== -1) {
+          if (oldIndex !== -1) {
             if (oldKey !== newKey) {
-              // A property is renamed. Rename it in the object's props
+              // A property is renamed.
-              // so it maintains its identity and hence its index
+
-              updatedState = setIn(updatedState, parentPath.concat([STATE_PROPS, index, 'key']), newKey)
+              // in case the new key shadows an existing key, remove the existing key
+              const newIndex = props.findIndex(item => item.key === newKey)
+              if (newIndex !== -1) {
+                const updatedProps = deleteIn(props, [newIndex])
+                updatedState = setIn(updatedState, parentPath.concat([STATE_PROPS]), updatedProps)
+              }
+
+              // Rename the key in the object's props so it maintains its identity and hence its index
+              updatedState = setIn(updatedState, parentPath.concat([STATE_PROPS, oldIndex, 'key']), newKey)
             } else {
               // operation.from and operation.path are the same:
               // property is moved but stays the same -> move it to the end of the props
-              const oldProp = props[index]
+              const oldProp = props[oldIndex]
-              const updatedProps = insertAt(deleteIn(props, [index]), [props.length - 1], oldProp)
+              const updatedProps = insertAt(deleteIn(props, [oldIndex]), [props.length - 1], oldProp)
 
               updatedState = setIn(updatedState, parentPath.concat([STATE_PROPS]), updatedProps)
             }
@@ -74,10 +82,9 @@ export function patchProps (state, operations) {
       const parentPath = initial(path)
       const props = getIn(updatedState, parentPath.concat(STATE_PROPS))
       if (props) {
-        const key = last(path)
         const newProp = {
-          key,
+          id: uniqueId(),
-          id: uniqueId()
+          key: last(path)
         }
         const updatedProps = insertAt(props, [props.length], newProp)
 
@@ -88,3 +95,14 @@ export function patchProps (state, operations) {
 
   return updatedState
 }
+
+export function getNextKeys(props, parentPath, key, includeKey = false) {
+  if (props) {
+    const index = props.findIndex(prop => prop.key === key)
+    if (index !== -1) {
+      return props.slice(index + (includeKey ? 0 : 1)).map(prop => prop.key)
+    }
+  }
+
+  return []
+}