From dedd55df33c9433ae738e1bc3eebc6325f69c7f3 Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Wed, 26 Jul 2023 15:32:26 +0200 Subject: [PATCH] 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. --- .../blender/editors/include/UI_grid_view.hh | 23 +++++++++++++++---- .../blender/editors/include/UI_tree_view.hh | 21 +++++++++++++---- .../editors/interface/views/grid_view.cc | 22 ++++++++++++------ .../editors/interface/views/tree_view.cc | 20 +++++++++++----- 4 files changed, 65 insertions(+), 21 deletions(-) diff --git a/source/blender/editors/include/UI_grid_view.hh b/source/blender/editors/include/UI_grid_view.hh index 21493155bda..3562b5dcc61 100644 --- a/source/blender/editors/include/UI_grid_view.hh +++ b/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 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); }; diff --git a/source/blender/editors/include/UI_tree_view.hh b/source/blender/editors/include/UI_tree_view.hh index be074d816da..77d237649d5 100644 --- a/source/blender/editors/include/UI_tree_view.hh +++ b/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 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; diff --git a/source/blender/editors/interface/views/grid_view.cc b/source/blender/editors/interface/views/grid_view.cc index ccb3bd3ba07..d32c0b763cd 100644 --- a/source/blender/editors/interface/views/grid_view.cc +++ b/source/blender/editors/interface/views/grid_view.cc @@ -186,28 +186,36 @@ void AbstractGridViewItem::change_state_delayed() { const std::optional 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() diff --git a/source/blender/editors/interface/views/tree_view.cc b/source/blender/editors/interface/views/tree_view.cc index 1d25ad74254..f4bf91be8a3 100644 --- a/source/blender/editors/interface/views/tree_view.cc +++ b/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 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(); } }