From 0e5dabed897cd963698b8efb6d02327147f6cba5 Mon Sep 17 00:00:00 2001 From: Jos de Jong Date: Wed, 8 Jul 2020 14:41:38 +0200 Subject: [PATCH] Fix `insertBefore` and `replace` relying on object key order --- src/JSONEditor.svelte | 23 +++++--- src/JSONNode.svelte | 7 +-- src/actions.js | 116 +++++++++++++++++---------------------- src/utils/updateProps.js | 38 +++++++++---- 4 files changed, 95 insertions(+), 89 deletions(-) diff --git a/src/JSONEditor.svelte b/src/JSONEditor.svelte index 430b1f4..b818ad1 100644 --- a/src/JSONEditor.svelte +++ b/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 - - // 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) + state = patchProps(statePatchResult.json, 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) diff --git a/src/JSONNode.svelte b/src/JSONNode.svelte index 6fda45d..325d8e1 100644 --- a/src/JSONNode.svelte +++ b/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 index = props.findIndex(prop => prop.key === oldKey) - const nextKeys = (index !== -1) - ? props.slice(index + 1).map(prop => prop.key) - : [] + const nextKeys = getNextKeys(props, path, key, false) onPatch(rename(path, oldKey, newKeyUnique, nextKeys)) } diff --git a/src/actions.js b/src/actions.js index 76d643b..bb8bfb6 100644 --- a/src/actions.js +++ b/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 => { - const newProp = findUniqueName(entry.key, parent) - return { - op: 'add', - path: compileJSONPointer(parentPath.concat(newProp)), - value: entry.value - } - }) + return [ + // insert new values + ...values.map(entry => { + const newProp = findUniqueName(entry.key, parent) + return { + op: 'add', + path: compileJSONPointer(parentPath.concat(newProp)), + value: entry.value + } + }), - // we move all lower down properties to the same parent again, - // to force them to move under the inserted properties instead of the - // 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) + // move all lower down keys so the inserted key will maintain it's position + ...nextKeys.map(key => moveDown(parentPath, key)) + ] } } @@ -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. - * If not provided, the renamed key will be moved - * to the bottom of the list with keys. + * these keys will be moved down, so the renamed + * key will maintain it's position above these 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 => { - return { - op: 'move', - from: compileJSONPointer(parentPath.concat(key)), - path: compileJSONPointer(parentPath.concat(key)) - } - }) + ...nextKeys.map(key => moveDown(parentPath, 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) - const insertActions = values.map(entry => { - const newProp = findUniqueName(entry.key, parent) - return { - op: 'add', - path: compileJSONPointer(parentPath.concat(newProp)), - value: entry.value - } - }) + return [ + // remove operations + ...removeAll(paths), - // we move all lower down properties to the same parent again, - // to force them to move under the inserted properties instead of the - // new properties appearing at the bottom of the object - const keys = Object.keys(parent) - const beforeKey = last(firstPath) - 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 - } - }) + // insert operations + ...values.map(entry => { + const newProp = findUniqueName(entry.key, parent) + return { + op: 'add', + path: compileJSONPointer(parentPath.concat(newProp)), + value: entry.value + } + }), - 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)) + } +} diff --git a/src/utils/updateProps.js b/src/utils/updateProps.js index 67030d8..87e2d25 100644 --- a/src/utils/updateProps.js +++ b/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 - // so it maintains its identity and hence its index - updatedState = setIn(updatedState, parentPath.concat([STATE_PROPS, index, 'key']), newKey) + // A property is renamed. + + // 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 updatedProps = insertAt(deleteIn(props, [index]), [props.length - 1], oldProp) + const oldProp = props[oldIndex] + 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 [] +} \ No newline at end of file