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

Undo manager without origin doesn't catch updates with origin #624

Open
2 tasks done
davidbrochart opened this issue Apr 3, 2024 — with Huly GitHub · 7 comments
Open
2 tasks done

Undo manager without origin doesn't catch updates with origin #624

davidbrochart opened this issue Apr 3, 2024 — with Huly GitHub · 7 comments
Assignees
Labels

Comments

Copy link

davidbrochart commented Apr 3, 2024

Checklist

Describe the bug It seems that an undo manager with no tracked origin doesn't catch updates made with an origin:

(async () => {
  const Y = await import("yjs");

  const ydoc = new Y.Doc();
  const yarray = ydoc.getArray();
  const ytext = new Y.Text("Hello");
  yarray.insert(0, [ytext]);
  const undoManager = new Y.UndoManager([], {doc: ydoc});
  undoManager.addToScope(yarray);
  undoManager.clear();
  ydoc.transact(() => {ytext.insert(5, ", world");}, 0);  // specify an origin
  console.log(undoManager.canUndo());
  // prints: false
  ytext.insert(12, "!");  // don't specify an origin
  console.log(undoManager.canUndo());
  // prints: true
})();

Expected behavior Since origins in an undo manager are only supposed to filter specific changes, I think having an undo manager with no specified origin should catch all changes, even those done with an origin set.

Environment Information

  • Node.js v20.9.0
  • Yjs v13.5.49

Huly®: YJS-797

@dmonad
Copy link
Member

dmonad commented Apr 3, 2024

This would be a breaking change. I also don't agree that an empty list should catch everything.

There could be an option to catch everything. However, I'd like to understand the use case first and if there are alternative solutions (there probably are)

@davidbrochart
Copy link
Author

I also don't agree that an empty list should catch everything.

Why? The documentation says that "the changes can be optionally scoped to transaction origins". Doesn't it mean that by not specifying a transaction origin, the scope should include all origins?

My use case is simple: I want to revert a document to a previous state in time (say t0). So at t0 I create an undo manager for a shared type of this document. Somewhere, changes are made to the document with a specified origin (that I'm not aware of). Then I decide to revert the document to the state at t0. But currently the undo manager says that there is nothing to undo.

@dmonad
Copy link
Member

dmonad commented Apr 3, 2024

The thing is that there are currently users of Yjs who specify an empty list and add origins dynamically. If I changed the behavior so that "empty" catches everything, then this would be a breaking change.

There is still the option to have a special flag to catch everything or to set the list to null. An empty list for catching everything is out of the question as it would be a breaking change.

@davidbrochart
Copy link
Author

There is still the option to have a special flag to catch everything or to set the list to null.

Sure, that would be an option.

@davidbrochart
Copy link
Author

Hi @dmonad, do you think this could be implemented?

@davidbrochart
Copy link
Author

Friendly ping @dmonad 😄

@dmonad
Copy link
Member

dmonad commented May 15, 2024

Hey David, sorry I can't give an estimate on when I have time to implement this feature.
I'm currently flooded with other things. This ticket takes a low priority for me because it is not relevant for the majority of users.

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

No branches or pull requests

2 participants