Closed oltionchampari closed 2 years ago
I can reproduce this error in my tests. I could even shorten it by omitting the selection in the copy number ranking.
The interesting fact is that only replacing the view (i.e., without add + remove before) works, but with adding and removing it breaks. My assumption is that the ref
of the ViewWrapper is created and looked up by a generated static hash. Hence, the object ref is created when adding the first view and gets removed. Then, with the second opening CLUE tries to reuse the previous object ref. However, that ref is not available anymore.
I investigate this hypothesis and see if this is the cause.
I extended the generateHash()
function by a random hash, to see whether or not the object ref is reused.
function generateHash(desc: IPluginDesc, selection: ISelection) {
// const s = `${selection.idtype ? selection.idtype.id : ''}r${selection.ids}`;
const s = `${selection.idtype ? selection.idtype.id : ''}r${selection.ids}_${BaseUtils.randomId()}`;
return `${desc.id}_${s}`;
}
It turns out that with the random hash replacing the view after add + remove is working successfully! So my initial assumption seems to be confirmed.
Diffing the provenance graphs from before and after the change reveals that the object reference is not reused and the inverse action was added to the graph (which was previously not possible, due to the error within the replace action).
Added random id
Successfully added inverse action
Reused object ref with id 14
and new object ref with id 21
; below the inverse edges
21
(see last diff screenshot), but not used in the case without the random id?
- Why is the object ref added in both cases with id
21
(see last diff screenshot), but not used in the case without the random id?
The lookup (and reusage) is done in ProvenanceGraph.findInArray()
and executed in initAction()
.
Without the random id the same object with id 14
is used from the _objects
array.
- Can we turn off / change the find object ref in CLUE?
Most likely not. Hence, adding a random id is probably the only solution and we must check the effects as mentioned in question 1.
I check further how and when this bug could have been introduced and I couldn't find any recent change that could cause this error. The existingView
variable in the replaceViewImpl
is used and also the generate_hash()
function hasn't changed (beside using template strings) since the beginning.
Interestingly, the ViewWrapper
in tdp_core uses the default hash with ${name}_${category}
and creates this.ref
with graph.findOrAddObject()
.
I applied the graph.findOrAddObject()
and the generate_hash()
without the random id (so original state) to the Ordino ViewWrapper and this also seems to work!
- this.ref = ObjectRefUtils.objectRef(this, plugin.desc.name, ObjectRefUtils.category.visual, generateHash(plugin.desc, selection));
+ this.ref = graph.findOrAddObject(ObjectRefUtils.objectRef(this, plugin.desc.name, ObjectRefUtils.category.visual, generateHash(plugin.desc, selection)));
The interesting point here is that no additional object ref 21
is created and the object ref 14
is reused successfully.
All following node ids are decreased by one, due to the missing object ref 21
. The screenshot also shows that the replace action was successful by adding the inverse action.
I've tested the compatibility with existing provenance graphs, by exporting a session from Ordino Daily (without the ref change) and imported it locally (with the ref change) again. Additionally I've tested the Ordino Paper Teaser Figure. In my tests all actions like add, remove, replace views work without problems. Also jumping to a specific state and the inverse actions work for me.
I also imported the previously broken clue_add-remove-replace-fail.json.txt session and now Ordino continues and finishes the action to replace the view. The graph got extended by the previously missing inverse action. Users could continue that session now.
Environment
Steps to reproduce the bug
Observed Behavior
The application crashes and the views are unresponsive
Expected Behavior
User should be able to replace view
Screenshot