embed() doesn't unembed() previous parent

This issue has been tracked since 2022-07-13.

If I'm reading the docs correctly, the entire embedding feature is designed like a tree:

cell B might or might not have a parent. If it does, it can be cell A.
cell A might or might not have children. If it does, they can be [cell B, cell C, cell D...].

The relationship from parent to children is represented by an array on values: getEmbeds() returns an array.
The other way around (from any given child to it's parent) is represented as a single id: parent() returns an id.

After playing for some days with this feature I hit a bug with how the embed() method works: it doesn't check (and unembed()) whether the cell that's going to be embedded is already embedded in another cell. This leads to an unreachable relation from the cell to it's previous parent (and a broken tree).

Example (pseudo-code):

parent1 = cell()
parent2 = cell()
child = cell()

parent1.embed(child)

parent1.getEmbeds()    <-- [child]
child.parent()         <-- parent1

parent2.embed(child)
parent2.getEmbeds()    <-- [child]
child.parent()         <-- parent2

parent1.getEmbeds()    <-- [child]     // shouldn't happen. "child" belongs to "parent2" now.
kumilingus wrote this answer on 2022-07-15

It's a good one. If the child already has a parent (i.e. child.get('parent') != null) we can either:

  • throw an exception (+ perhaps fail silently if the new parent is the same as the current one).
  • unembed the child first, then embed it into the new parent

I think I like the first option more. The other one implicitly changing another model (the original parent).

Workaround:

const currentParent = child.getParentCell();
if (currentParent) {
  if (currentParent === newParent) return;
  currentParent.unembed(child);
}
newParent.embed(child);
alexandernst wrote this answer on 2022-07-15

Maybe just add force as an option to embed(opt)? (or unembedFromCurrentParent or something like that)

kumilingus wrote this answer on 2022-07-20

That's also an option. We'd need to make sure that no exception is thrown in the middle of the "transaction" though (to keep the graph data valid).

graph.startBatch();
parent.unembed(child);
newParent.embed(child);
// embed() can also throw an exception
// we'd have detect and throw the error prior starting the batch
graph.stopBatch();
alexandernst wrote this answer on 2022-07-20

I have been willing to ask you about this for some time now. You mentioning the entire batch thing sounds like the perfect excuse for me to throw my question:

Is there a way to detect if we're in the middle of a batch?

I have been doing it with graph._batches?.["fit-embeds"] > 0, but I'm (ab)using a private var (_batches), which is not great.

kumilingus wrote this answer on 2022-07-20

I am aware that none of the batch logic is documented (as far as I know it's useful only with JointJS+), but you can use graph.hasActiveBatch('fit-embeds').

alexandernst wrote this answer on 2022-09-23

This solution ^ is not properly covering your concerns in #1733 (comment) (neither the force option that I mentioned).

kumilingus wrote this answer on 2022-09-23

I was only worried about the case where we internally un-embed the cell instead of throwing error (either as default behavior or with force option). When we throw an error, the responsibility is on the user.
Not adding the force option now was a conscious decision for two reasons:

  • we are in the release process, didn't want to spend time on this as it is not trivial and definitely not a blocker
  • keep the API clean if (a.isEmbedded()) { a.getParentCell().unembed(a); } c.embed(a);
More Details About Repo
Owner Name clientIO
Repo Name joint
Full Name clientIO/joint
Language JavaScript
Created Date 2009-09-11
Updated Date 2022-12-03
Star Count 3715
Watcher Count 155
Fork Count 817
Issue Count 51

YOU MAY BE INTERESTED

Issue Title Created Date Updated Date