Backend undo manager
Closed this issue ยท 30 comments
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?
Can we still register the undo manager front-end side for it to work in jupyterlite?
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?
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?
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.
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.
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?
indeed, JupterCAD updates object metadata after rendering.
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.
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
It turns out the issue was indeed with this._savemeta(result)
. When I commented it out, the redo worked properly.
I'm wondering if the undo manager should support this use case?
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.
Here is how an object looks like. It's a standard YMap
and the metadata is not defined as a shared type.
"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"
}
}
}
},
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?
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.
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).
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.
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
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?
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
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.
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 ๐
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.
Yes I think it would work (metadata
doesn't need to be a YMap
, only data
).
I can work on origins in pycrdt
in parallel, but I also don't know about the amount of work.
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 ๐บ
As I said, it wouldn't work in all cases ๐ญ
@Meriem-BenIsmail could you test your undo manager with #385 to see if the redo works?
@trungleduc I just tested #385 and both the undo and redo work perfectly !