PopupMenu: add_submenu() documentation does not tell that submenu should already exist

[Houkime]

Godot version

3.3.2 stable AND v4.0.dev.custom_build [a0d800e]

System information

Arch Linux (kernel 5.12.15-arch1-1, X11)

Issue description

PopupMenu's add_submenu_item() description reads:

Adds an item that will act as a submenu of the parent PopupMenu node when clicked. The submenu argument is the name of the child PopupMenu node that will be shown when the item is clicked.

An id can optionally be provided. If no id is provided, one will be created from the index.

First times i read this, i thought that this method will actually create the PopupMenu subnode, and the name here is so that i can access the child node like this (wow, github actually has a dedicated gdscript highlighting):

extends MenuButton

func _ready():
	get_popup().add_submenu_item("submenu label","submenu")
	get_popup().get_node("submenu").add_item("submenu item")

On 4.0 this throws a misleading (and buggy) error of previously freed instance (#48891), which makes things even worse, since now i think that my submenu WAS created but some incredibly evil bugforce insta-erased it.

It took me a trip to the godot sources to find an example of how godot editor itself does this (editor_node.cpp).

vcs_actions_menu = VersionControlEditorPlugin::get_singleton()->get_version_control_actions_panel();
vcs_actions_menu->set_name("Version Control");
vcs_actions_menu->connect("index_pressed", callable_mp(this, &EditorNode::_version_control_menu_option));
p->add_separator();
p->add_child(vcs_actions_menu);
p->add_submenu_item(TTR("Version Control"), "Version Control");
vcs_actions_menu->add_item(TTR("Set Up Version Control"), RUN_VCS_SETTINGS);
vcs_actions_menu->add_item(TTR("Shut Down Version Control"), RUN_VCS_SHUT_DOWN);

So, the right way to do this is :

extends MenuButton

func _ready():
	var submenu = PopupMenu.new()
	submenu.set_name("submenu")
	submenu.add_item("submenu item")
	get_popup().add_child(submenu)
	get_popup().add_submenu_item("submenu label","submenu")

The documentation (or, ideally, function's name) should reflect the fact that it is the reader's job to get a correctly named submenu there. Example code like above (but above is written from the MenuButton point of view) would also be nice.

Steps to reproduce

• Try to forget all you know as a Godot dev.
• Look at the documentation of add_submenu_item().

Minimal reproduction project

No response

[Houkime]

On the improvement side of things, comparing my expectations and how it actually works, it is hard not to notice that something like this would be a much easier way:

submenu = add_submenu_item("submenu label")
submenu.add_item("submenu item")

Maybe this is how it should actually work?
This said, i see the uses for linking submenus vs growing them out, and both ways are probably useful.
In this situation one might remember that this will take only a few gdscript lines to make a popup menu derivative that has this added streamlined interface, so under godot improvement rules, this might not be worth it.

That said, depending on how it is used inside the godot itself, this added method can be a good refactoring to improve the editor creation for example, and then the only thing left would be to add one line to bind it to gdscript.

[EricEzaM]

Yeah having to reference by the nodes name feels pretty yuck.

[kalshen2018]

Easily misunderstood and difficult to use.it took me two hours to test.

[jsjtxietian]

When deal with another bug, I found the doc is easily misunderstood for me too. Let's fix it.