Fix node names of submenu items across the editor

[YuriSizov]

Fixes #84582, a regression from #84183. Also removes some programmer remarks and fixes some docs.

There is still an open question whether this validation is excessive or not, whether we should allow generated names here or not. We might want that, which means we need a second version of validate_node_name which allows @ specifically. But I would argue that we want the user to name their node here. And the case of the C# module was the only place where we forgot to do that.

[akien-mga]

lol

[akien-mga]

I don't know what's the preferred style but I notice the AddChild function above does this inline:

Suggested change

	_menuPopup = new PopupMenu
	{
	Name = "CSharpTools",
	};
	_menuPopup = new PopupMenu { Name = "CSharpTools" };

[YuriSizov]

Maybe it's supposed to be inlined when it's just one property? cc @raulsntos

[raulsntos]

Yes, I think we tend to inline them when it's just one property and the line is not overly long. But we don't really enforce this so we're probably not consistent everywhere. I think it's fine either way.

[akien-mga]

Looks good to me.

I was initially confused by the bug report / reason for enforcing a valid non-autogenerated name for PopupMenu, as usually all nodes in the end can do their work without hassle regardless of their name.

But looking at the API, I now understand that there's a two step process where PopupMenu must first be created, named, and added as child, for that name to then be passed as argument to add_submenu_item. Seems a bit weird to rely on names like this, but that's a pre-existing problem we won't solve here :)

[YuriSizov]

Seems a bit weird to rely on names like this, but that's a pre-existing problem we won't solve here :)

Yeah, something that we should've fixed in 4.0, but alas...

[Shilo]

Looks good. Thank you!

There is still an open question whether this validation is excessive or not, whether we should allow generated names here or not. We might want that, which means we need a second version of validate_node_name which allows @ specifically. But I would argue that we want the user to name their node here.

As for the open question. I think both changes are appropriate: allowing @UnamedNode... for submenus as well as enforcing the naming standard.

The reason I feel validation should allow unnamed nodes is because there are still many nodes in the editor that are unnamed (such as the top & run bar nodes). The consistency and validation seems wavy in those areas. But I am new to the source code so take my input with a grain of salt.

[akien-mga]

Thanks!