Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix selection regression in EditorHelpSearch #87476

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jan 22, 2024

Investigated clearing the selection details on removing a TreeItem but think that's a bit of a larger investigation so keeping this restricted

The bug was caused by the removed item being currently selected, and then failing on re-select here:

godot/scene/gui/tree.cpp

Lines 2615 to 2617 in 0bcc0e9

if (!r_in_range && &selected_cell == &c) {
if (!selected_cell.selected || allow_reselect) {
selected_cell.selected = true;

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 22, 2024

My alternative fix was:

diff --git a/scene/gui/tree.cpp b/scene/gui/tree.cpp
index c67c3cd98d..fa22313d14 100644
--- a/scene/gui/tree.cpp
+++ b/scene/gui/tree.cpp
@@ -121,6 +121,9 @@ void TreeItem::_change_tree(Tree *p_tree) {

                if (tree->selected_item == this) {
                        tree->selected_item = nullptr;
+                       for (Cell &cell : cells) {
+                               cell.selected = false;
+                       }
                }

                if (tree->drop_mode_over == this) {

or

diff --git a/scene/gui/tree.h b/scene/gui/tree.h
index 2dda408dd7..23040e7e50 100644
--- a/scene/gui/tree.h
+++ b/scene/gui/tree.h
@@ -176,6 +176,9 @@ private:
                                parent->first_child = next;
                        }
                }
+               for (Cell &c : cells) {
+                       c.selected = false;
+               }
        }

        bool _is_any_collapsed(bool p_only_visible);

Which also solves this, unsure which is best but as that one has some potentially wider implications I kept it simple here

@YuriSizov
Copy link
Contributor

Which also solves this, unsure which is best but as that one has some potentially wider implications I kept it simple here

I prefer the local solution, so I think your PR is fine. As long as it doesn't produce any noticeable flickering.

@AThousandShips
Copy link
Member Author

It happens as it clears the tree so I don't think it will, didn't notice any myself (other than the existing rendering issues with open popups)

@YuriSizov YuriSizov merged commit 6fea273 into godotengine:master Jan 22, 2024
16 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@AThousandShips AThousandShips deleted the search_fix branch January 22, 2024 19:46
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open button gets disabled for some help classes when searching
2 participants