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

Free submenu children when clearing PopupMenu #79965

Merged
merged 1 commit into from Sep 25, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 27, 2023

While doing something else, I noticed that PopupMenu doesn't free submenu children when calling clear(). This means for example that the Create New menu in filesystem is created every time you right-click, which results in infinite unused menus.

@KoBeWi KoBeWi added this to the 4.2 milestone Jul 27, 2023
@KoBeWi KoBeWi requested review from a team as code owners July 27, 2023 17:52
@YuriSizov
Copy link
Contributor

I don't like this being the default and only option (which also makes this change compatibility breaking). It makes sense in the editor codebase, and in other code-first cases, but it conflicts with the node-first approach that some of our users prefer.

Say, you have a popup menu with a submenu, and you add both nodes in the editor UI. In the current form, what you see is what you get, and clearing the parent menu won't remove the submenu. You would only need to connect it as one of the items, which you need to do from code anyway.

With the proposed change clearing the items of the parent menu will remove the node that you've added to the tree visually. You will no longer get what you see, and you will have to recreate it from code every time. This just makes things harder for no reason, and it wastes a tiny bit of time recreating nodes.

As we discussed before, a flag can be added to the clear() method instead. I think it's still not an ideal solution, as we'd still be capable of missing the bugs this is trying to address. But I guess it's easier to spot if in the editor code we are forgetting to call clear() with true, which we would almost always want to do. So unless a better solution is suggested, the flag (off by default) would be acceptable.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 25, 2023

I could add a flag and make it on by default in C++ and off by default in GDScript (it's possible).

@YuriSizov
Copy link
Contributor

I could add a flag and make it on by default in C++ and off by default in GDScript (it's possible).

Aye, it's possible, but it may be frowned upon. I remember we discussed a case like that before, though I don't recall any details.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 25, 2023

We use this for Node methods with internal children argument.

@YuriSizov
Copy link
Contributor

Ah, right, that was it. Well, that works as well then.

@KoBeWi KoBeWi requested a review from a team as a code owner August 25, 2023 12:08
@YuriSizov YuriSizov requested review from a team and removed request for a team August 25, 2023 13:00
@YuriSizov YuriSizov requested a review from a team August 25, 2023 13:02
@akien-mga
Copy link
Member

Needs a rebase, then I guess it should be ready to merge.

@KoBeWi KoBeWi requested review from a team as code owners September 25, 2023 14:45
@akien-mga akien-mga merged commit 9142057 into godotengine:master Sep 25, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the popup_menu_sidequest branch September 25, 2023 15:30
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.

None yet

4 participants