Backend undo manager

[davidbrochart]

In jupyter-ydoc v3 there will be an undo manager that will allow to undo/redo operations on a shared model in the backend in a document-agnostic way. Every document inheriting from YBaseDoc will be responsible for adding to the undo manager's scope the shared types that it wishes to be included. For instance, for a YNotebook, the cells are included in the undo manager's scope.
For a YJCad document, it looks like self._yobjects should be included?

cc @Meriem-BenIsmail

[martinRenou]

Can we still register the undo manager front-end side for it to work in jupyterlite?

[davidbrochart]

Can we still register the undo manager front-end side for it to work in jupyterlite?

Yes, they are unrelated.

For a YJCad document, it looks like self._yobjects should be included?

I'm asking because @Meriem-BenIsmail could undo but not redo, when just adding self._yobjects to the scope. Is there something else that should be included?

[trungleduc]

I'm asking because @Meriem-BenIsmail could undo but not redo, when just adding self._yobjects to the scope. Is there something else that should be included?

Nope, the rendering logic is triggered on _yobjects change. Did she get the new _yobjects in the frontend?

[arjxn-py]

could undo but not redo

Hi, I can confirm as I encountered the same scenario in 383#comment

My observation was that redo only works once that only reverts last undo operation, but not after that.

[Meriem-BenIsmail]

Hi, when expanding the scope of the undo manager for the YJCad example like this
self.undo_manager.expand_scope(self._yobjects), the undo works but the same can't be said for the redo.
It seems the redo stack of the undo manager is always empty.

[davidbrochart]

One thing that could explain this behavior, is that JupyterCAD reacts to an undo operation by doing another operation on self._yobjects, which would clear the redo stack. Could that be the case?

[trungleduc]

indeed, JupterCAD updates object metadata after rendering.

JupyterCAD/packages/base/src/3dview/mainviewmodel.ts

Line 109 in 09f22d8

this._saveMeta(result);

[Meriem-BenIsmail]

The redo stack behavior isn't the only issue here. Whenever the objects are updated, this method should be invoked. It gets called during the undo operation, but not during the redo.

[davidbrochart]

indeed, JupterCAD updates object metadata after rendering.

But self._ymetadata is a separate shared type, so it is fine. It wouldn't be if self._yobjects were changed.

no, _ymetadata is the metadata of the document. Each object has its metadata and it is updated by the above call

[Meriem-BenIsmail]

It turns out the issue was indeed with this._savemeta(result) . When I commented it out, the redo worked properly.

[trungleduc]

I'm wondering if the undo manager should support this use case?

[davidbrochart]

No, it cannot support this use case.
Could you describe the schema of an object? If it has a shared type for the metadata that is well separated from the shared type of the "data", then we could only pass the later to the under manager's scope.

[trungleduc]

Here is how an object looks like. It's a standard YMap and the metadata is not defined as a shared type.

JupyterCAD/packages/schema/src/schema/jcad.json

Line 83 in 09f22d8

"jcadObject": {

    "jcadObject": {
      "title": "IJCadObject",
      "type": "object",
      "additionalProperties": false,
      "required": ["name", "visible"],
      "properties": {
        "name": {
          "type": "string"
        },
        "visible": {
          "type": "boolean"
        },
        "shape": {
          "$ref": "#/definitions/parts"
        },
        "parameters": {
          "type": "object"
        },
        "shapeMetadata": {
          "$ref": "#/definitions/shapeMetadata"
        },
        "operators": {
          "type": "array",
          "items": {
            "type": "object"
          }
        },
        "dependencies": {
          "type": "array",
          "items": {
            "type": "string"
          }
        }
      }
    },

[davidbrochart]

Thanks.
Is shapeMetadata automatically computed from the other values? Where is the line between what can be changed e.g. by the user and what reacts to these changes?

[trungleduc]

Is shapeMetadata automatically computed from the other values?

Yes, the geometry properties are computed by Opencascade after rendering. Some other keys are defined at shape creation time. None are modifiable by users and there are no reactivities on these values.

[davidbrochart]

Well I don't know this project enough, but if a user action on an object leads to a change of the same object, then it cannot work with an undo manager (at least it's not possible to redo after an undo).

[trungleduc]

can we detect the origin of the modification? if it is from the undo manager we can skip the metadata update since it's correct anyway.

[davidbrochart]

Good point. There is an issue for that, I can look into it.
But I don't think it will solve @arjxn-py's issue since his undo manager lives in the frontend and origins are not propagated.
I'm wondering why metadata is stored in the shared model if it can automatically be computed?

[trungleduc]

it can automatically be computed

JupyterCAD needs OpenCascade in the frontend to compute these geometry properties, but we also want to have access to these properties from a kernel

[davidbrochart]

In any case, if all the data were stored under a shared type, separate from the metadata, we could add that shared type to the undo manager's scope. Do you think it's possible?

[trungleduc]

yes, I thought about this solution but I feel more comfortable with the update origin approach since it's more natural to have the object metadata in the object itself. But we can use this solution as the last resort

[davidbrochart]

The metadata would still be in the object itself, but all the other (undoable) data would be under e.g. a YMap, so that we can pass it to an undo manager. Right now we can only pass the whole object itself, or even worse the list of objects.

[davidbrochart]

I feel more comfortable with the update origin approach

The problem with that is that origins don't propagate through the network, so if the metadata update happens in the frontend, it won't solve @Meriem-BenIsmail's issue, and if the metadata update happens in the backend, it won't solve @arjxn-py's issue 😄

[trungleduc]

something like this could work ?

_yobjects = YList[YMap[ data: YMap, metadata: YMap ]]

If it could, it's possible to update the shared model schema but I'm not sure about the amount of work.

[davidbrochart]

Yes I think it would work (metadata doesn't need to be a YMap, only data).

[davidbrochart]

I can work on origins in pycrdt in parallel, but I also don't know about the amount of work.

[trungleduc]

I can work on origins in pycrdt in parallel, but I also don't know about the amount of work.

if it's the same amount of work, yours would have a much greater impact 😺

[davidbrochart]

As I said, it wouldn't work in all cases 🐭

[trungleduc]

@Meriem-BenIsmail could you test your undo manager with #385 to see if the redo works?

[Meriem-BenIsmail]

@trungleduc I just tested #385 and both the undo and redo work perfectly !