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

Merge transactions in undo manager #592

Open
SebastianStehle opened this issue Oct 29, 2023 — with Huly GitHub · 7 comments
Open

Merge transactions in undo manager #592

SebastianStehle opened this issue Oct 29, 2023 — with Huly GitHub · 7 comments
Assignees

Comments

Copy link

SebastianStehle commented Oct 29, 2023

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. I am using yjs for a diagram editor or I am working on the integration of yjs into this editor. In my editor the selection state is also part of the model and I would like to ignore that in the undo redo operations, or to be more precise, it should not be added to the stack.

Lets consider a very basic example with 2 transactions and the initial state 0:

  1. Diagram has item A and B, B is selected.
  2. User selects A now
  3. User adds item C, which gets selected automatically.

Describe the solution you'd like If I just ignore transaction, then A would be selected after undo operation, which is not the same state as before. Therefore what I would like to achieve is that the transaction 1 is ignored but added to the undo stack after transaction 2 is completed.

Describe alternatives you've considered I think I can also implement it on top of the undo manager or implement it by adding additional metadata to the stack item as described in the docs, but I think this makes the most sense for me

Additional context Add any other context or screenshots about the feature request here.

Huly®: YJS-403

@himself65
Copy link
Contributor

You should set trackedOrigins or call stopCapturing() manually

@SebastianStehle
Copy link
Author

How would that help? Then the state diff to select my shape would not be captured and not reverted

@himself65
Copy link
Contributor

Sorry for my misunderstood.

First of all, the selection state should be in the awarenessStore, not in CRDT.

In the undoManager, there is a deleteFilter field that you can control whether to undo the item, that is similar to what you expected of the transactions.

@SebastianStehle
Copy link
Author

It could be an idea to move the selection to the awareness store, but I am not sure if it makes my life easier.

I use redux for my application and bind the redux store to yjs models with a custom binding that I have built: https://github.com/SebastianStehle/yjs-redux. The yjs stuff is just a recent addition and the redux store has existed way before that.

Many operations also require the selection state to be changed. For example, when you add a new item you want this to be selected. When you ungroup a group you want to children to be selected. Therefore it was easy to move this to the main state and just implement it in the reducer. Ofc it could be also modeled in a middleware and then synced to awareness state or something like that, but anyway. Some other operations like deletion are also simpler with the current setup. So there are a wide range of reasons not to put the selection into the awareness state.

No idea how deleteFilter would help.

@dmonad
Copy link
Member

dmonad commented Nov 16, 2023

As @himself65 said, with trackedOrigins, stopCapturing, and deleteFilter, you have a rich palette of tools to achieve almost all desired behaviors. https://docs.yjs.dev/api/undo-manager

I am using yjs for a diagram editor or I am working on the integration of yjs into this editor. In my editor the selection state is also part of the model and I would like to ignore that in the undo redo operations, or to be more precise, it should not be added to the stack.

Lets consider a very basic example with 2 transactions and the initial state 0:

0. Diagram has item A and B, B is selected.

1. User selects A now

2. User adds item C, which gets selected automatically.

Describe the solution you'd like If I just ignore transaction, then A would be selected after undo operation, which is not the same state as before. Therefore what I would like to achieve is that the transaction 1 is ignored but added to the undo stack after transaction 2 is completed.

Describe alternatives you've considered I think I can also implement it on top of the undo manager or implement it by adding additional metadata to the stack item as described in the docs, but I think this makes the most sense for me

Now, do you want to ignore the selection state (In my editor the selection state is also part of the model and I would like to ignore that in the undo redo operations) or not (If I just ignore transaction, then A would be selected after undo operation, which is not the same state as before.)?

If you manipulate the selection state in a different transaction, using an origin that is not tracked, then the selection should not change when performing undo/redo. See https://docs.yjs.dev/api/undo-manager

I guess that what you are asking is that there should be a configuration option to disable tracking of all fields that are named selection. I don't know yet how I would implement that in practice.

Maybe there could be an option isInScope: (item) => boolean that allows you to configure scope more granularity.

However, I agree with @himself65 that the selection state should be tracked using the awareness. 1. every user should be able to have a different selection. 2. you probably don't want to persist the selection state across sessions (e.g. when you reload the window).

@SebastianStehle
Copy link
Author

Perhaps my description was not precise and confusing. I want to "ignore" selection changes in a way that it does not add something to the undo stack but it gets merged with the next update.

I have achieved it the following way: https://github.com/mydraft-cc/ui/blob/collaboration/src/wireframes/collaboration/services/extended-undo-manager.ts#L33

I don't see how I could implement it in another way.

@dmonad
Copy link
Member

dmonad commented Nov 17, 2023

Ah okay, then you need to do something like you did. It is also possible to merge stack-items in Y.UndoManager. But currently this can only be done via startCapturing & stopCapturing or using the captureInterval.

If your solution works, then I'd like to close this ticket. I don't think we can cover really all use-cases in a single UndoManager, so it's nice to see that the current API helped to implement the desired behavior.

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

3 participants