Command manager is saving the entire attrs object on each attr() call (unexpected behaviour?)

alexandernst commented 5 months ago

Current versus expected behaviour

Consider the following code:

const a1 = new ShapeV4();
a1.position(20, 20);
graph.addCells([a1]);

a1.attr({ title: { textWrap: { text: "foo" } } });
console.log(cm.undoStack[1].data.next);

a1.position(21, 21);
console.log(cm.undoStack[2].data.next);

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 a title object with all the properties of the title element, but only the textWrap/text attribute was modified by the .attr() call. It also contains the pathBody 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

Open the POC at https://codesandbox.io/p/sandbox/jj4-textwrap-bug-y9l8kp
Look at the console

Version

4.0.1+

What browsers are you seeing the problem on?

Chrome, Safari

What operating system are you seeing the problem on?

Mac

kumilingus commented 5 months ago

This is expected.

The CommandManager makes a snapshot of an attribute value before and after the change (using shallow clone for performance). The snapshot is then applied simply with set() method i.e. without merging.

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 undefined it would get lost in the JSON export).

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 color or borderWidth (with data-binding to attrs) and record these instead of the attrs object (using cmdBeforeAdd: (eventName) => eventName !== 'change:attrs')).

alexandernst commented 4 months ago

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:

{
    prop1: "foo",
    prop2: ["a", "b", "c"],
    prop3: {
        prop3sub1: "bar",
        prop3sub2: 99,
    }
}

New props:

{
    prop1: "foo",
    prop2: ["a", "b", "d"],
    prop3: {
        prop3sub1: "bar",
    },
    prop4: "xyz"
}

Generated diff:

[
  {
    op: "remove",
    path: "/prop3/prop3sub2",
  }, {
    op: "replace",
    path: "/prop2/2",
    value: "d",
  }, {
    op: "add",
    path: "/prop4",
    value: "xyz",
  }
]

The command manager could hold a set of patches instead of the entire snapshot of all props.

Pros to this solution:

reduced command manager size (both in-memory and when exported to a file)
faster undo / redo operations, since the command manager would need to apply changes only to the props that were actually changed
the patches can be represented as json (they are, in fact, json)

Cons:

this is a breaking change

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 paths. This means that changing any prop will create a new snapshot of all the paths in the cell. We could workaround the problem with something like what you suggested (I had a couple similar ideas), but it feels hacky and imho this should be handled by the command manager.

alexandernst commented 4 months ago

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, "full_snapshots" would be assumed, thus keeping backwards compatibility.

alexandernst commented 2 months ago

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

github-actions[bot] commented 1 week ago

This issue is stale because it has been open 60 days with no activity. Please remove stale label or comment or this will be closed in 14 days.

clientIO / joint