Feature suggestion: isolated/atomic operation groups in HistoryEditor

[arimah]

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

Feature 🛠

Background

When developing an editor with rich functionality, it is occasionally necessary to implement high-level actions that comprise many low-level operations. For example, wrapping text in a link may involve first unwrapping an existing link in the selection – two (or more) low-level operations, one high-level actions. Or, perhaps you need to manipulate multiple blocks independently in order to, say, change indentation levels or create/remove lists.

For the user, these are single actions that should be undone with a single stroke of Ctrl+Z or Cmd+Z, and they should not be folded into previous interactions. Undoing a link insertion should not undo text that was entered before making the link, e.g.

One way to accomplish this is to use HistoryEditor.withoutMerging(), which prevents the next operation from being merged into the previous, effectively forcing a new history state. Unfortunately, it also creates new history states for all operations performed inside the callback. A workaround is to call withoutMerging() only for the first operation, if you need to perform many, but this quickly gets messy and it can be hard to track what operations have even been emitted.

Proposed solution

In addition to HistoryEditor.withoutMerging(), perhaps we can introduce a new HistoryEditor.atomic() (or asAtomic(), or isolated(), or asSingleState(), or whatever you want to call it), which always combines all of its operations into a single undoable state, and never merges with anything before or after it. This would make it trivial to write code along these lines:

// Everything here becomes one isolated history state.
HistoryEditor.atomic(editor, () => {
  MyTransforms.unwrapLink(editor);
  MyTransforms.wrapLink(editor, linkProps);
});

// And this is a separate, isolated state.
HistoryEditor.atomic(editor, () => {
  for (const [block, path] of affectedBlocks(editor)) {
    MyTransforms.doSomething(editor, block, path);
  }
});

As another example, suppose you want to transform * at the start of a line to a bullet list, but you want the user to be able to undo the space insertion and the list transformation independently. This would now be trivial to implement:

// First insert the space.
HistoryEditor.atomic(editor, () => {
  editor.insertText(' ');
});

// Then delete the trigger text and transform to a list.
HistoryEditor.atomic(editor, () => {
  Transforms.delete(editor, {
    distance: 2,
    reverse: true,
    unit: 'character',
  });
  MyTransforms.formatBulletList(editor);
});

This also means you can now safely use transformations that may result in multiple operations without worrying about "leaking" too many things into the history state. E.g. insertText is not safe to call inside withoutMerging() if the selection is not collapsed, as it will generate a remove_text operation followed by insert_text.

If the new proposed function is called within its own callback, as in

HistoryEditor.atomic(editor, () => {
  HistoryEditor.atomic(editor, () => { ... });
});

then the outermost call should probably become the only undoable history state.

Let me know what you think of this idea. :) I believe it would make many situations drastically simpler, and remove several of the footguns that are built into withoutMerging().

Unsolved problem: what happens if atomic() is called inside withoutMerging()? What about the reverse?

[BrentFarese]

My question here is how is this different than just using Editor.withoutNormalizing, which basically batches a set of Operations into an "atomic" unit anyways (and, as a consequence, makes the history state include that batch of operations)? I think Editor.withoutNormalizing could be renames to Editor.transaction really and it might be more descriptive.

Let me know. Might work on this issue or ask the folks at MLH to do so.

[arimah]

If I understand the code correctly, Editor.withoutNormalizing() defers normalization until the passed callback returns, and does not have any effect at all on the history. If you look at the code for HistoryEditor, you can see it doesn't care at all about whether the editor is currently normalizing or not; history states are updated when editor.apply() is called (that is, when operations are pushed to the editor).

My proposal is more about controlling exactly which operations become part of a history state – that is, being able to say "anything that happens inside this callback is its own isolated history state". That way you could undo/redo higher-level operations (e.g. wrap selection in link) without having to worry about which low-level operations they use under the hood (e.g. unwrap existing link, merge some text nodes, split some text nodes, wrap a new link).

Thank you for the question. :)

[BrentFarese]

If I understand the code correctly, Editor.withoutNormalizing() defers normalization until the passed callback returns, and does not have any effect at all on the history. If you look at the code for HistoryEditor, you can see it doesn't care at all about whether the editor is currently normalizing or not; history states are updated when editor.apply() is called (that is, when operations are pushed to the editor).

My proposal is more about controlling exactly which operations become part of a history state – that is, being able to say "anything that happens inside this callback is its own isolated history state". That way you could undo/redo higher-level operations (e.g. wrap selection in link) without having to worry about which low-level operations they use under the hood (e.g. unwrap existing link, merge some text nodes, split some text nodes, wrap a new link).

Thank you for the question. :)

No problem! Yes, but that is what Editor.withoutNormalizing does. It baches all Operations submitted within the callback into 1 "transaction" in the history stack. These are the lines in the code that do that. Basically, if operations.length !== 0, which is the case when you use Editor.withoutNormalizing and multiple Operations are submitted within the callback, then all those Operations are merged into a single array in the history stack.

When we want to ensure a given "user action" is 1 history event, we wrap it in Editor.withoutNormalizing. This works for wrapping/unwrapping links, etc. in our codebase as it stands now.

Does this change your assessment in this issue? I think Slate already supports what you want. I am actually in favor of renaming Editor.withoutNormalizing to Editor.transaction as that is more descriptive of what it actually does.

[arimah]

Well... it may happen to work with the current implementation, but I don't think that's the intent at all. Editor.withoutNormalizing() is designed to defer normalization, so you can perform possibly complex modifications of the node tree without normalization changing things under your nose. That is, it allows the node tree to be potentially invalid until you're done fiddling with it.

The field editor.operations contains all operations that have been applied since the last flush (which means: since the last onChange event). See the relevant lines in createEditor(). There are ways to produce multiple operations that don't involve Editor.withoutNormalizing(): for example, editor.insertText("something") with a non-collapsed selection causes two operations to be emitted.

This happens to interact with HistoryEditor approximately as you describe, but still doesn't give you that fine-grained control I was hoping for. The first operation in a given "tick" can still be merged into the previous history state (see shouldMerge()). From this we can see that if, say, you emit an insert_text as the first operation in your user action, it may get merged into previous typing, even if you don't want it to. Moreover, you can't produce multiple isolated user actions in a single "tick".

So... in short, no, I think using Editor.withoutNormalizing() for this is misleading, and renaming it to Editor.transaction seems like it would cause even more problems. Also, remember that slate-history an optional plugin. Anything that directly influences the history stack should, in my opinion, be manipulated through HistoryEditor.

[BrentFarese]

Well... it may happen to work with the current implementation, but I don't think that's the intent at all.

This is the intent of the system. If you want to batch Operations, defer normalization and onChange and have those batched operations show up in the history stack in a single array, that is what Editor.withoutNormalizing is for. There may be edge cases that your proposed HistoryEditor.atomic covers that are not covered by Editor.withoutNormalizing, but the vast majority of cases are covered pretty well already. Do you want to submit a PR with your proposal? Happy to consider an improvement that covers the edge cases you are concerned about.

Thanks!

[arimah]

Hmmmm. I'm now fairly confused, particularly about this bit:

If you want to batch Operations, defer normalization and onChange and have those batched operations show up in the history stack in a single array, that is what Editor.withoutNormalizing is for.

That's a lot of "and"s to describe one single method's behaviour, for starters. :smile: But, beyond that, what you've just said contradicts how Slate actually works in its current form. Let's go through it one bit at a time.

batch Operations

Operations are batched (per event tick) automatically; that's what the Promise.resolve in createEditor() does. I assume this exists so onChange doesn't fire for every single operation. It also ensures normalization has a chance to run before onChange, so you never catch the document in an invalid state.

defer normalization

True to its name, it does indeed do that!

and [defer] onChange

Slate does that automatically and non-configurably, as part of the operation batching as linked above.

and have those batched operations show up in the history stack in a single array

Except the HistoryEditor doesn't seem to care at all whether normalization has been deferred. All operations that occur in the same tick are always merged into a single history state – whether intended or not, that's the effect of the if (operations.length !== 0) condition that you linked to earlier. (And that's a good thing, of course!)

By way of example, I put together a small example app on CodeSandbox that I hope showcases what I mean. In addition to the multi-operation action I implemented, you can also generate a multi-operation batch by selecting some text and typing over it. This will generate a remove_text followed by an insert_text.

So now I'm left wondering whether the implementation is inconsistent with the intention, or whether the intention you describe is the result of some species of misunderstanding. Maybe you have more insider knowledge than I do; I can only comment on how the editor actually works. Perhaps I am the one who's misunderstood how to use these APIs.

---

I hope you don't read any antagonism into my words. I'm just confused and would like to know how to proceed. Maybe my app needs a small rewrite, or maybe I need to open a bug report titled "Editor.withoutNormalizing only defers normalization". :smile:

Thank you for taking the time to discuss this.

[BrentFarese]

So your characterization is correct @arimah. My apologies! I had forgotten that we internally modified how onChange is called for our implementation and we've made it entirely synchronous vs. asynchronous as it is in regular Slate. So, the way you've described it above is entirely correct.

For our implementation tho, we call onChange in Editor.normalize at these lines right before the return. This makes onChange synchronous and, for us, means that every time we wrap a bunch of transforms in Editor.withoutNormalizing, it acts to defer onChange, group all Operations in the callback into 1 point in the history stack, and then call onChange synchronously after the editor has been normalized.

This means it's pretty easy for us to group a number of Operations into a distinct point in the history stack. We just wrap multiple transforms in Editor.withoutNormalizing. For us, it acts a bit more like an Editor.transaction (hence my confusing statements above).

Perhaps what we did is helpful to you, maybe not. If you are only looking for grouping Operations together in the history stack in a reliable fashion, then maybe having HistoryEditor.atomic or HistoryEditor.transaction would be useful. We don't have a need for it yet for the reasons explained above, our history stack is pretty clean/nice.

But I'm glad to take a look at a PR or a proposal to add this functionality. Hopefully this discussion wasn't wasteful! Thanks.

[rmccue]

I'm running into a similar issue (I believe) where one user action is multiple operations internally, and is leading to confusing undo behaviour.

Specifically, based on an external event, I use Transforms.insertNodes to insert an inline void element. Internally, Slate appears to be composing this of a split_node and insert_node.

Without calling HistoryEditor.withoutMerging, these node events are being merged into the previous typing (despite being a discrete user action); with calling withoutMerging, the undo stack ends up with two items. Undoing only the insert_node without the split_node appears to be crashing Slate (as Transforms.insertNodes calls withoutNormalizing internally).

[bryanph]

@BrentFarese Your change to how onChange gets called is interesting (synchronously instead of like currently on next tick), do you think it could be a reasonable default?

Also renaming withoutNormalizing to something like Transaction seems like a great idea!

Although I feel like a Transaction API would ideally look a little different so that we can even support rollback and other normal database-y transaction features...!

Perhaps something more akin to:

const transaction = editor.startTransaction()
Transforms.insertText(transaction, 'abc')
Transforms.insertText(transaction, 'def')
transaction.commit()

Which would work if we'd assume the changes are applied synchronously like in @BrentFarese's implementation.

As for the original issue, I do agree with @arimah that the current withoutMerging behavior is not very flexible, but this might be more or less resolved by making onChange called synchronously in Editor.normalize like @BrentFarese suggested?

Also just want to add that it's easy to confuse the atomicity of operations applied to the slate value and operation merging in the history. I think they're two separate issues (although one can influence the other, as the with-history plugin currently does rely on editor.operations to merge changes.

[BrentFarese]

@BrentFarese Your change to how onChange gets called is interesting (synchronously instead of like currently on next tick), do you think it could be a reasonable default?

Yes, we have been using it for 12 months at Aline and it works very well. Just wrap anything you don't want to synchronously normalize in withoutNormalizing and you're good.

It's a simple code change so we implemented it using patch-package and patched Slate directly.

[DCzajkowski]

We have found with @gtluszcz that a pseudo-implementation of HistoryEditor.atomic could be:

HistoryEditor.atomic = function (editor, callback) {
  editor.history.undos.push({ operations: [], selectionBefore: null })

  callback() // This callback must produce one and only one history entry!!!

  editor.history.undos.splice(-2, 1)
}

It seems to work for our needs (it prevents merging operations inside callback with the previous history entry).

This implementation relies on the fact, that lastOp in https://github.com/ianstormtaylor/slate/blob/f0f477226402e8200a35c7244a637437a332e6c1/packages/slate-history/src/with-history.ts#L73C11-L73C17 will be undefined, therefore there is no previous operation Slate could merge callback with. Next, we remove that empty history entry.

Of course, this implementation can be adjusted to be more safe etc. (eg. instead of .splice(-2, 1) we can find that operation by some special symbol)