thechiselgroup / biomixer

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

Node Whitelisting Bad With Undo Redo #412

Closed everbeek closed 10 years ago

everbeek commented 10 years ago

I expanded on mappings and concepts for the root node. I then deleted the root node's ontology. I then did a mapping expansion on the still-present mapped node. This brought back the root node, as expected, but the entire cohort of concept expansions came with it, because the root node was in the concept expansion whitelist.

Review the purpose of the whitelist, and whether there is another way to achieve its goals. If not, figure out how to easily adjust whitelist use to prevent this.

everbeek commented 10 years ago

It might not be whitelists. I expanded mappings from one of the mappings, then deleted all ontologies other than the root one. Then I re-expanded the root mappings, and only got back the three immediately mapped ones, not the additional huge set I saw before. There was now a non-temporary mapping edge present though. I can fix that by going through mapping edges as we remove them from the graph, to remove them from the normal edge registry and put them in the temporary registry. This is problematic when combined with undo and redo though...

Do I need to redesign the temporary edge system to not use a registry? I would have to use a whitelist to see when to use temporary edges, and that's just another form of registry that will lead to the same problems.

Both of these registries cause problems when we delete nodes, and when we undo expansions. Both contains entries that become invalidated when these events happen. Will it be sufficient to check all deleted nodes for whitelist status and their edges to see if they should be temporary? The first part seems correct. Checking for temporary edge status changes would require looking at the whitelist, I am sure; we cannot know if an edge should be permanent without knowing that the source had a mapping expansion done.

Ok, so on node delete, remove all whitelists for that node (double check that they re-add when redoing expansions). Then, check all edges of deleting nodes for removal from the normal registry and addition to the temporary registry...ugh...I don't like the two registries, so look into a programmatic and efficient way to work with temporary hover-over mapping edges instead.

everbeek commented 10 years ago

I have the temporary edge registry replaced by logic making use of a flatter, refactored edge registry.

The whitelist nodes that are deleted could be removed from the whitelist, but because the undo/redo works on the basis of sets of nodes, and not re-enacting expansions, we lose the whitelisting if we do this. I won't be changing how the undo.redo work; using generic sets is the right way. But...it looks like changes to the whitelist state should be stored for undo/redo as well.

everbeek commented 10 years ago

I thought about making whitelist status a node property, but that doesn't fix the delete then re-expand problem.

I am now thinking about using the expansion sets, since that whitelist really is dealing with parents of expansion sets. The history of active expansion sets should contain the same information as the whitelist. Maybe my problem is about to evaporate!

everbeek commented 10 years ago

Came back after a vacation, and things looked alien. I had changed things to inspect the breadcrumb trail for expansions, and to find the crumb that has either a deletion for a node in question, or an expansion of said node that corresponds to the expansion we are checking the whitelist for. So, rather than being a whitelist, it is an inspection of history. By using the history directly, we work around the issue of undo/redo causing problems with an explicit whitelist.

There's a problem in that we need parent nodes in the logic, but the initial expansions have no node object available. I will have to do some late binding...but the logic could be reached before that parent node is bound to the expansion set. I've set it up with the parent node when it is first available, and it is working, but I suspect race conditions could develop in the path to root situation...yup, there is! Sorted it out. Assessing whether this issue is settled.

everbeek commented 10 years ago

Alright, I did mapping expansion on root, removed original ontology, then mapping expanded one of the remaining nodes. It brought back the root but didn't bring back the entire concept neighbourhood. The changes are working.

everbeek commented 10 years ago

Seeing what I can refactor to clean up the new system. Not much.