Slate: make `operation.newPath` consistent

[ianstormtaylor]

Do you want to request a _feature_ or report a _bug_?

Improvement.

What's the current behavior?

Right now there are two properties involved in a move_node operation: the path and the newPath. The path makes sense, since it refers to the node in the current document.

But the newPath is confusing, because it currently refers to the new location of the node, but in respect to the current (old) document. This is usually fine, but gets tricky in cases of siblings. For example, given a document of:

<document>
  <paragraph key="a">
    one
  </paragraph>
  <quote key="b">
    <paragraph>two</paragraph>
  </quote>
</document>

To arrive at:

<document>
  <quote>
    <paragraph>two</paragraph>
    <paragraph>one</paragraph>
  </quote>
</document>

You use an operation of:

{
  type: 'move_node',
  path: [0],
  newPath: [1, 1],
}

Where [1, 1] refers to the location the node would be move to relative to the current document, but without removing the node from its original location. However, when you look at the end state, you'll notice that the node doesn't end up at [1, 1], but at [0, 1] on account of removing it earlier in the document and shifting everything backward one space.

This seems confusing?

What's the expected behavior?

Change the newPath to refer to the location in the document after the original node has been removed, so that it always ends up where newPath says it will be?

EDIT: Based on the conversation below, we should go with "before the original node has been removed" always instead.

[justinweiss]

I think this is true sometimes, but not all the time.

This might have changed, but when I last worked on this, moving a node under the same parent would act as if you had removed the node first, but other moves would act as if you hadn't. For example: move_node with path: [2] and newPath: [4], the node would end up in position [4] in the new document, not position [3]. Moves to new parents worked the way you described.

I don't mind using either the new tree or the old tree, but I think all moves should be consistent. For what it's worth, I wrote a helper function to rewrite all the moves to use old-tree-positions, because I found them easier to think about.

[ianstormtaylor]

@justinweiss thank you! That's helpful.

For what it's worth, I wrote a helper function to rewrite all the moves to use old-tree-positions, because I found them easier to think about.

What was it that made them easier you think?

[justinweiss]

You only have to keep one operation in your mind, instead of two.

If you're talking old-tree-positions, you're imagining the same action as insert_node -- you only need to visualize one tree.

If you're talking new-tree-positions, you have to mentally run a remove_node operation before you can figure out the correct position. path then refers to one tree, newPath to another, and you need to be able to keep them both in your mind at the same time.

I'll get that second option wrong a hundred times before I get the first option wrong once 😆

[ianstormtaylor]

@justinweiss nice, thank you! That's super helpful. Thinking of it as an insert_node is a good way to do it.

[bryanph]

@justinweiss so in what sense are the path operations inconsistent now? I remember struggling with this a while ago, being confused about what the newPath actually referred to. Are there any changes we need to make to make it consistent?

[justinweiss]

~@brianph The inconsistency is how we treat paths when both the path and newPath are under the same parent, and when they aren't. When it's under the same parent, newPath refers to a new-tree position (the tree after the node was removed), and for other moves, newPath refers to the old-tree position.~

It looks like this code has been significantly changed since the last time I dove into it, so my comments are probably no longer accurate :-)

[bryanph]

EDIT: Based on the conversation below, we should go with "before the original node has been removed" always instead.

@ianstormtaylor isn't this already the case? :thinking:

edit: i just reread justin's comment: so the inconsistent case is the case where the node is moved in the same "path level" (how do you call this :sweat_smile: )?

[ianstormtaylor]

@bryanph yup, it sounds like we just have a single inconsistent case, where it's moved related to its siblings inside the same parent.

[justinweiss]

It also may no longer be inconsistent -- I'm still using 0.33, and it looks like things have changed significantly since then.