-
Notifications
You must be signed in to change notification settings - Fork 852
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
Command manager is saving the entire attrs object on each attr() call (unexpected behaviour?) #2687
Comments
This is expected. The Even if we would provide a callback, we would have to somehow record the property removal. It would not appear in the diff (and if we record it as I am not convinced these extra data is an issue. How many data are we talking about? If this size is an issue, you can also introduce top-level properties such as |
Hi Roman! Sorry for not replying earlier. It has been a crazy month nearing the launch. Its a difficult problem and the solutions won't be easy (if you were to actually accept this as a bug that needs to be fixed). A possible solution would be to use JSON Patch Notation Objects (RFC 6902) (ref implementation in js) with a diffing algorithm that could produce sequential patches (very much like a cvs). Consider the following props / data and the possible patch: Original props:
New props:
Generated diff:
The command manager could hold a set of patches instead of the entire snapshot of all props. Pros to this solution:
Cons:
About our use case and why I created this issue: We do actually have quite a big undo/redo CM size because of this behavior. Each cell contains quite some elements, and each element might contain complex svg |
Actually, this can be implemented without it being a breaking change! A new option can be added to the constructor of the command manager. Such an option could let users specify which method do they want to use. Eg: const commandManager = new dia.CommandManager({
diffing_algorithm: "full_snapshots", // "diffs_rfc6902",
}); If the option isn't provided, |
Hi @kumilingus ! I just wanted to let you know that I implemented a command manager with JSON Patch Notation Objects. Feel free to take a look at it and grab whatever might fit in JJ+! 🙏 https://github.com/Develatio/nebulant-builder/blob/develop/src/core/CommandManager.js |
Current versus expected behaviour
Consider the following code:
I get the following results in the console:
The second result is totally expected. It contains an object with the modified property (
position
), but the first object contains unexpected data. It contains atitle
object with all the properties of thetitle
element, but only thetextWrap/text
attribute was modified by the.attr()
call. It also contains thepathBody
object, which wasn't modified.In a real world scenario, when working on nodes with lots of attrs and/or embedded data, this behaviour results in quite a big (in size) undo and redo stacks.
Is this expected behaviour? Is there a way to make the command manager apply a diffing algorithm to the data that is being saved in the undo/redo stacks?
Steps to reproduce
Version
4.0.1+
What browsers are you seeing the problem on?
Chrome, Safari
What operating system are you seeing the problem on?
Mac
The text was updated successfully, but these errors were encountered: