Fix T91940: Asset Browser catalogs continuously redraw

Issue was that the `on_activate()` callback of tree-items were
continuously called, because the active-state was queried before we
fully reconstructed the tree and its state from the previous redraw.
Such issues could happen in more places, so I've refactored the API a
bit to reflect the requirements for state queries, and include some
sanity checks.
The actual fix for the issue is to delay the state change until the tree
is fully reconstructed, by letting the tree-items pass a callback to
check if they should be active.

source/blender/editors/include/UI_tree_view.hh

@@ -139,11 +139,17 @@ class AbstractTreeView : public TreeViewItemContainer {
   friend TreeViewBuilder;
   friend TreeViewLayoutBuilder;
 
+  bool is_reconstructed_ = false;
+
  public:
   virtual ~AbstractTreeView() = default;
 
   void foreach_item(ItemIterFn iter_fn, IterOptions options = IterOptions::None) const;
 
+  /** Check if the tree is fully (re-)constructed. That means, both #build_tree() and
+   * #update_from_old() have finished. */
+  bool is_reconstructed() const;
+
  protected:
   virtual void build_tree() = 0;
 
@@ -156,6 +162,10 @@ class AbstractTreeView : public TreeViewItemContainer {
                                                  const TreeViewItemContainer &old_items);
   static AbstractTreeViewItem *find_matching_child(const AbstractTreeViewItem &lookup_item,
                                                    const TreeViewItemContainer &items);
+  /** Items may want to do additional work when state changes. But these state changes can only be
+   * reliably detected after the tree was reconstructed (see #is_reconstructed()). So this is done
+   * delayed. */
+  void change_state_delayed();
   void build_layout_from_tree(const TreeViewLayoutBuilder &builder);
 };
 
@@ -175,9 +185,15 @@ class AbstractTreeView : public TreeViewItemContainer {
 class AbstractTreeViewItem : public TreeViewItemContainer {
   friend class AbstractTreeView;
 
+ public:
+  using IsActiveFn = std::function<bool()>;
+
+ private:
   bool is_open_ = false;
   bool is_active_ = false;
 
+  IsActiveFn is_active_fn_;
+
  protected:
   /** This label is used for identifying an item (together with its parent's labels). */
   std::string label_{};
@@ -188,6 +204,7 @@ class AbstractTreeViewItem : public TreeViewItemContainer {
   virtual void build_row(uiLayout &row) = 0;
 
   virtual void on_activate();
+  virtual void is_active(IsActiveFn is_active_fn);
   virtual bool on_drop(const wmDrag &drag);
   virtual bool can_drop(const wmDrag &drag) const;
   /** Custom text to display when dragging over a tree item. Should explain what happens when
@@ -211,16 +228,29 @@ class AbstractTreeViewItem : public TreeViewItemContainer {
 
   const AbstractTreeView &get_tree_view() const;
   int count_parents() const;
-  /** Activates this item, deactivates other items, calls the #AbstractTreeViewItem::on_activate()
-   * function and ensures this item's parents are not collapsed (so the item is visible). */
-  void activate();
   void deactivate();
+  /** Must not be called before the tree was reconstructed (see #is_reconstructed()). Otherwise we
+   * can't be sure about the item state. */
   bool is_active() const;
   void toggle_collapsed();
+  /** Must not be called before the tree was reconstructed (see #is_reconstructed()). Otherwise we
+   * can't be sure about the item state. */
   bool is_collapsed() const;
   void set_collapsed(bool collapsed);
   bool is_collapsible() const;
   void ensure_parents_uncollapsed();
+
+ protected:
+  /** Activates this item, deactivates other items, calls the #AbstractTreeViewItem::on_activate()
+   * function and ensures this item's parents are not collapsed (so the item is visible).
+   * Must not be called before the tree was reconstructed (see #is_reconstructed()). Otherwise we
+   * can't be sure about the current item state and may call state-change update functions
+   * incorrectly. */
+  void activate();
+
+ private:
+  /** See #AbstractTreeView::change_state_delayed() */
+  void change_state_delayed();
 };
 
 /** \} */
@@ -242,7 +272,6 @@ class BasicTreeViewItem : public AbstractTreeViewItem {
   BasicTreeViewItem(StringRef label, BIFIconID icon = ICON_NONE);
 
   void build_row(uiLayout &row) override;
-  void on_activate() override;
   void on_activate(ActivateFn fn);
 
  protected:
@@ -255,6 +284,11 @@ class BasicTreeViewItem : public AbstractTreeViewItem {
 
   uiBut *button();
   BIFIconID get_draw_icon() const;
+
+ private:
+  static void tree_row_click_fn(struct bContext *C, void *arg1, void *arg2);
+
+  void on_activate() override;
 };
 
 /** \} */

source/blender/editors/interface/tree_view.cc

@@ -92,17 +92,20 @@ void AbstractTreeView::update_from_old(uiBlock &new_block)
 {
   uiBlock *old_block = new_block.oldblock;
   if (!old_block) {
+    /* Initial construction, nothing to update. */
+    is_reconstructed_ = true;
     return;
   }
 
   uiTreeViewHandle *old_view_handle = ui_block_view_find_matching_in_old_block(
       &new_block, reinterpret_cast<uiTreeViewHandle *>(this));
-  if (!old_view_handle) {
-    return;
-  }
+  BLI_assert(old_view_handle);
 
   AbstractTreeView &old_view = reinterpret_cast<AbstractTreeView &>(*old_view_handle);
   update_children_from_old_recursive(*this, old_view);
+
+  /* Finished (re-)constructing the tree. */
+  is_reconstructed_ = true;
 }
 
 void AbstractTreeView::update_children_from_old_recursive(const TreeViewItemContainer &new_items,
@@ -134,6 +137,19 @@ AbstractTreeViewItem *AbstractTreeView::find_matching_child(
   return nullptr;
 }
 
+bool AbstractTreeView::is_reconstructed() const
+{
+  return is_reconstructed_;
+}
+
+void AbstractTreeView::change_state_delayed()
+{
+  BLI_assert_msg(
+      is_reconstructed(),
+      "These state changes are supposed to be delayed until reconstruction is completed");
+  foreach_item([](AbstractTreeViewItem &item) { item.change_state_delayed(); });
+}
+
 /* ---------------------------------------------------------------------- */
 
 void AbstractTreeViewItem::on_activate()
@@ -141,6 +157,11 @@ void AbstractTreeViewItem::on_activate()
   /* Do nothing by default. */
 }
 
+void AbstractTreeViewItem::is_active(IsActiveFn is_active_fn)
+{
+  is_active_fn_ = is_active_fn;
+}
+
 bool AbstractTreeViewItem::on_drop(const wmDrag & /*drag*/)
 {
   /* Do nothing by default. */
@@ -186,6 +207,9 @@ int AbstractTreeViewItem::count_parents() const
 
 void AbstractTreeViewItem::activate()
 {
+  BLI_assert_msg(get_tree_view().is_reconstructed(),
+                 "Item activation can't be done until reconstruction is completed");
+
   if (is_active()) {
     return;
   }
@@ -207,11 +231,15 @@ void AbstractTreeViewItem::deactivate()
 
 bool AbstractTreeViewItem::is_active() const
 {
+  BLI_assert_msg(get_tree_view().is_reconstructed(),
+                 "State can't be queried until reconstruction is completed");
   return is_active_;
 }
 
 bool AbstractTreeViewItem::is_collapsed() const
 {
+  BLI_assert_msg(get_tree_view().is_reconstructed(),
+                 "State can't be queried until reconstruction is completed");
   return is_collapsible() && !is_open_;
 }
 
@@ -237,6 +265,13 @@ void AbstractTreeViewItem::ensure_parents_uncollapsed()
   }
 }
 
+void AbstractTreeViewItem::change_state_delayed()
+{
+  if (is_active_fn_()) {
+    activate();
+  }
+}
+
 /* ---------------------------------------------------------------------- */
 
 TreeViewBuilder::TreeViewBuilder(uiBlock &block) : block_(block)
@@ -247,6 +282,7 @@ void TreeViewBuilder::build_tree_view(AbstractTreeView &tree_view)
 {
   tree_view.build_tree();
   tree_view.update_from_old(block_);
+  tree_view.change_state_delayed();
   tree_view.build_layout_from_tree(TreeViewLayoutBuilder(block_));
 }
 
@@ -283,11 +319,10 @@ BasicTreeViewItem::BasicTreeViewItem(StringRef label, BIFIconID icon_) : icon(ic
   label_ = label;
 }
 
-static void tree_row_click_fn(struct bContext *UNUSED(C), void *but_arg1, void *UNUSED(arg2))
+void BasicTreeViewItem::tree_row_click_fn(struct bContext * /*C*/, void *but_arg1, void * /*arg2*/)
 {
   uiButTreeRow *tree_row_but = (uiButTreeRow *)but_arg1;
-  AbstractTreeViewItem &tree_item = reinterpret_cast<AbstractTreeViewItem &>(
-      *tree_row_but->tree_item);
+  BasicTreeViewItem &tree_item = reinterpret_cast<BasicTreeViewItem &>(*tree_row_but->tree_item);
 
   /* Let a click on an opened item activate it, a second click will close it then.
    * TODO Should this be for asset catalogs only? */

source/blender/editors/space_file/asset_catalog_tree_view.cc

@@ -151,9 +151,7 @@ ui::BasicTreeViewItem &AssetCatalogTreeView::build_catalog_items_recursive(
 {
   ui::BasicTreeViewItem &view_item = view_parent_item.add_tree_item<AssetCatalogTreeViewItem>(
       &catalog);
-  if (is_active_catalog(catalog.get_catalog_id())) {
-    view_item.activate();
-  }
+  view_item.is_active([this, &catalog]() { return is_active_catalog(catalog.get_catalog_id()); });
 
   catalog.foreach_child([&view_item, this](AssetCatalogTreeItem &child) {
     build_catalog_items_recursive(view_item, child);
@@ -171,9 +169,8 @@ void AssetCatalogTreeView::add_all_item()
     params->asset_catalog_visibility = FILE_SHOW_ASSETS_ALL_CATALOGS;
     WM_main_add_notifier(NC_SPACE | ND_SPACE_ASSET_PARAMS, nullptr);
   });
-  if (params->asset_catalog_visibility == FILE_SHOW_ASSETS_ALL_CATALOGS) {
-    item.activate();
-  }
+  item.is_active(
+      [params]() { return params->asset_catalog_visibility == FILE_SHOW_ASSETS_ALL_CATALOGS; });
 }
 
 void AssetCatalogTreeView::add_unassigned_item()
@@ -187,10 +184,8 @@ void AssetCatalogTreeView::add_unassigned_item()
     params->asset_catalog_visibility = FILE_SHOW_ASSETS_WITHOUT_CATALOG;
     WM_main_add_notifier(NC_SPACE | ND_SPACE_ASSET_PARAMS, nullptr);
   });
-
-  if (params->asset_catalog_visibility == FILE_SHOW_ASSETS_WITHOUT_CATALOG) {
-    item.activate();
-  }
+  item.is_active(
+      [params]() { return params->asset_catalog_visibility == FILE_SHOW_ASSETS_WITHOUT_CATALOG; });
 }
 
 bool AssetCatalogTreeView::is_active_catalog(CatalogID catalog_id) const