operations/invert should not rely on internal state

[bryanph]

Do you want to request a feature or report a bug?

Feature/bug-ish

What's the current behavior?

The undo/redo stack makes use of the invert function here

slate/packages/slate/src/operations/invert.js

Line 22 in ecf4892

function invertOperation(op) {

to invert the applied operations and apply them. This function replies on internal state which is removed here:

slate/packages/slate/src/models/operation.js

Lines 239 to 251 in ecf4892

	for (const key of ATTRIBUTES) {
	let value = this[key]
	// Skip keys for objects that should not be serialized, and are only used
	// for providing the local-only invert behavior for the history stack.
	if (key == 'document') continue
	if (key == 'selection') continue
	if (key == 'value') continue
	if (key == 'node' && type != 'insert_node') continue
	if (key == 'mark' || key == 'marks' || key == 'node') {
	value = value.toJSON()
	}

As a result, reverting operations on the user-side requires custom code to invert the given operations after this internal state is gone (like after a refresh).

What's the expected behavior?

My suggestion is to make operation.invert() just be a function of the document state and the raw operation, not on any internal state so that the case of inverting an operation from within the editor and from outside the editor is the same. This allows you to instantiate the history stack with operations that have been synced to a server on refresh and would make implementing a "version" example quite straight forward.

[ianstormtaylor]

I think there's a nice property of being able to invert an operation without needing the original value/document that created it. But I agree that right now not all operations are living up to that goal.

I'd be down with making that more the case, by standardizing extra properties on the operations that can allow us to remove these "stateful" ones. For example, I think the only reason selection is kept in set_selection operations is to have access to the "previous properties" so it can invert them. But we could instead standardize a propertiesProperties property with that information instead?

[bryanph]

I agree, it's a nice property. Another option might be just to include those values in the JSON as well on perhaps a properties object as you suggest. Then the user can decide whether to store the more detailed version or the minimal one. At times it can be useful to have these properties in user-land as well I find.

Maybe we should just aim to include an example which demonstrates versions with slate? So an example where there are a few version states (references to Change objects) between which you can switch (and which roll back or apply changes as needed). Then we can decide what we need to do to make an example like that functional.

Ultimately, we'd probably want to somehow identify Change objects so we can refer to them as a rollback target and so we can create something like a version tree, with different branches (basically git-like)

[ianstormtaylor]

@bryanph that sounds good to me! I think the example should ideally use the lower-level Operation objects, and not need to keep Change objects around.

[bryanph]

We still need to be able to refer to groups of operations though, since that is what defines an actual delta to the user. Perhaps we can introduce a Delta model or something like that, which holds a group of operations and identifies it with some key? That way we can keep it away from the Change model and just make the Change object refer to the Delta model. This model could then also be responsible for inverting the group of operations and perhaps even identifying it as an abstract operation or command (in the spirit of #2066).

[ianstormtaylor]

That could be. But what if we made it simpler and just used immutable List groupings for operations like we already do on changes?

[bryanph]

Ok so how would moving to a given version work? Assuming we have an array of arrays of operations. To use a git analogy, a version is basically a commit? And the operations define how we move to this commit from the previous one?

[ianstormtaylor]

Yup exactly. The index in the top-level list is the version, and you take all of the bottom-level lists from your current index to the index you want to be at. If it’s forward you apply them directly. If it’s backward you invert them first and then apply them.

[bryanph]

@ianstormtaylor great, hopefully I can find some time to work on this some time soon :).

[zhujinxuan]

Hi, I am thinking about this solution:

When an operation is create, its undo-simulation is created simultaneously.

Reason (sorry about some math denotation, I find it is hard for me to express some ideas without math):

Suppose we have Document -> Document change as an operation f, based on the operation information itself, we cannot find an inverse g that for any after state of Document, f . g = g . f = 1. (where 1 is the map of returning itself (1 x = 1)). Because the inverse is heavily depending on the from document.

However, for any pair of documents processed by one or more operations, (Document a, Document b), there exists a pair of [operation] (p, q), that p a = b, q b = a. Then for a path from Document a to Document c, if we have the operations on path Document a >>> (p1, q1) = Document b, Document b >> (p2, q2) = Document c, then we can say Document a >>> (p1 + p2, q1 + q2) = Document c. Here we denote >>> as Document d >>> (operation foo, operation bar) = Document e that e is the document that d changes through operation foo, and d is the document that e changes through operation bar.

In short, we shall think an operation as exact arrow from one document to another document, rather than an abstract map between a set of documents to another set of documents.

Therefore, we can create an operation like this:

const a :: operation = editor.createOperation('set_mark', properties);

Then we can have operation structure like:

operation a: {
   type: set_mark,
   range: ....
   inverse: bundleOperation {
      inverse: a, // Refer to itself
      type: bundle,
      children: [
          {
              type: unset_mark,
              range: ....
              inverse: undefined,
          },
          {
              type: unset_mark,
              range: ....
              inverse: undefined,
          }
      ]
   }
}

Then during undo, we can just propose the inverse part. For collaborative editing, we can send the operation along with the inverse operation.

Note: for implementing this proposal, we may need to implement bundle operation first.

[zhujinxuan]

Then we define operation in this way:

data Operation a = Operation {
   type :: String,
   properties :: Properties,
   inverse: Maybe Operation
} | Bundle [Operations]