Fix API design issue with activating UI view items

The `on_activate()` function of an item should only be called when the
item was activated through the view, not through an external data change
(e.g. changing an active item through Python). That is important because
`on_activate()` is expected to do things like sending an undo push. This
is now respected in tree and grid views.

source/blender/editors/include/UI_grid_view.hh

@@ -56,11 +56,17 @@ class AbstractGridViewItem : public AbstractViewItem {
   /** See AbstractViewItem::matches(). */
   /* virtual */ bool matches(const AbstractViewItem &other) const override;
 
-  /** Called when the item's state changes from inactive to active. */
+  /**
+   * Called when the grid view changes an item's state from inactive to active. Will only be called
+   * if the state change is triggered through the grid view, not through external changes. E.g. a
+   * click on an item calls it, a change in the value returned by #should_be_active() to reflect an
+   * external state change does not.
+   */
   virtual void on_activate();
   /**
-   * If the result is not empty, it controls whether the item should be active or not,
-   * usually depending on the data that the view represents.
+   * If the result is not empty, it controls whether the item should be active or not, usually
+   * depending on the data that the view represents. Note that since this is meant to reflect
+   * externally managed state changes, #on_activate() will never be called if this returns true.
    */
   virtual std::optional<bool> should_be_active() const;
 
@@ -69,7 +75,10 @@ class AbstractGridViewItem : public AbstractViewItem {
 
   /**
    * Activates this item, deactivates other items, and calls the
-   * #AbstractGridViewItem::on_activate() function.
+   * #AbstractGridViewItem::on_activate() function. Should only be called when the item was
+   * activated through the grid view (e.g. through a click), not if the grid view reflects an
+   * external change (e.g. #AbstractGridViewItem::should_be_active() changes from returning false
+   * to returning true).
    * Requires the tree to have completed reconstruction, see #is_reconstructed(). Otherwise the
    * actual item state is unknown, possibly calling state-change update functions incorrectly.
    */
@@ -79,6 +88,12 @@ class AbstractGridViewItem : public AbstractViewItem {
  private:
   /** See #AbstractTreeView::change_state_delayed() */
   void change_state_delayed();
+  /**
+   * Like #activate() but does not call #on_activate(). Use it to reflect changes in the active
+   * state that happened externally.
+   * \return true of the item was activated.
+   */
+  bool set_state_active();
   static void grid_tile_click_fn(bContext *, void *but_arg1, void *);
   void add_grid_tile_button(uiBlock &block);
 };

source/blender/editors/include/UI_tree_view.hh

@@ -210,12 +210,16 @@ class AbstractTreeViewItem : public AbstractViewItem, public TreeViewItemContain
 
  protected:
   /**
-   * Called when the items state changes from inactive to active.
+   * Called when the tree view changes an item's state from inactive to active. Will only be called
+   * if the state change is triggered through the tree view, not through external changes. E.g. a
+   * click on an item calls it, a change in the value returned by #should_be_active() to reflect an
+   * external state change does not.
    */
   virtual void on_activate();
   /**
-   * If the result is not empty, it controls whether the item should be active or not,
-   * usually depending on the data that the view represents.
+   * If the result is not empty, it controls whether the item should be active or not, usually
+   * depending on the data that the view represents. Note that since this is meant to reflect
+   * externally managed state changes, #on_activate() will never be called if this returns true.
    */
   virtual std::optional<bool> should_be_active() const;
 
@@ -250,7 +254,10 @@ class AbstractTreeViewItem : public AbstractViewItem, public TreeViewItemContain
 
   /**
    * 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).
+   * function and ensures this item's parents are not collapsed (so the item is visible). Should
+   * only be called when the item was activated through the tree view (e.g. through a click), not
+   * if the tree view reflects an external change (e.g. #AbstractTreeViewItem::should_be_active()
+   * changes from returning false to returning true).
    * Requires the tree to have completed reconstruction, see #is_reconstructed(). Otherwise the
    * actual item state is unknown, possibly calling state-change update functions incorrectly.
    */
@@ -273,6 +280,12 @@ class AbstractTreeViewItem : public AbstractViewItem, public TreeViewItemContain
 
   /** See #AbstractTreeView::change_state_delayed() */
   void change_state_delayed();
+  /**
+   * Like #activate() but does not call #on_activate(). Use it to reflect changes in the active
+   * state that happened externally.
+   * \return true of the item was activated.
+   */
+  bool set_state_active();
 
   void add_treerow_button(uiBlock &block);
   int indent_width() const;

source/blender/editors/interface/views/grid_view.cc

@@ -186,28 +186,36 @@ void AbstractGridViewItem::change_state_delayed()
 {
   const std::optional<bool> should_be_active = this->should_be_active();
   if (should_be_active.has_value() && *should_be_active) {
-    activate();
+    /* Don't call #activate() here, since this reflects an external state change and therefore
+     * shouldn't call #on_activate(). */
+    set_state_active();
   }
 }
 
-void AbstractGridViewItem::activate()
+bool AbstractGridViewItem::set_state_active()
 {
   BLI_assert_msg(get_view().is_reconstructed(),
                  "Item activation can't be done until reconstruction is completed");
 
   if (!is_activatable_) {
-    return;
+    return false;
   }
   if (is_active()) {
-    return;
+    return false;
   }
 
-  /* Deactivate other items in the tree. */
+  /* Deactivate other items in the view. */
   get_view().foreach_item([](auto &item) { item.deactivate(); });
 
-  on_activate();
-
   is_active_ = true;
+  return true;
+}
+
+void AbstractGridViewItem::activate()
+{
+  if (set_state_active()) {
+    on_activate();
+  }
 }
 
 void AbstractGridViewItem::deactivate()

source/blender/editors/interface/views/tree_view.cc

@@ -494,26 +494,32 @@ int AbstractTreeViewItem::count_parents() const
   return i;
 }
 
-void AbstractTreeViewItem::activate()
+bool AbstractTreeViewItem::set_state_active()
 {
   BLI_assert_msg(get_tree_view().is_reconstructed(),
                  "Item activation can't be done until reconstruction is completed");
 
   if (!is_activatable_) {
-    return;
+    return false;
   }
   if (is_active()) {
-    return;
+    return false;
   }
 
   /* Deactivate other items in the tree. */
   get_tree_view().foreach_item([](auto &item) { item.deactivate(); });
-
-  on_activate();
   /* Make sure the active item is always visible. */
   ensure_parents_uncollapsed();
 
   is_active_ = true;
+  return true;
+}
+
+void AbstractTreeViewItem::activate()
+{
+  if (set_state_active()) {
+    on_activate();
+  }
 }
 
 void AbstractTreeViewItem::deactivate()
@@ -595,7 +601,9 @@ void AbstractTreeViewItem::change_state_delayed()
 {
   const std::optional<bool> should_be_active = this->should_be_active();
   if (should_be_active.has_value() && *should_be_active) {
-    activate();
+    /* Don't call #activate() here, since this reflects an external state change and therefore
+     * shouldn't call #on_activate(). */
+    set_state_active();
   }
 }