From 4e6c8e00fcd5d7812047a807bea6bf38b9e0b159 Mon Sep 17 00:00:00 2001
From: derammo <817160+derammo@users.noreply.github.com>
Date: Wed, 3 Aug 2022 18:08:16 -0400
Subject: [PATCH] fixed Tree UI control bug corrupting child cache

---
 scene/gui/tree.cpp | 34 ++++++++++++++++++++++++++++++++--
 scene/gui/tree.h   |  7 +++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/scene/gui/tree.cpp b/scene/gui/tree.cpp
index 1eb6c5a5544..ede7bfb0bfb 100644
--- a/scene/gui/tree.cpp
+++ b/scene/gui/tree.cpp
@@ -749,6 +749,7 @@ int TreeItem::get_child_count() {
 }
 
 Array TreeItem::get_children() {
+	// Don't need to explicitly create children cache, because get_child_count creates it.
 	int size = get_child_count();
 	Array arr;
 	arr.resize(size);
@@ -770,6 +771,22 @@ int TreeItem::get_index() {
 	return idx - 1;
 }
 
+#ifdef DEV_ENABLED
+void TreeItem::validate_cache() const {
+	if (!parent || parent->children_cache.is_empty()) {
+		return;
+	}
+	TreeItem *scan = parent->first_child;
+	int index = 0;
+	while (scan) {
+		DEV_ASSERT(parent->children_cache[index] == scan);
+		++index;
+		scan = scan->get_next();
+	}
+	DEV_ASSERT(index == parent->children_cache.size());
+}
+#endif
+
 void TreeItem::move_before(TreeItem *p_item) {
 	ERR_FAIL_NULL(p_item);
 	ERR_FAIL_COND(is_root);
@@ -797,7 +814,11 @@ void TreeItem::move_before(TreeItem *p_item) {
 		parent->children_cache.clear();
 	} else {
 		parent->first_child = this;
-		parent->children_cache.insert(0, this);
+		// If the cache is empty, it has not been built but there
+		// are items in the tree (note p_item != nullptr,) so we cannot update it.
+		if (!parent->children_cache.is_empty()) {
+			parent->children_cache.insert(0, this);
+		}
 	}
 
 	prev = item_prev;
@@ -807,6 +828,8 @@ void TreeItem::move_before(TreeItem *p_item) {
 	if (tree && old_tree == tree) {
 		tree->update();
 	}
+
+	validate_cache();
 }
 
 void TreeItem::move_after(TreeItem *p_item) {
@@ -839,12 +862,17 @@ void TreeItem::move_after(TreeItem *p_item) {
 	if (next) {
 		parent->children_cache.clear();
 	} else {
-		parent->children_cache.append(this);
+		// If the cache is empty, it has not been built but there
+		// are items in the tree (note p_item != nullptr,) so we cannot update it.
+		if (!parent->children_cache.is_empty()) {
+			parent->children_cache.append(this);
+		}
 	}
 
 	if (tree && old_tree == tree) {
 		tree->update();
 	}
+	validate_cache();
 }
 
 void TreeItem::remove_child(TreeItem *p_item) {
@@ -859,6 +887,7 @@ void TreeItem::remove_child(TreeItem *p_item) {
 	if (tree) {
 		tree->update();
 	}
+	validate_cache();
 }
 
 void TreeItem::set_selectable(int p_column, bool p_selectable) {
@@ -1396,6 +1425,7 @@ TreeItem::TreeItem(Tree *p_tree) {
 
 TreeItem::~TreeItem() {
 	_unlink_from_tree();
+	validate_cache();
 	prev = nullptr;
 	clear_children();
 	_change_tree(nullptr);
diff --git a/scene/gui/tree.h b/scene/gui/tree.h
index f0819e2980a..bcc2419b80a 100644
--- a/scene/gui/tree.h
+++ b/scene/gui/tree.h
@@ -342,6 +342,13 @@ public:
 	Array get_children();
 	int get_index();
 
+#ifdef DEV_ENABLED
+	// This debugging code can be removed once the current refactoring of this class is complete.
+	void validate_cache() const;
+#else
+	void validate_cache() const {}
+#endif
+
 	void move_before(TreeItem *p_item);
 	void move_after(TreeItem *p_item);