search

thechiselgroup / biomixer

BioMixer
http://bio-mixer.appspot.com/
16 stars 13 forks source link

Losing Edges #538

Closed everbeek closed 9 years ago

everbeek commented 9 years ago

Had a mapping expansion, and lost edges. Was inspecting to see that they came back on hover over (as per temporary edges), when I then lost all the edges off of the central main node too. It was during node dragging, and likely involved mouse events on edges or nodes while dragging.

Attempting to replicate...

everbeek commented 9 years ago

I identified a logical error in manifestEdge() that might be the root of the problem. I add edges there, and can either allow or disallow temporary edges. I do not believe there is a guarantee that all egdes coming in will be all temporary style or all non-temporary, but when I call the addEdges() inside that, I pass all edges collected (possibly of both types), but with one argument designating the whole set as temporary or not.

I have separated the two types into two containers, and make two calls to the addEdges() now, to fix that logic. It might fix this bug. Inspecting further, because I was not able to replicate this yet.

everbeek commented 9 years ago

Accidentally replicated. The logic error was not causing this bug. Investigating again.

Aha, it was subsequent to deleting a node that was mapped to the central node. Replicable!

everbeek commented 9 years ago

Mis-committed #539 to this. Oops.

everbeek commented 9 years ago

Seems to be an error in var hiddenNodes = computeNodesToDeleteFunc(), not actually getting the nodes properly. This might be causing the downstream problems I observed in the debugger.

No, it's when node interaction checks return null in wasConceptClearedForInteraction...is that because I have registered the new deletion set too early, or are my return value semantics confused?

I have the nodeInteraction() changed to be an empty array, instead of null, and the bug no longer appears. But...the other case where null can be returned is in a composite set, and it seems unusual for a bug to be based on a return value like this, that occurs in more than one place, and appears in my comments. It seems like, if a node does not interact with a given set, that we must look at the previous set for whether it was cleared or not, which is to say, the solution is in the logic of wasConceptClearedForExpansion(), as opposed to changing the return values of the nodeInteraction() implementations. Verifying partly by looking at whether the null return was by design or accident, and whether the logic I might need to change serves a real purpose, or was just incorrect.