Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UndoManager doesn't persist meta #611

Open
davidlessonup opened this issue Feb 16, 2024 · 4 comments
Open

UndoManager doesn't persist meta #611

davidlessonup opened this issue Feb 16, 2024 · 4 comments
Assignees

Comments

@davidlessonup
Copy link

Checklist

[ ] Are you reporting a bug? Use github issues for bug reports and feature requests. For general questions, please use https://discuss.yjs.dev/
[ ] Try to report your issue in the correct repository. Yjs consists of many modules. When in doubt, report it to https://github.com/yjs/yjs/issues/

Is your feature request related to a problem? Please describe.
Currently, we are using Yjs to store the content for a collaborative slide creation tool, with state related to the current slide on multiple parts in the YDoc, to track which slide the change belongs to we store the slide identifier in the undo meta, exactly like the docs describe here, but the meta is not reliable in a situation where you undo/redo/undo in succession.
This seems to be caused by the fact that when a StackItem is moved from the undo stack to the redo stack (or vice-versa) the meta data is not persisted. A new item is created, (as seen here), without transferring the meta of the popped event.

There is also no other indicator in the stack-item-added callback that we are dealing with a new user action or a transfer between undo and redo stack.

Describe the solution you’d like
Ideally when a StackItem is moved from one stack to the other, the metadata should be persisted in the item as well, or alternatively  there should be a way to access the previous metadata, so we can reapply the meta in the stack-item-added callback.

Describe alternatives you’ve considered
Right now we are crawling through the changedParentTypes in the event and getting the affected slide information from there, but this does not cover all edge cases and feels brittle.

@dmonad
Copy link
Member

dmonad commented Feb 25, 2024

The meta property is intended to store information about the "before-state" that is required to restore exactly that state. One example would be to store the cursor position that should be recovered when undo is triggered. When you undo several undo-items at once (e.g. because they are merged), you usually only want to recover the first meta state.

Hence, we intentionally discard the meta properties of stack items that are merged. Only the first meta generated should survive.

There might be an argument why stack-items should be merged. However, this would be confusing to most, as the first stack-item created should always be the "authority". I suggest that you store all relevant information on all meta states.

If this seems unfeasible to you, can you please explain your use-case?

@davidlessonup
Copy link
Author

davidlessonup commented Apr 1, 2024

First of all, apologies for the late reply.

The meta property is intended to store information about the "before-state" that is required to restore exactly that state. One example would be to store the cursor position that should be recovered when undo is triggered. When you undo several undo-items at once (e.g. because they are merged), you usually only want to recover the first meta state.

The issue we're having lies somewhere with this.

Let's consider an undo action:
When the stackItem is popped from undo the beforeState sync doesn't apply in time for the creation of the redo stackItem meta.

Shouldn't the previous meta be accessible for cases like this? Because when moving something from a stack to another more often than not you could be creating the same meta information

@dmonad
Copy link
Member

dmonad commented Apr 8, 2024

Let's say:

  • Before performing a change, the cursor is at position X.
  • Before performing the second change, the cursor position is at position Y.
  • These two changes are merged into a single undo item.

When I hit undo, I expect that the cursor position is at position X.

I don't see a need to preserve the state "cursor it at position Y", because that change is merged into the first undo-stack item. We usually only want to restore a single state.

This is what the undo-manager was designed for. I'm not saying that we can't extend it. But I need to understand the problem first.

Now let's say we want to we want to preserve the meta of the second change. Clearly, we don't want to restore position X AND Y. So how exactly should "metas" be merged? Please be more specific.

This is an extremely generic ask and I honestly don't have an incentive to fix your specific problem. I'm not sure a solution would be beneficial to all.

@davidlessonup
Copy link
Author

davidlessonup commented Apr 15, 2024

Merging aside, and like you mentioned above, meta is supposed to store a "before-state" (which in redo it can be considered an"after-state")

Here's an example of what is trying to be done. When the user redos an action it should restore the position to where it was before the undo. No matter how long the user takes between the actions.

Also due to the current order of events (StackItem gets added before pop), it is not possible to just create a new meta directly with the correct position.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants