[Bf-blender-cvs] [758f3f7456a] master: Fix T91940: Asset Browser catalogs continuously redraw
Julian Eisel
noreply at git.blender.org
Tue Oct 5 16:11:09 CEST 2021
Commit: 758f3f7456ac1e31f411c4ac1b19760ad6e5539c
Author: Julian Eisel
Date: Tue Oct 5 16:01:01 2021 +0200
Branches: master
https://developer.blender.org/rB758f3f7456ac1e31f411c4ac1b19760ad6e5539c
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.
===================================================================
M source/blender/editors/include/UI_tree_view.hh
M source/blender/editors/interface/tree_view.cc
M source/blender/editors/space_file/asset_catalog_tree_view.cc
===================================================================
diff --git a/source/blender/editors/include/UI_tree_view.hh b/source/blender/editors/include/UI_tree_view.hh
index 4028bee9e15..46eaf56a3c0 100644
--- a/source/blender/editors/include/UI_tree_view.hh
+++ b/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;
};
/** \} */
diff --git a/source/blender/editors/interface/tree_view.cc b/source/blender/editors/interface/tree_view.cc
index 63b12d4fc89..28c757ddc79 100644
--- a/source/blender/editors/interface/tree_view.cc
+++ b/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? */
diff --git a/source/blender/editors/space_file/asset_catalog_tree_view.cc b/source/blender/editors/space_file/asset_catalog_tree_view.cc
index ff8775155c2..291582dac08 100644
--- a/source/blender/editors/space_file/asset_catalog_tree_view.cc
+++ b/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) cons
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list