UI Code Quality: General refactor of Outliner View Layer display mode creation

See https://developer.blender.org/D9499.

* Turn functions into member functions (makes API for a type more obvious &
  local, allows implicitly sharing data through member variables, enables order
  independend definition of functions, allows more natural language for
  function names because of the obvious context).
* Move important variables to classes rather than passing around all the time
  (shorter, more task-focused code, localizes important data names).
* Add helper class for adding object children sub-trees (smaller, more focused
  units are easier to reason about, have higher coherence, better testability,
  can manage own resources easily with RAII).
* Use C++ iterators over C-macros (arguably more readable, less macros is
  generally preferred)
* Add doxygen groups (visually emphasizes the coherence of code sections,
  provide place for higher level comments on sections).
* Prefer references over pointers for passing by reference (makes clear that
  NULL is not a valid value and that the current scope is not the owner).

3 files changed, 229 insertions, 157 deletions

diff --git a/source/blender/editors/space_outliner/tree/tree_view.cc b/source/blender/editors/space_outliner/tree/tree_view.cc
index de9bc1237b5..8352a23080d 100644
--- a/source/blender/editors/space_outliner/tree/tree_view.cc
+++ b/source/blender/editors/space_outliner/tree/tree_view.cc
@@ -28,7 +28,7 @@ namespace outliner = blender::outliner;
 /* Convenience. */
 using blender::outliner::AbstractTreeView;
 
-TreeView *outliner_tree_view_create(eSpaceOutliner_Mode mode)
+TreeView *outliner_tree_view_create(eSpaceOutliner_Mode mode, SpaceOutliner *space_outliner)
 {
   AbstractTreeView *tree_view = nullptr;
 
@@ -40,7 +40,7 @@ TreeView *outliner_tree_view_create(eSpaceOutliner_Mode mode)
     case SO_ID_ORPHANS:
       break;
     case SO_VIEW_LAYER:
-      tree_view = new outliner::TreeViewViewLayer();
+      tree_view = new outliner::TreeViewViewLayer(*space_outliner);
       break;
   }
 
diff --git a/source/blender/editors/space_outliner/tree/tree_view.hh b/source/blender/editors/space_outliner/tree/tree_view.hh
index b7d71c0b608..57ee7c58c29 100644
--- a/source/blender/editors/space_outliner/tree/tree_view.hh
+++ b/source/blender/editors/space_outliner/tree/tree_view.hh
@@ -25,6 +25,7 @@
 struct bContext;
 struct ListBase;
 struct SpaceOutliner;
+struct TreeElement;
 struct TreeSourceData;
 
 #ifdef __cplusplus
@@ -34,17 +35,40 @@ namespace outliner {
 
 using Tree = ListBase;
 
+/* -------------------------------------------------------------------- */
+/* Tree-View Interface */
+
 class AbstractTreeView {
  public:
+  AbstractTreeView(SpaceOutliner &space_outliner) : _space_outliner(space_outliner)
+  {
+  }
   virtual ~AbstractTreeView() = default;
 
   /** Build a tree for this view and the current context. */
   virtual Tree buildTree(const TreeSourceData &source_data, SpaceOutliner &space_outliner) = 0;
+
+ protected:
+  SpaceOutliner &_space_outliner;
 };
 
-class TreeViewViewLayer : public AbstractTreeView {
+/* -------------------------------------------------------------------- */
+/* View Layer Tree-View */
+
+class TreeViewViewLayer final : public AbstractTreeView {
  public:
-  Tree buildTree(const TreeSourceData &source_data, SpaceOutliner &space_outliner) override final;
+  TreeViewViewLayer(SpaceOutliner &space_outliner);
+
+  Tree buildTree(const TreeSourceData &source_data, SpaceOutliner &space_outliner) override;
+
+ private:
+  ViewLayer *_view_layer;
+  bool _show_objects = true;
+
+  void add_view_layer(ListBase &, TreeElement &);
+  void add_layer_collections_recursive(ListBase &, ListBase &, TreeElement &);
+  void add_layer_collection_objects(ListBase &, LayerCollection &, TreeElement &);
+  void add_layer_collection_objects_children(TreeElement &);
 };
 
 }  // namespace outliner
@@ -67,7 +91,7 @@ typedef struct TreeSourceData {
   struct ViewLayer *view_layer;
 } TreeSourceData;
 
-TreeView *outliner_tree_view_create(eSpaceOutliner_Mode mode);
+TreeView *outliner_tree_view_create(eSpaceOutliner_Mode mode, SpaceOutliner *space_outliner);
 void outliner_tree_view_destroy(TreeView **tree_view);
 
 ListBase outliner_tree_view_build_tree(TreeView *tree_view,
diff --git a/source/blender/editors/space_outliner/tree/tree_view_view_layer.cc b/source/blender/editors/space_outliner/tree/tree_view_view_layer.cc
index 665e8d1954f..0d35db36e5b 100644
--- a/source/blender/editors/space_outliner/tree/tree_view_view_layer.cc
+++ b/source/blender/editors/space_outliner/tree/tree_view_view_layer.cc
@@ -26,6 +26,7 @@
 
 #include "BLI_ghash.h"
 #include "BLI_listbase.h"
+#include "BLI_listbase_wrapper.hh"
 
 #include "BLT_translation.h"
 
@@ -37,140 +38,100 @@
 namespace blender {
 namespace outliner {
 
-/**
- * For all objects in the tree, lookup the parent in this map,
- * and move or add tree elements as needed.
- */
-static void outliner_make_object_parent_hierarchy_collections(SpaceOutliner *space_outliner,
-                                                              GHash *object_tree_elements_hash)
-{
-  GHashIterator gh_iter;
-  GHASH_ITER (gh_iter, object_tree_elements_hash) {
-    Object *child = static_cast<Object *>(BLI_ghashIterator_getKey(&gh_iter));
-
-    if (child->parent == NULL) {
-      continue;
-    }
+/* Convenience/readability. */
+template<typename T> using List = ListBaseWrapper<T>;
 
-    ListBase *child_ob_tree_elements = static_cast<ListBase *>(
-        BLI_ghashIterator_getValue(&gh_iter));
-    ListBase *parent_ob_tree_elements = static_cast<ListBase *>(
-        BLI_ghash_lookup(object_tree_elements_hash, child->parent));
-    if (parent_ob_tree_elements == NULL) {
-      continue;
-    }
+class ObjectsChildrenBuilder {
+ public:
+  ObjectsChildrenBuilder(SpaceOutliner &outliner);
+  ~ObjectsChildrenBuilder();
 
-    LISTBASE_FOREACH (LinkData *, link, parent_ob_tree_elements) {
-      TreeElement *parent_ob_tree_element = static_cast<TreeElement *>(link->data);
-      TreeElement *parent_ob_collection_tree_element = NULL;
-      bool found = false;
+  void operator()(TreeElement &collection_tree_elem);
 
-      /* We always want to remove the child from the direct collection its parent is nested under.
-       * This is particularly important when dealing with multi-level nesting (grandchildren). */
-      parent_ob_collection_tree_element = parent_ob_tree_element->parent;
-      while (!ELEM(TREESTORE(parent_ob_collection_tree_element)->type,
-                   TSE_VIEW_COLLECTION_BASE,
-                   TSE_LAYER_COLLECTION)) {
-        parent_ob_collection_tree_element = parent_ob_collection_tree_element->parent;
-      }
+ private:
+  SpaceOutliner &_outliner;
+  GHash &_object_tree_elements_hash;
 
-      LISTBASE_FOREACH (LinkData *, link_iter, child_ob_tree_elements) {
-        TreeElement *child_ob_tree_element = static_cast<TreeElement *>(link_iter->data);
+  void object_tree_elements_lookup_create_recursive(TreeElement *te_parent);
+  void make_object_parent_hierarchy_collections();
+};
 
-        if (child_ob_tree_element->parent == parent_ob_collection_tree_element) {
-          /* Move from the collection subtree into the parent object subtree. */
-          BLI_remlink(&parent_ob_collection_tree_element->subtree, child_ob_tree_element);
-          BLI_addtail(&parent_ob_tree_element->subtree, child_ob_tree_element);
-          child_ob_tree_element->parent = parent_ob_tree_element;
-          found = true;
-          break;
-        }
-      }
+/* -------------------------------------------------------------------- */
+/** \name Tree View for a View Layer.
+ *
+ * \{ */
 
-      if (!found) {
-        /* We add the child in the tree even if it is not in the collection.
-         * We deliberately clear its sub-tree though, to make it less prominent. */
-        TreeElement *child_ob_tree_element = outliner_add_element(
-            space_outliner, &parent_ob_tree_element->subtree, child, parent_ob_tree_element, 0, 0);
-        outliner_free_tree(&child_ob_tree_element->subtree);
-        child_ob_tree_element->flag |= TE_CHILD_NOT_IN_COLLECTION;
-        BLI_addtail(child_ob_tree_elements, BLI_genericNodeN(child_ob_tree_element));
-      }
-    }
-  }
+TreeViewViewLayer::TreeViewViewLayer(SpaceOutliner &space_outliner)
+    : AbstractTreeView(space_outliner)
+{
 }
 
-/**
- * Build a map from Object* to a list of TreeElement* matching the object.
- */
-static void outliner_object_tree_elements_lookup_create_recursive(GHash *object_tree_elements_hash,
-                                                                  TreeElement *te_parent)
+Tree TreeViewViewLayer::buildTree(const TreeSourceData &source_data, SpaceOutliner &space_outliner)
 {
-  LISTBASE_FOREACH (TreeElement *, te, &te_parent->subtree) {
-    TreeStoreElem *tselem = TREESTORE(te);
+  Tree tree = {nullptr};
 
-    if (tselem->type == TSE_LAYER_COLLECTION) {
-      outliner_object_tree_elements_lookup_create_recursive(object_tree_elements_hash, te);
-    }
-    else if (tselem->type == 0 && te->idcode == ID_OB) {
-      Object *ob = (Object *)tselem->id;
-      ListBase *tree_elements = static_cast<ListBase *>(
-          BLI_ghash_lookup(object_tree_elements_hash, ob));
+  _view_layer = source_data.view_layer;
+  _show_objects = !(space_outliner.filter & SO_FILTER_NO_OBJECT);
 
-      if (tree_elements == NULL) {
-        tree_elements = static_cast<ListBase *>(MEM_callocN(sizeof(ListBase), __func__));
-        BLI_ghash_insert(object_tree_elements_hash, ob, tree_elements);
-      }
+  const bool show_children = (space_outliner.filter & SO_FILTER_NO_CHILDREN) == 0;
 
-      BLI_addtail(tree_elements, BLI_genericNodeN(te));
-      outliner_object_tree_elements_lookup_create_recursive(object_tree_elements_hash, te);
+  if (space_outliner.filter & SO_FILTER_NO_COLLECTION) {
+    /* Show objects in the view layer. */
+    for (Base *base : List<Base>(_view_layer->object_bases)) {
+      TreeElement *te_object = outliner_add_element(
+          &space_outliner, &tree, base->object, nullptr, 0, 0);
+      te_object->directdata = base;
+    }
+
+    if (show_children) {
+      outliner_make_object_parent_hierarchy(&tree);
     }
   }
-}
+  else {
+    /* Show collections in the view layer. */
+    TreeElement &ten = *outliner_add_element(
+        &space_outliner, &tree, source_data.scene, nullptr, TSE_VIEW_COLLECTION_BASE, 0);
+    ten.name = IFACE_("Scene Collection");
+    TREESTORE(&ten)->flag &= ~TSE_CLOSED;
 
-static void outliner_object_tree_elements_lookup_free(GHash *object_tree_elements_hash)
-{
-  GHASH_FOREACH_BEGIN (ListBase *, tree_elements, object_tree_elements_hash) {
-    BLI_freelistN(tree_elements);
-    MEM_freeN(tree_elements);
+    add_view_layer(ten.subtree, ten);
+    if (show_children) {
+      add_layer_collection_objects_children(ten);
+    }
   }
-  GHASH_FOREACH_END();
+
+  return tree;
 }
 
-static void outliner_add_layer_collection_objects(SpaceOutliner *space_outliner,
-                                                  ListBase *tree,
-                                                  ViewLayer *layer,
-                                                  LayerCollection *lc,
-                                                  TreeElement *ten)
+void TreeViewViewLayer::add_view_layer(ListBase &tree, TreeElement &parent)
 {
-  LISTBASE_FOREACH (CollectionObject *, cob, &lc->collection->gobject) {
-    Base *base = BKE_view_layer_base_find(layer, cob->ob);
-    TreeElement *te_object = outliner_add_element(space_outliner, tree, base->object, ten, 0, 0);
-    te_object->directdata = base;
+  /* First layer collection is for master collection, don't show it. */
+  LayerCollection *lc = static_cast<LayerCollection *>(_view_layer->layer_collections.first);
+  if (lc == nullptr) {
+    return;
+  }
 
-    if (!(base->flag & BASE_VISIBLE_VIEWLAYER)) {
-      te_object->flag |= TE_DISABLED;
-    }
+  add_layer_collections_recursive(tree, lc->layer_collections, parent);
+  if (_show_objects) {
+    add_layer_collection_objects(tree, *lc, parent);
   }
 }
 
-static void outliner_add_layer_collections_recursive(SpaceOutliner *space_outliner,
-                                                     ListBase *tree,
-                                                     ViewLayer *layer,
-                                                     ListBase *layer_collections,
-                                                     TreeElement *parent_ten,
-                                                     const bool show_objects)
+void TreeViewViewLayer::add_layer_collections_recursive(ListBase &tree,
+                                                        ListBase &layer_collections,
+                                                        TreeElement &parent_ten)
 {
-  LISTBASE_FOREACH (LayerCollection *, lc, layer_collections) {
+  for (LayerCollection *lc : List<LayerCollection>(layer_collections)) {
     const bool exclude = (lc->flag & LAYER_COLLECTION_EXCLUDE) != 0;
     TreeElement *ten;
 
-    if (exclude && ((space_outliner->show_restrict_flags & SO_RESTRICT_ENABLE) == 0)) {
-      ten = parent_ten;
+    if (exclude && ((_space_outliner.show_restrict_flags & SO_RESTRICT_ENABLE) == 0)) {
+      ten = &parent_ten;
     }
     else {
       ID *id = &lc->collection->id;
-      ten = outliner_add_element(space_outliner, tree, id, parent_ten, TSE_LAYER_COLLECTION, 0);
+      ten = outliner_add_element(
+          &_space_outliner, &tree, id, &parent_ten, TSE_LAYER_COLLECTION, 0);
 
       ten->name = id->name + 2;
       ten->directdata = lc;
@@ -186,73 +147,160 @@ static void outliner_add_layer_collections_recursive(SpaceOutliner *space_outlin
       }
     }
 
-    outliner_add_layer_collections_recursive(
-        space_outliner, &ten->subtree, layer, &lc->layer_collections, ten, show_objects);
-    if (!exclude && show_objects) {
-      outliner_add_layer_collection_objects(space_outliner, &ten->subtree, layer, lc, ten);
+    add_layer_collections_recursive(ten->subtree, lc->layer_collections, *ten);
+    if (!exclude && _show_objects) {
+      add_layer_collection_objects(ten->subtree, *lc, *ten);
     }
   }
 }
 
-static void outliner_add_view_layer(SpaceOutliner *space_outliner,
-                                    ListBase *tree,
-                                    TreeElement *parent,
-                                    ViewLayer *layer,
-                                    const bool show_objects)
+void TreeViewViewLayer::add_layer_collection_objects(ListBase &tree,
+                                                     LayerCollection &lc,
+                                                     TreeElement &ten)
 {
-  /* First layer collection is for master collection, don't show it. */
-  LayerCollection *lc = static_cast<LayerCollection *>(layer->layer_collections.first);
-  if (lc == NULL) {
-    return;
+  for (CollectionObject *cob : List<CollectionObject>(lc.collection->gobject)) {
+    Base *base = BKE_view_layer_base_find(_view_layer, cob->ob);
+    TreeElement *te_object = outliner_add_element(
+        &_space_outliner, &tree, base->object, &ten, 0, 0);
+    te_object->directdata = base;
+
+    if (!(base->flag & BASE_VISIBLE_VIEWLAYER)) {
+      te_object->flag |= TE_DISABLED;
+    }
   }
+}
 
-  outliner_add_layer_collections_recursive(
-      space_outliner, tree, layer, &lc->layer_collections, parent, show_objects);
-  if (show_objects) {
-    outliner_add_layer_collection_objects(space_outliner, tree, layer, lc, parent);
+void TreeViewViewLayer::add_layer_collection_objects_children(TreeElement &collection_tree_elem)
+{
+  /* Call helper to add children. */
+  ObjectsChildrenBuilder child_builder{_space_outliner};
+  child_builder(collection_tree_elem);
+}
+
+/** \} */
+
+/* -------------------------------------------------------------------- */
+/** \name Object Chilren helper.
+ *
+ * Helper to add child objects to the sub-tree of their parent, recursively covering all nested
+ * collections.
+ *
+ * \{ */
+
+ObjectsChildrenBuilder::ObjectsChildrenBuilder(SpaceOutliner &outliner)
+    : _outliner(outliner),
+      _object_tree_elements_hash(
+          *BLI_ghash_new(BLI_ghashutil_ptrhash, BLI_ghashutil_ptrcmp, __func__))
+{
+}
+
+ObjectsChildrenBuilder::~ObjectsChildrenBuilder()
+{
+  GHASH_FOREACH_BEGIN (ListBase *, tree_elements, &_object_tree_elements_hash) {
+    BLI_freelistN(tree_elements);
+    MEM_freeN(tree_elements);
   }
+  GHASH_FOREACH_END();
+
+  BLI_ghash_free(&_object_tree_elements_hash, nullptr, nullptr);
 }
 
-Tree TreeViewViewLayer::buildTree(const TreeSourceData &source_data, SpaceOutliner &space_outliner)
+void ObjectsChildrenBuilder::operator()(TreeElement &collection_tree_elem)
 {
-  Tree tree = {nullptr};
+  object_tree_elements_lookup_create_recursive(&collection_tree_elem);
+  make_object_parent_hierarchy_collections();
+}
 
-  if (space_outliner.filter & SO_FILTER_NO_COLLECTION) {
-    /* Show objects in the view layer. */
-    LISTBASE_FOREACH (Base *, base, &source_data.view_layer->object_bases) {
-      TreeElement *te_object = outliner_add_element(
-          &space_outliner, &tree, base->object, nullptr, 0, 0);
-      te_object->directdata = base;
+/**
+ * Build a map from Object* to a list of TreeElement* matching the object.
+ */
+void ObjectsChildrenBuilder::object_tree_elements_lookup_create_recursive(TreeElement *te_parent)
+{
+  for (TreeElement *te : List<TreeElement>(te_parent->subtree)) {
+    TreeStoreElem *tselem = TREESTORE(te);
+
+    if (tselem->type == TSE_LAYER_COLLECTION) {
+      object_tree_elements_lookup_create_recursive(te);
     }
+    else if (tselem->type == 0 && te->idcode == ID_OB) {
+      Object *ob = (Object *)tselem->id;
+      ListBase *tree_elements = static_cast<ListBase *>(
+          BLI_ghash_lookup(&_object_tree_elements_hash, ob));
 
-    if ((space_outliner.filter & SO_FILTER_NO_CHILDREN) == 0) {
-      outliner_make_object_parent_hierarchy(&tree);
+      if (tree_elements == nullptr) {
+        tree_elements = static_cast<ListBase *>(MEM_callocN(sizeof(ListBase), __func__));
+        BLI_ghash_insert(&_object_tree_elements_hash, ob, tree_elements);
+      }
+
+      BLI_addtail(tree_elements, BLI_genericNodeN(te));
+      object_tree_elements_lookup_create_recursive(te);
     }
   }
-  else {
-    /* Show collections in the view layer. */
-    TreeElement *ten = outliner_add_element(
-        &space_outliner, &tree, source_data.scene, nullptr, TSE_VIEW_COLLECTION_BASE, 0);
-    ten->name = IFACE_("Scene Collection");
-    TREESTORE(ten)->flag &= ~TSE_CLOSED;
-
-    bool show_objects = !(space_outliner.filter & SO_FILTER_NO_OBJECT);
-    outliner_add_view_layer(
-        &space_outliner, &ten->subtree, ten, source_data.view_layer, show_objects);
-
-    if ((space_outliner.filter & SO_FILTER_NO_CHILDREN) == 0) {
-      GHash *object_tree_elements_hash = BLI_ghash_new(
-          BLI_ghashutil_ptrhash, BLI_ghashutil_ptrcmp, __func__);
-      outliner_object_tree_elements_lookup_create_recursive(object_tree_elements_hash, ten);
-      outliner_make_object_parent_hierarchy_collections(&space_outliner,
-                                                        object_tree_elements_hash);
-      outliner_object_tree_elements_lookup_free(object_tree_elements_hash);
-      BLI_ghash_free(object_tree_elements_hash, nullptr, nullptr);
+}
+
+/**
+ * For all objects in the tree, lookup the parent in this map,
+ * and move or add tree elements as needed.
+ */
+void ObjectsChildrenBuilder::make_object_parent_hierarchy_collections()
+{
+  GHashIterator gh_iter;
+  GHASH_ITER (gh_iter, &_object_tree_elements_hash) {
+    Object *child = static_cast<Object *>(BLI_ghashIterator_getKey(&gh_iter));
+
+    if (child->parent == nullptr) {
+      continue;
     }
-  }
 
-  return tree;
+    ListBase *child_ob_tree_elements = static_cast<ListBase *>(
+        BLI_ghashIterator_getValue(&gh_iter));
+    ListBase *parent_ob_tree_elements = static_cast<ListBase *>(
+        BLI_ghash_lookup(&_object_tree_elements_hash, child->parent));
+    if (parent_ob_tree_elements == nullptr) {
+      continue;
+    }
+
+    for (LinkData *link : List<LinkData>(parent_ob_tree_elements)) {
+      TreeElement *parent_ob_tree_element = static_cast<TreeElement *>(link->data);
+      TreeElement *parent_ob_collection_tree_element = nullptr;
+      bool found = false;
+
+      /* We always want to remove the child from the direct collection its parent is nested under.
+       * This is particularly important when dealing with multi-level nesting (grandchildren). */
+      parent_ob_collection_tree_element = parent_ob_tree_element->parent;
+      while (!ELEM(TREESTORE(parent_ob_collection_tree_element)->type,
+                   TSE_VIEW_COLLECTION_BASE,
+                   TSE_LAYER_COLLECTION)) {
+        parent_ob_collection_tree_element = parent_ob_collection_tree_element->parent;
+      }
+
+      for (LinkData *link_iter : List<LinkData>(child_ob_tree_elements)) {
+        TreeElement *child_ob_tree_element = static_cast<TreeElement *>(link_iter->data);
+
+        if (child_ob_tree_element->parent == parent_ob_collection_tree_element) {
+          /* Move from the collection subtree into the parent object subtree. */
+          BLI_remlink(&parent_ob_collection_tree_element->subtree, child_ob_tree_element);
+          BLI_addtail(&parent_ob_tree_element->subtree, child_ob_tree_element);
+          child_ob_tree_element->parent = parent_ob_tree_element;
+          found = true;
+          break;
+        }
+      }
+
+      if (!found) {
+        /* We add the child in the tree even if it is not in the collection.
+         * We deliberately clear its sub-tree though, to make it less prominent. */
+        TreeElement *child_ob_tree_element = outliner_add_element(
+            &_outliner, &parent_ob_tree_element->subtree, child, parent_ob_tree_element, 0, 0);
+        outliner_free_tree(&child_ob_tree_element->subtree);
+        child_ob_tree_element->flag |= TE_CHILD_NOT_IN_COLLECTION;
+        BLI_addtail(child_ob_tree_elements, BLI_genericNodeN(child_ob_tree_element));
+      }
+    }
+  }
 }
 
+/** \} */
+
 }  // namespace outliner
 }  // namespace blender