griefith/test2
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Refactor: Deduplicate view item active state logic

Browse Source 
Much of this was duplicated between grid view and tree view items which
keeping them in sync was becoming a hassle already. Now the logic is
shared via the base class. I find this makes the interfaces easier to
scan through visually as well.

Had to add a virtual iterator function that can be called on an
`AbstractView`.
This commit is contained in:
Julian Eisel
2023-07-26 16:33:28 +02:00
parent 29aa89e4ec
commit 2e9bc6373c
7 changed files with 165 additions and 221 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
source/blender/editors/include/UI_abstract_view.hh
View File

@@ -88,6 +88,8 @@ class AbstractView {
virtual void draw_overlays(const ARegion &region) const;
virtual void foreach_view_item(FunctionRef<void(AbstractViewItem &)> iter_fn) const = 0;
/**
* Makes \a item valid for display in this view. Behavior is undefined for items not registered
* with this.
@@ -110,6 +112,15 @@ class AbstractView {
protected:
AbstractView() = default;
/**
* Items may want to do additional work when state changes. But these state changes can only be
* reliably detected after the view has completed reconstruction (see #is_reconstructed()). So
* the actual state changes are done in a delayed manner through this function.
*
* Overrides should call the base class implementation.
*/
virtual void change_state_delayed();
virtual void update_children_from_old(const AbstractView &old_view) = 0;
/**
@@ -151,6 +162,20 @@ class AbstractViewItem {
virtual void build_context_menu(bContext &C, uiLayout &column) const;
/**
* Called when the view changes an item's state from inactive to active. Will only be called if
* the state change is triggered through the 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. 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;
/**
* Queries if the view item supports renaming in principle. Renaming may still fail, e.g. if
* another item is already being renamed.
@@ -204,7 +229,17 @@ class AbstractViewItem {
bool is_interactive() const;
void disable_activatable();
/**
* Activates this item, deactivates other items, and calls the #AbstractViewItem::on_activate()
* function. Should only be called when the item was activated through the view (e.g. through a
* click), not if the view reflects an external change (e.g.
* #AbstractViewItem::should_be_active() changes from returning false to returning true).
*
* Requires the view to have completed reconstruction, see #is_reconstructed(). Otherwise the
* actual item state is unknown, possibly calling state-change update functions incorrectly.
*/
void activate();
void deactivate();
/**
* Requires the view to have completed reconstruction, see #is_reconstructed(). Otherwise we
* can't be sure about the item state.
@@ -240,6 +275,20 @@ class AbstractViewItem {
*/
virtual void update_from_old(const AbstractViewItem &old);
/**
* Like #activate() but does not call #on_activate(). Use it to reflect changes in the active
* state that happened externally.
* Can be overridden to customize behavior but should always call the base class implementation.
* \return true of the item was activated.
*/
virtual bool set_state_active();
/**
* See #AbstractView::change_state_delayed(). Overrides should call the base class
* implementation.
*/
virtual void change_state_delayed();
/**
* \note Do not call this directly to avoid constantly rechecking the filter state. Instead use
* #is_filtered_visible_cached() for querying.

41
source/blender/editors/include/UI_grid_view.hh
View File

@@ -56,44 +56,10 @@ class AbstractGridViewItem : public AbstractViewItem {
/** See AbstractViewItem::matches(). */
/* virtual */ bool matches(const AbstractViewItem &other) const override;
/**
* 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. 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;
/* virtual */ std::unique_ptr<DropTargetInterface> create_item_drop_target() final;
virtual std::unique_ptr<GridViewItemDropTarget> create_drop_target();
/**
* Activates this item, deactivates other items, and calls the
* #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 view to have completed reconstruction, see #is_reconstructed(). Otherwise the
* actual item state is unknown, possibly calling state-change update functions incorrectly.
*/
void activate();
void deactivate();
private:
/** See #AbstractGridView::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);
};
@@ -156,15 +122,10 @@ class AbstractGridView : public AbstractView {
virtual void build_items() = 0;
private:
void foreach_view_item(FunctionRef<void(AbstractViewItem &)> iter_fn) const final;
void update_children_from_old(const AbstractView &old_view) override;
AbstractGridViewItem *find_matching_item(const AbstractGridViewItem &item_to_match,
const AbstractGridView &view_to_search_in) const;
/**
* Items may want to do additional work when state changes. But these state changes can only be
* reliably detected after the view has completed reconstruction (see #is_reconstructed()). So
* the actual state changes are done in a delayed manner through this function.
*/
void change_state_delayed();
/**
* Add an already constructed item, moving ownership to the grid-view.

42
source/blender/editors/include/UI_tree_view.hh
View File

@@ -138,18 +138,13 @@ class AbstractTreeView : public AbstractView, public TreeViewItemContainer {
virtual void build_tree() = 0;
private:
void foreach_view_item(FunctionRef<void(AbstractViewItem &)> iter_fn) const final;
void update_children_from_old(const AbstractView &old_view) override;
static void update_children_from_old_recursive(const TreeViewOrItem &new_items,
const TreeViewOrItem &old_items);
static AbstractTreeViewItem *find_matching_child(const AbstractTreeViewItem &lookup_item,
const TreeViewOrItem &items);
/**
* Items may want to do additional work when state changes. But these state changes can only be
* reliably detected after the tree has completed reconstruction (see #is_reconstructed()). So
* the actual state changes are done in a delayed manner through this function.
*/
void change_state_delayed();
void draw_hierarchy_lines(const ARegion &region) const;
void draw_hierarchy_lines_recursive(const ARegion &region,
const TreeViewOrItem &parent,
@@ -209,20 +204,6 @@ class AbstractTreeViewItem : public AbstractViewItem, public TreeViewItemContain
bool is_collapsible() const;
protected:
/**
* 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. 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;
/** See AbstractViewItem::get_rename_string(). */
/* virtual */ StringRef get_rename_string() const override;
/** See AbstractViewItem::rename(). */
@@ -252,18 +233,6 @@ class AbstractTreeViewItem : public AbstractViewItem, public TreeViewItemContain
*/
virtual bool matches_single(const AbstractTreeViewItem &other) 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). 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.
*/
void activate();
void deactivate();
/**
* Can be called from the #AbstractTreeViewItem::build_row() implementation, but not earlier. The
* hovered state can't be queried reliably otherwise.
@@ -278,14 +247,11 @@ class AbstractTreeViewItem : public AbstractViewItem, public TreeViewItemContain
static void collapse_chevron_click_fn(bContext *, void *but_arg1, void *);
static bool is_collapse_chevron_but(const uiBut *but);
/** 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.
* Override of #AbstractViewItem::set_state_active() that also ensures the parents of this
* element are uncollapsed so that the item is visible.
*/
bool set_state_active();
bool set_state_active() final;
void add_treerow_button(uiBlock &block);
int indent_width() const;

28
source/blender/editors/interface/views/abstract_view.cc
View File

@@ -62,6 +62,34 @@ void AbstractView::update_from_old(uiBlock &new_block)
/** \} */
/* ---------------------------------------------------------------------- */
/** \name State Managment
* \{ */
void AbstractView::change_state_delayed()
{
BLI_assert_msg(
is_reconstructed(),
"These state changes are supposed to be delayed until reconstruction is completed");
/* Debug-only sanity check: Ensure only one item requests to be active. */
#ifndef NDEBUG
bool has_active = false;
foreach_view_item([&has_active](AbstractViewItem &item) {
if (item.should_be_active().value_or(false)) {
BLI_assert_msg(
!has_active,
"Only one view item should ever return true for its `should_be_active()` method");
has_active = true;
}
});
#endif
foreach_view_item([](AbstractViewItem &item) { item.change_state_delayed(); });
}
/** \} */
/* ---------------------------------------------------------------------- */
/** \name Default implementations of virtual functions
* \{ */

63
source/blender/editors/interface/views/abstract_view_item.cc
View File

@@ -32,6 +32,69 @@ void AbstractViewItem::update_from_old(const AbstractViewItem &old)
/** \} */
/* ---------------------------------------------------------------------- */
/** \name Active Item State
* \{ */
void AbstractViewItem::on_activate()
{
/* Do nothing by default. */
}
std::optional<bool> AbstractViewItem::should_be_active() const
{
return std::nullopt;
}
bool AbstractViewItem::set_state_active()
{
BLI_assert_msg(get_view().is_reconstructed(),
"Item activation can't be done until reconstruction is completed");
if (!is_activatable_) {
return false;
}
if (is_active()) {
return false;
}
/* Deactivate other items in the view. */
get_view().foreach_view_item([](auto &item) { item.deactivate(); });
is_active_ = true;
return true;
}
void AbstractViewItem::activate()
{
if (set_state_active()) {
on_activate();
}
}
void AbstractViewItem::deactivate()
{
is_active_ = false;
}
/** \} */
/* ---------------------------------------------------------------------- */
/** \name General State Management
* \{ */
void AbstractViewItem::change_state_delayed()
{
const std::optional<bool> should_be_active = this->should_be_active();
if (should_be_active.has_value() && *should_be_active) {
/* Don't call #activate() here, since this reflects an external state change and therefore
* shouldn't call #on_activate(). */
set_state_active();
}
}
/** \} */
/* ---------------------------------------------------------------------- */
/** \name Renaming
* \{ */

81
source/blender/editors/interface/views/grid_view.cc
View File

@@ -37,6 +37,14 @@ AbstractGridViewItem &AbstractGridView::add_item(std::unique_ptr<AbstractGridVie
return added_item;
}
/* Implementation for the base class virtual function. More specialized iterators below. */
void AbstractGridView::foreach_view_item(FunctionRef<void(AbstractViewItem &)> iter_fn) const
{
for (const auto &item_ptr : items_) {
iter_fn(*item_ptr);
}
}
void AbstractGridView::foreach_item(ItemIterFn iter_fn) const
{
for (const auto &item_ptr : items_) {
@@ -63,28 +71,6 @@ AbstractGridViewItem *AbstractGridView::find_matching_item(
return match ? *match : nullptr;
}
void AbstractGridView::change_state_delayed()
{
BLI_assert_msg(
is_reconstructed(),
"These state changes are supposed to be delayed until reconstruction is completed");
/* Debug-only sanity check: Ensure only one item requests to be active. */
#ifndef NDEBUG
bool has_active = false;
foreach_item([&has_active](AbstractGridViewItem &item) {
if (item.should_be_active().value_or(false)) {
BLI_assert_msg(
!has_active,
"Only one view item should ever return true for its `should_be_active()` method");
has_active = true;
}
});
#endif
foreach_item([](AbstractGridViewItem &item) { item.change_state_delayed(); });
}
void AbstractGridView::update_children_from_old(const AbstractView &old_view)
{
const AbstractGridView &old_grid_view = dynamic_cast<const AbstractGridView &>(old_view);
@@ -172,57 +158,6 @@ void AbstractGridViewItem::add_grid_tile_button(uiBlock &block)
UI_but_func_set(view_item_but_, grid_tile_click_fn, view_item_but_, nullptr);
}
void AbstractGridViewItem::on_activate()
{
/* Do nothing by default. */
}
std::optional<bool> AbstractGridViewItem::should_be_active() const
{
return std::nullopt;
}
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) {
/* Don't call #activate() here, since this reflects an external state change and therefore
* shouldn't call #on_activate(). */
set_state_active();
}
}
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 false;
}
if (is_active()) {
return false;
}
/* Deactivate other items in the view. */
get_view().foreach_item([](auto &item) { item.deactivate(); });
is_active_ = true;
return true;
}
void AbstractGridViewItem::activate()
{
if (set_state_active()) {
on_activate();
}
}
void AbstractGridViewItem::deactivate()
{
is_active_ = false;
}
AbstractGridView &AbstractGridViewItem::get_view() const
{
if (UNLIKELY(!view_)) {

80
source/blender/editors/interface/views/tree_view.cc
View File

@@ -73,6 +73,12 @@ void TreeViewItemContainer::foreach_item_recursive(ItemIterFn iter_fn, IterOptio
/* ---------------------------------------------------------------------- */
/* Implementation for the base class virtual function. More specialized iterators below. */
void AbstractTreeView::foreach_view_item(FunctionRef<void(AbstractViewItem &)> iter_fn) const
{
foreach_item_recursive(iter_fn);
}
void AbstractTreeView::foreach_item(ItemIterFn iter_fn, IterOptions options) const
{
foreach_item_recursive(iter_fn, options);
@@ -224,28 +230,6 @@ AbstractTreeViewItem *AbstractTreeView::find_matching_child(
return nullptr;
}
void AbstractTreeView::change_state_delayed()
{
BLI_assert_msg(
is_reconstructed(),
"These state changes are supposed to be delayed until reconstruction is completed");
/* Debug-only sanity check: Ensure only one item requests to be active. */
#ifndef NDEBUG
bool has_active = false;
foreach_item([&has_active](AbstractTreeViewItem &item) {
if (item.should_be_active().value_or(false)) {
BLI_assert_msg(
!has_active,
"Only one view item should ever return true for its `should_be_active()` method");
has_active = true;
}
});
#endif
foreach_item([](AbstractTreeViewItem &item) { item.change_state_delayed(); });
}
/* ---------------------------------------------------------------------- */
TreeViewItemDropTarget::TreeViewItemDropTarget(AbstractTreeView &view, DropBehavior behavior)
@@ -416,16 +400,6 @@ bool AbstractTreeViewItem::has_active_child() const
return found;
}
void AbstractTreeViewItem::on_activate()
{
/* Do nothing by default. */
}
std::optional<bool> AbstractTreeViewItem::should_be_active() const
{
return std::nullopt;
}
bool AbstractTreeViewItem::supports_collapsing() const
{
return true;
@@ -496,35 +470,13 @@ int AbstractTreeViewItem::count_parents() const
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 false;
}
if (is_active()) {
return false;
if (AbstractViewItem::set_state_active()) {
/* Make sure the active item is always visible. */
ensure_parents_uncollapsed();
return true;
}
/* Deactivate other items in the tree. */
get_tree_view().foreach_item([](auto &item) { item.deactivate(); });
/* 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()
{
is_active_ = false;
return false;
}
bool AbstractTreeViewItem::is_hovered() const
@@ -597,16 +549,6 @@ bool AbstractTreeViewItem::matches(const AbstractViewItem &other) const
return true;
}
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) {
/* Don't call #activate() here, since this reflects an external state change and therefore
* shouldn't call #on_activate(). */
set_state_active();
}
}
/* ---------------------------------------------------------------------- */
class TreeViewLayoutBuilder {