model_panel.tool._fill_tree() fails for complex model trees

[Tristan Croll]

To replicate:

from chimerax.core.models import Model
top_model = Model('test model', session)


def recursively_add_models(parent, depth=3, num=3):
    for i in range(num):
        m = Model('{},{}'.format(depth, i), session)
        if depth>0:
            recursively_add_models(m, depth=depth-1, num=num)
        parent.add([m])
                
recursively_add_models(top_model)
session.models.add([top_model])

new_top_model = Model('test', session)
new_top_model.add([top_model])
session.models.add([new_top_model])

This will fire a long string of tracebacks like the one below, but will eventually correctly fill the tree anyway. The problem as I see it is that core.models.add() fires the MODEL_ID_CHANGED trigger for every model in the tree, and as a recursive algorithm starts from the bottom and works up - so it's called for the children before the parent has been properly placed in the new tree. Deferring the MODEL_ID_CHANGED trigger until after all the models have been added (e.g. as per the diff below) gets rid of the tracebacks, but the repeated rebuilding of the tree still becomes slow for large trees - about 8 seconds in this case.

I note that even if I comment out the lines triggering MODEL_ID_CHANGED in models.add(), the ModelPanel still rebuilds its tree correctly due to the firing of the ADD_MODELS trigger. So the question is, is the firing of MODEL_ID_CHANGED superfluous here (or could it be reduced to a single firing passing the top-level parent Model)?

Error processing trigger "model id changed"
Traceback (most recent call last):
  File "/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/core/triggerset.py", line 126, in invoke
    return self._func(self._name, data)
  File "/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/model_panel/tool.py", line 69, in <lambda>
    lambda *args: self._fill_tree(*args, always_rebuild=True))
  File "/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/model_panel/tool.py", line 148, in _fill_tree
    parent = item_stack[0] if len(item_stack) == 1 else item_stack[len_id-1]
IndexError: list index out of range

diff --git a/src/core/models.py b/src/core/models.py
index acbe34b..eaa776c 100644
--- a/src/core/models.py
+++ b/src/core/models.py
@@ -293,7 +293,7 @@ class Models(State):
     def empty(self):
         return len(self._models) == 0
 
-    def add(self, models, parent=None, _notify=True, _from_session=False):
+    def add(self, models, parent=None, _notify=True, _need_fire_id_trigger=[], _from_session=False):
         start_count = len(self._models)
 
         d = self.drawing if parent is None else parent
@@ -302,11 +302,12 @@ class Models(State):
                 d.add_drawing(m)
 
         # Clear model ids if they are not subids of parent id.
-        need_fire_id_trigger = []
+        #~ if _notify:
+            #~ need_fire_id_trigger = []
         for model in models:
             if model.id and model.id[:-1] != d.id:
                 # Model has id that is not a subid of parent, so assign new id.
-                need_fire_id_trigger.append(model)
+                _need_fire_id_trigger.append(model)
                 del self._models[model.id]
                 model.id = None
                 if hasattr(model, 'parent'):
@@ -319,13 +320,7 @@ class Models(State):
             self._models[model.id] = model
             children = model.child_models()
             if children:
-                self.add(children, model, _notify=False)
-
-        # IDs that change from None to non-None don't fire the MODEL_ID_CHANGED
-        # trigger, so do it by hand
-        for id_changed_model in need_fire_id_trigger:
-            session = self._session()
-            session.triggers.activate_trigger(MODEL_ID_CHANGED, id_changed_model)
+                self.add(children, model, _notify=False, _need_fire_id_trigger=_need_fire_id_trigger)
 
         # Notify that models were added
         if _notify:
@@ -336,6 +331,12 @@ class Models(State):
                 m.added_to_session(session)
             session.triggers.activate_trigger(ADD_MODELS, m_add)
 
+            # IDs that change from None to non-None don't fire the MODEL_ID_CHANGED
+            # trigger, so do it by hand
+            for id_changed_model in _need_fire_id_trigger:
+                session = self._session()
+                session.triggers.activate_trigger(MODEL_ID_CHANGED, id_changed_model)
+
         # Initialize view if first model added
         if _notify and not _from_session and start_count == 0 and len(self._models) > 0:
             v = session.main_view

[Eric Pettersen]

Thanks for the patch. I tested and it looks good. It's committed to the code now.

--Eric