move_node tests and code clean up (#2555)

* tests

* refactor moveNodeByPath

* refactor element.move_node

* refactor invert.move_node

packages/slate/src/commands/by-path.js

@@ -183,19 +183,25 @@ Commands.mergeNodeByPath = (editor, path) => {
 }
 
 /**
- * Move a node by `path` to a new parent by `newPath` and `index`.
+ * Move a node by `path` to a new parent by `newParentPath` and `newIndex`.
  *
  * @param {Editor} editor
  * @param {Array} path
- * @param {String} newPath
+ * @param {String} newParentPath
- * @param {Number} index
+ * @param {Number} newIndex
  */
 
-Commands.moveNodeByPath = (editor, path, newPath, newIndex) => {
+Commands.moveNodeByPath = (editor, path, newParentPath, newIndex) => {
   const { value } = editor
 
-  // If the operation path and newPath are the same,
+  // If the operation path and newParentPath are the same,
   // this should be considered a NOOP
+  if (PathUtils.isEqual(path, newParentPath)) {
+    return editor
+  }
+
+  const newPath = newParentPath.concat(newIndex)
+
   if (PathUtils.isEqual(path, newPath)) {
     return editor
   }
@@ -204,7 +210,7 @@ Commands.moveNodeByPath = (editor, path, newPath, newIndex) => {
     type: 'move_node',
     value,
     path,
-    newPath: newPath.concat(newIndex),
+    newPath,
   })
 }
 

packages/slate/src/interfaces/element.js

@@ -1763,13 +1763,12 @@ class ElementInterface {
     const newParentPath = PathUtils.lift(newPath)
     this.assertNode(newParentPath)
 
-    const [p, np] = PathUtils.crop(path, newPath)
+    const position = PathUtils.compare(path, newPath)
-    const position = PathUtils.compare(p, np)
 
     // If the old path ends above and before a node in the new path, then
     // removing it will alter the target, so we need to adjust the new path.
     if (path.size < newPath.size && position === -1) {
-      newPath = PathUtils.decrement(newPath, 1, p.size - 1)
+      newPath = PathUtils.decrement(newPath, 1, path.size - 1)
     }
 
     let ret = this

packages/slate/src/operations/invert.js

@@ -45,36 +45,17 @@ function invertOperation(op) {
       let inversePath = newPath
       let inverseNewPath = path
 
-      const pathLast = path.size - 1
+      const position = PathUtils.compare(path, newPath)
-      const newPathLast = newPath.size - 1
 
       // If the node's old position was a left sibling of an ancestor of
       // its new position, we need to adjust part of the path by -1.
-      if (
-        path.size < inversePath.size &&
-        path.slice(0, pathLast).every((e, i) => e == inversePath.get(i)) &&
-        path.last() < inversePath.get(pathLast)
-      ) {
-        inversePath = inversePath
-          .slice(0, pathLast)
-          .concat(inversePath.get(pathLast) - 1)
-          .concat(inversePath.slice(pathLast + 1, inversePath.size))
-      }
-
       // If the node's new position is an ancestor of the old position,
       // or a left sibling of an ancestor of its old position, we need
       // to adjust part of the path by 1.
-      if (
+      if (path.size < newPath.size && position === -1) {
-        newPath.size < inverseNewPath.size &&
+        inversePath = PathUtils.decrement(newPath, 1, path.size - 1)
-        newPath
+      } else if (path.size > newPath.size && position !== -1) {
-          .slice(0, newPathLast)
+        inverseNewPath = PathUtils.increment(path, 1, newPath.size - 1)
-          .every((e, i) => e == inverseNewPath.get(i)) &&
-        newPath.last() <= inverseNewPath.get(newPathLast)
-      ) {
-        inverseNewPath = inverseNewPath
-          .slice(0, newPathLast)
-          .concat(inverseNewPath.get(newPathLast) + 1)
-          .concat(inverseNewPath.slice(newPathLast + 1, inverseNewPath.size))
       }
 
       const inverse = op.set('path', inversePath).set('newPath', inverseNewPath)

packages/slate/test/commands/by-path/move-node-by-path/new-path-exists.js

@@ -0,0 +1,38 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+import PathUtils from '../../../../src/utils/path-utils'
+import assert from 'assert'
+
+const pathA = PathUtils.create([0, 0])
+const pathB = PathUtils.create([1])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 0)
+  assert(editor.operations.size >= 1)
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>
+        <paragraph>I am gonna move</paragraph>
+      </paragraph>
+      <paragraph>
+        <paragraph>I am an existing node at newPath</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <paragraph />
+      <paragraph>
+        <paragraph>I am gonna move</paragraph>
+        <paragraph>I am an existing node at newPath</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)

packages/slate/test/commands/by-path/move-node-by-path/new-sibling-is-text-node.js

@@ -0,0 +1,34 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+import PathUtils from '../../../../src/utils/path-utils'
+import assert from 'assert'
+
+const pathA = PathUtils.create([0])
+const pathB = PathUtils.create([1])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 1)
+  assert(editor.operations.size >= 1)
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>I am gonna move</paragraph>
+      <paragraph>
+        <cursor />Existing text
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <paragraph>
+        <cursor />Existing text<paragraph>I am gonna move</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)

packages/slate/test/commands/by-path/move-node-by-path/path-left-sibling-of-new-path-ancestor.js

@@ -0,0 +1,39 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+import PathUtils from '../../../../src/utils/path-utils'
+import assert from 'assert'
+
+const pathA = PathUtils.create([0])
+const pathB = PathUtils.create([1])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 1)
+  assert(editor.operations.size >= 1)
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>
+        <cursor />I am gonna move
+      </paragraph>
+      <paragraph>
+        <paragraph>I am an existing node in newParent</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <paragraph>
+        <paragraph>I am an existing node in newParent</paragraph>
+        <paragraph>
+          <cursor />I am gonna move
+        </paragraph>
+      </paragraph>
+    </document>
+  </value>
+)

packages/slate/test/commands/by-path/move-node-by-path/sibling-swap.js

@@ -0,0 +1,39 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+import PathUtils from '../../../../src/utils/path-utils'
+import assert from 'assert'
+
+const pathA = PathUtils.create([0, 0])
+const pathB = PathUtils.create([0])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 1)
+  assert(editor.operations.size >= 1)
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>
+        <paragraph>
+          <cursor />I am gonna move
+        </paragraph>
+        <paragraph>I am an existing node at newPath</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <paragraph>
+        <paragraph>I am an existing node at newPath</paragraph>
+        <paragraph>
+          <cursor />I am gonna move
+        </paragraph>
+      </paragraph>
+    </document>
+  </value>
+)

packages/slate/test/commands/by-path/move-node-by-path/text-nodes.js

@@ -0,0 +1,35 @@
+/** @jsx h */
+
+import h from '../../../helpers/h'
+import PathUtils from '../../../../src/utils/path-utils'
+import assert from 'assert'
+
+const pathA = PathUtils.create([0, 0])
+const pathB = PathUtils.create([1])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 1)
+  assert(editor.operations.size >= 1)
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>Text that will move</paragraph>
+      <paragraph>
+        <cursor />Existing text
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = (
+  <value>
+    <document>
+      <paragraph />
+      <paragraph>
+        <cursor />Existing textText that will move
+      </paragraph>
+    </document>
+  </value>
+)

packages/slate/test/history/undo/move-node-by-path-ancestor-left-sibling.js

@@ -0,0 +1,27 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+import PathUtils from '../../../src/utils/path-utils'
+
+const pathA = PathUtils.create([0])
+const pathB = PathUtils.create([1])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 1)
+  editor.flush().undo()
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>
+        <cursor />I am gonna move
+      </paragraph>
+      <paragraph>
+        <paragraph>I am an existing node in newParent</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = input

packages/slate/test/history/undo/move-node-by-path.js

@@ -0,0 +1,27 @@
+/** @jsx h */
+
+import h from '../../helpers/h'
+import PathUtils from '../../../src/utils/path-utils'
+
+const pathA = PathUtils.create([0, 0])
+const pathB = PathUtils.create([1])
+
+export default function(editor) {
+  editor.moveNodeByPath(pathA, pathB, 1)
+  editor.flush().undo()
+}
+
+export const input = (
+  <value>
+    <document>
+      <paragraph>
+        <paragraph>I am gonna move</paragraph>
+      </paragraph>
+      <paragraph>
+        <paragraph>I am an existing node at newPath</paragraph>
+      </paragraph>
+    </document>
+  </value>
+)
+
+export const output = input