Closed everbeek closed 10 years ago
This problem could be eliminated if the ExpansionSetFilter did was not node oriented, but was expansion set oriented; as per the other filters, it has methods for nodes provided to it. I made the filter interface in a way that it supports only node iteration, and this forces the implementations to do trickier things to fulfill their contract. If I broke the filter interface, I could fix the problem in this issue, but I am very hesitant to do so; I would need to introduce an intermediate class, Filterable, to stand in for ExpansionSets, Nodes, and Ontologies. I feel like the implementational complexity is easier to cope with than yet another framework complexity. I am thinking about this...
I can avoid this change by trying to adjust how expansion sets are found from a node. I will defer filter interface changes then. To find a node's expansion set, I can do the same thing I recently implemented with regards to greying out the menu depending on whether they were expanded or not (#401). No...I think I should change how the filters work, to reify the filter's primary object of interest. Can I do this without tainting those objects with filter knowledge?
I am in the middle of an exploratory refactoring, changing filter generics to be the classes that they are actually filtering on. I am at the point where I am trying to gather the expansion sets that should have filters made for them. Clearly, those expansion sets in the redo stack are not among these, but what about expansion sets where all nodes have been deleted, and have been re-expanded? I have to understand this bug again before proceeding...
The bug was that re-expansions created "permanent" associations between later expansions, and these were not fixed when undoing expansions. By dealing with expansions directly, if we then undo an expansion, the node will have its proper association with its original expansion, and not be associated with the now undone re-expansion. Ok, good. But...the node is actually still associated with both if we use each expansion to find its constituents. This means that....when we trigger an expansion filter, we cannot filter the nodes associated with that expansion if a given node was deleted then re-expanded, because that node is rightfully hidden by the new expansion.
So maybe it's more like this; if a node is a part of more than one expansion, regardless of whether it was deleted, we will only hide it if all of its expansions are hidden. This essentially ignored the deletion for the purpose of filtering. Thinking...ok, it's easier to think about efficiency first...so for any hidden expansion set, I also have to search the other expansion sets for each node in the one we are hiding. This is not acceptable. So except for parents, nodes will always be hidden in the deletion-ignoring way described above. If a node comes back in via a different expansion, the original expansion will co-own the node, and they get to fight about whether it is visible or not.
Based on that, I will continue with the refactoring, because the current behavior is buggy, whereas that is just sub-optimal or perhaps meeting under-specified requirements.
Have it refactored. 0) Need to have the checkboxes update in a new way when different checkboxes are changed. [Fixed] 1) There seems to be slow down when rendering...calling the checkbox stuff often I think! [Fixed] 2) Parents of a displayed set are being hidden when I really want them shown still, even if their own parent expansion set is hidden. [New issue #419]
I fixed a problem with deleted expansion sets, and it became clear that I really need to fix #419 right away. Deleting a whole expansion set that preceded another leaves the second one with no root to have arcs attached to; we end up with a completely disconnected graph.
We filter out any nodes that are (currently) deleted from the graph. We always need to hold on to nodes that are deleted, since they could be re-added via an undo.
A gotchya here is that if a node is added via a different expansion operation, then this expansion set thinks it has the node, while the new expansion set thinks it has the node (and should) and the node will think it belongs to the new expansion set.
This is because nodes know their expansion set, and it would overwrite the original one (I believe) when a node was expanded again via a new expansion operation. BaseNode class has expansionSetAsMember and expansionSetAsParent fields, which are used to get from node to expansion sets during filtering operations.
In any case, these dangling references are both necessary and will cause misbehavior. Undoing an expansion will resolve part of it (not the node's believed expansion set parent though). Hiding the older expanion will hide the node even though it strictly belongs to the new expansion in this time and place.
One possible fix would be to filter expansion set operations on the basis of the node having a matching expansion set value. This is ok...I have to do a similar thing to check for deleted nodes anyway. These funny things are necessary if we don't want the expansion sets directly changed by node deletion operations, which they should not actually know about, given our undo/redo abilities. We'd have to manage explicit undo/redo additions and removal graph containers, views, and expansion sets otherwise!