spoenemann
commented
4 years ago Ok, we should execute all removals before adding new elements.
Closed BjBe82 closed 3 years ago
spoenemann
commented
4 years ago Ok, we should execute all removals before adding new elements.
spoenemann
commented
3 years ago A fix was submitted in #206. @BjBe82 do you have the chance to verify whether the issue is fixed in your context? A new next version of the sprotty package should be available soon.
BjBe82
commented
3 years ago The issue is not fixed by the fix in "applyMatches". Based on the stacktrace it is in:
protected computeAnimation(newRoot: SModelRoot, matchResult: MatchResult, context: CommandExecutionContext): SModelRoot | Animation {
const animationData: UpdateAnimationData = {
fades: [] as ResolvedElementFade[]
};
forEachMatch(matchResult, (id, match) => {
if (match.left !== undefined && match.right !== undefined) {
// The element is still there, but may have been moved
this.updateElement(match.left as SModelElement, match.right as SModelElement, animationData);
} else if (match.right !== undefined) {
// An element has been added
const right = match.right as SModelElement;
if (isFadeable(right)) {
right.opacity = 0;
animationData.fades.push({
element: right,
type: 'in'
});
}
} else if (match.left instanceof SChildElement) {
// An element has been removed
const left = match.left;
if (isFadeable(left) && match.leftParentId !== undefined) {
if (newRoot.index.getById(left.id) === undefined) {
const parent = newRoot.index.getById(match.leftParentId);
if (parent instanceof SParentElement) {
const leftCopy = context.modelFactory.createElement(left) as SChildElement & Fadeable;
parent.add(leftCopy); // ====> Here is the error
animationData.fades.push({
element: leftCopy,
type: 'out'
});
}
}
}
}
});I can also avoid the error if i use "new UpdateModelAction(input, false)" in "doSubmitModel" to deactivate animation
spoenemann
commented
3 years ago CC @danieldietrich
danieldietrich
commented
3 years ago Hi @BjBe82, I'm trying to reproduce your case but so far I was not able to create a failing test. Do the following examples reflect your situation? I simplified your example by flattening only 2 instead of 3 elements:
// given a root...
context.root = graphFactory.createRoot({
type: 'graph',
id: 'model',
children: [
{
type: 'node',
id: 'a',
children: [
{
type: 'node',
id: 'b'
}
]
}
]
});
// here is a flatten command using matches
const flattenCommand1 = new UpdateModelCommand({
kind: UpdateModelCommand.KIND,
animate: true,
matches: [
{
right: {
type: 'node',
id: 'b'
},
rightParentId: 'model'
},
{
left: {
type: 'node',
id: 'b'
},
leftParentId: 'a'
}
]
});
// here is a flatten command using a new root
const flattenCommand2 = new UpdateModelCommand({
kind: UpdateModelCommand.KIND,
animate: true,
newRoot: {
type: 'graph',
id: 'model',
children: [
{
type: 'node',
id: 'a',
},
{
type: 'node',
id: 'b'
}
]
}
});
// both executions work successfully
flattenCommand1.execute(context)
flattenCommand2.execute(context)Another question: does your runtime inject a SModelRegistry? This would explain that operations are performed on the same model root instead of another instance.
Thank you for your help!
BjBe82
commented
3 years ago Sorry for the delay i am currently working on another project. I am using a LocalModelSource which has the following "currentRoot":
this.currentRoot = {
type: 'graph',
id: 'mbse-graph',
children: []
};The modification of the hierarchy of the model is also done in the LocalModelSource and triggered by an Action:
handle(action: Action) {
switch (action.kind) {
case CreateFlatModelAction.KIND:
this.createFlatModel(action as CreateFlatModelAction);
break;
default:
// this.logger.info(this, 'Action: ' + action.kind);
super.handle(action);
}
}I just captured the model in LocalModelSource:: doSubmitModel(..) like this and attached it.
protected async doSubmitModel(newRoot: SModelRootSchema, update: boolean | Match[], cause?: Action, index?: SModelIndex<SModelElementSchema>): Promise<void> {
var modelSourceIndex = new SModelIndex<SModelElementSchema>();
modelSourceIndex.add(this.currentRoot);
console.log(JSON.stringify(newRoot));
return super.doSubmitModel(newRoot, update, cause, index);
}The error appears for element with ID “96f958fb-d59f-45c4-ae19-c80289d65bff_Block” which is strange because it is part of both outputs and basically the if branch “An element has been removed” should not be valid for that element.
So something is strange with the matching calculation or my model but with animation turned of anything works :/. Maybe I can reduce the model to a smaller size at the weekend.
danieldietrich
commented
3 years ago Thank you for your help @BjBe82 , I will check if I'm able to reproduce the case and come back to you!
BjBe82
commented
3 years ago After some additional investigation, the element id reported is not the source of the issue ... it is the parent. The usecase is to show only relevant parts involved in an information flow by removing not involved parts (i.e. removing the containment hierarchy).
Initial simplified view:
Model: output.txt
After the operation was performed to only show relevant parts (animation turend off):
Model: output2.txt
In the example, "B" is removed from the Model but for the animation it is added again. This causes a recursive call for the children which tries to add “C” to the model. But "C" is already part of the root and the index will throw an exception.
Error in the small model:
logging.ts:51 18:40:14 sprotty CommandStack: Failed to execute command: Error: Duplicate ID in model: C_Block
at SGraphIndex../node_modules/sprotty/lib/base/model/smodel.js.SModelIndex.add (smodel.ts:225)
at SGraphIndex../node_modules/sprotty/lib/graph/sgraph.js.SGraphIndex.add (sgraph.ts:191)
at SGraphIndex../node_modules/sprotty/lib/base/model/smodel.js.SModelIndex.add (smodel.ts:230)
at SGraphIndex../node_modules/sprotty/lib/graph/sgraph.js.SGraphIndex.add (sgraph.ts:191)
at SGraph../node_modules/sprotty/lib/base/model/smodel.js.SParentElement.add (smodel.ts:103)
at update-model.ts:185
at Object.forEachMatch (model-matching.ts:33)
at UpdateModelCommand../node_modules/sprotty/lib/features/update/update-model.js.UpdateModelCommand.computeAnimation (update-model.ts:163)
at UpdateModelCommand../node_modules/sprotty/lib/features/update/update-model.js.UpdateModelCommand.performUpdate (update-model.ts:101)
at UpdateModelCommand../node_modules/sprotty/lib/features/update/update-model.js.UpdateModelCommand.execute (update-model.ts:89)In theory there should be an additional check if an element, which is only added for the fade out animation has children which are still part of the model. If this is the case it should be removed from the children list. Maybe it’s also valid to remove all children of type “node” or “port” for the animation for the element … not sure if the animation does anything with child nodes and ports anyway.
At the moment this is not critical since we turned of the animation for the operation anyway but there might be other usecase where a fix might be important. What popped into my mind is a class + package diagram and the ability to hide the packages on demand … this will be like the above usecase where the classes are “moved” to the root and the packages get removed during the “hide” action.
danieldietrich
commented
3 years ago @BjBe82 thank you for your investigation and all the details, that helps!
danieldietrich
commented
3 years ago Status update: the hint with the parent was the missing part. I was able to reproduce the bug.
spoenemann
commented
3 years ago @BjBe82 did #209 fix the issue?
BjBe82
commented
3 years ago I can confirm that the fix solves the issue.
I have the following simplified model:
I have a operation which removes the hierarchy and produces a flat version of the model which is then set as "currentRoot". The new model looks like this:
Winn update is called i get the following error:
Current workaround is something like this at the moment (which is bad because it adds an additional command on stack which needs to be undone):
As it looks like the update implementation tries to add the Element C in currentRoot before removing it from Element B first.