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

Add methods to add submenus without using names #85477

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 28, 2023

Inspired by #84668

This PR adds add_direct_submenu_item() and related methods. It takes a PopupMenu directly and does not rely on its name. Also automatically adds the menu as a child if it doesn't have a parent. Refactored PopupMenu internals to not use submenu names anywhere, they are only kept for compatibility. The old methods are now deprecated.

There is some global_menu_add_submenu_item() method in DisplayServer, I did not touch it. I also changed editor usages to the new methods (except one case in AnimationNodeStateMachineEditor) and removed menu names, because they are no longer needed (this technically reverts #84617).

Note that when using the old methods, they are now required to have the submenu already added as a child (it allows to call the new method and simplify the code). This was actually documented, but wasn't enforced.

The new methods might need some testing. I did check if editor works correctly and tested some internal popups, but did not test the methods extensively (especially together with the old methods).

EDIT:
Fixes #84692

@KoBeWi KoBeWi added this to the 4.3 milestone Nov 28, 2023
@KoBeWi KoBeWi requested review from a team as code owners November 28, 2023 16:38
@KoBeWi KoBeWi force-pushed the submenus_that_shall_not_be_named branch from 694a53c to 298d0f6 Compare November 28, 2023 16:40
@KoBeWi KoBeWi force-pushed the submenus_that_shall_not_be_named branch from 298d0f6 to 50f1438 Compare November 28, 2023 22:09
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some global_menu_add_submenu_item() method in DisplayServer, I did not touch it.

This should not be changed, global menus can exist without node, so they're always using string IDs (for attached PopupMenus it's __PopupMenu#{INSTANCE_ID} so it's not directly related to PopupMenu submenu string).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. Didn't test.

doc/classes/PopupMenu.xml Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the submenus_that_shall_not_be_named branch from 42fa4c5 to aeec3c1 Compare February 22, 2024 14:14
@akien-mga akien-mga merged commit 42a15bc into godotengine:master Feb 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the submenus_that_shall_not_be_named branch February 23, 2024 11:37
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.

Executing PopupMenu.activate_item_by_event function crashes Godot
4 participants