Expose Node child manipulation callbacks

[Spartan322]

Based on #43729, ported to 4.0, thank Xrayez for doing most of the work.

While #57541 solves godotengine/godot-proposals#724 and godotengine/godot-proposals#2544 using signals, it was suggested at #43729 (comment) this was still being considered had it been ported to 4.0. I took a bit of liberty with the original PR and kept it more in line with the other virtual methods.

This does slightly diverge in function from #57541 in that it is called with every call to add_child, move_child, and remove_child that succeeds regardless of the scene tree. While in GDScript you may be capable to override the add, move, and remove child methods, (whether that should be commonly recommended aside, by itself it does reduce this PR's value in GDScript only) but in a more static language like C# this would only work if the method was virtual, otherwise you'd only hide it when you cast to your known class.

Exposed Additions

virtual void _add_child_notify(child: Node)
virtual void _move_child_notify(child: Node)
virtual void _remove_child_notify(child: Node)

Usage

extends Node

func _add_child_notify(child): print("child was added")
func _move_child_notify(child): print("child was moved")
func _remove_child_notify(child): print("child was removed")

Also corrects Control and Window not calling the base add_child_notify and remove_child_notify methods.

Potential Considerations

• (This has since been addressed, TabContainer is currently consistent with other nodes) TabContainer is the only Node that hides an internal node, its TabBar internal node, from the add_child_notify, remove_child_notify, and move_child_notify methods resulting in inconsistent behavior compared to all other internal nodes including its own. Can be corrected by adding either to the internal TabBar if branch:
  1. Calling the CanvasItem add_child_notify, remove_child_notify, and move_child_notify so as to be consistent with previous function of not otherwise calling the Control base for the internal TabBar
  2. Directly calling the respective _add_child_notify, _remove_child_notify, _move_child_notify virtual methods
• Documentation may optimally need to note that _add_child_notify, _remove_child_notify, and _move_child_notify will otherwise include internal nodes.

Test Project:
NodeChildNotifyTestProject.zip

[akien-mga]

This was added recently, but for consistency I guess it could also get a GDVIRTUAL_CALL (and call to base class in Light2D and Light3D where it's overridden).

[Spartan322]

I can't seem to find where its overriden in Light2D and Light3D, I do see the add and remove notifies overriden in Window without making a base call, I'm supposing that needs to be corrected here as well. TabContainer is also unique in that the TabBar seems to be hidden as an internal node from this method, I believe all the other internal nodes will get notified by this since none of their parents' overrides the notify, unless the notify isn't being called on the other internal nodes for some reason. Probably could fix the internal TabBar being hidden from the add and document that all internal nodes will be passed to it if that's the case.

[Spartan322]

Actually I believe this is somewhat unique compared to the signal in that it gets called regardless of whether its in the tree or not? The child_enter_tree and child_exit_tree are only supposed to run when entering and exiting a scene tree since they propagate whereas this runs on every _add_child_nocheck (add_child), move_child, and remove_child call.

[akien-mga]

To be clear, I was commenting about owner_changed_notify:

$ rg owner_changed_notify
scene/3d/light_3d.h
88:     virtual void owner_changed_notify() override;

scene/3d/light_3d.cpp
179:void Light3D::owner_changed_notify() {

scene/main/node.h
212:    virtual void owner_changed_notify();

scene/main/node.cpp
431:void Node::owner_changed_notify() {
1559:   owner_changed_notify();

scene/2d/light_2d.cpp
33:void Light2D::owner_changed_notify() {

scene/2d/light_2d.h
76:     virtual void owner_changed_notify() override;

But it's good that it led you to double check code and find another child notify override in Window :)

[akien-mga]

I wonder if this should be called first or last as a general rule (I don't have the answer).

[Spartan322]

The derived Control nodes already generally do that, only TabContainer puts it lower then the top and that's only for its internal TabBar, but I suppose its not exactly a big deal either way.

[Diddykonga]

Should the exposed parts of these callbacks be named using past-tense verbiage?:
child_added
child_moved
child_removed

[YuriSizov]

Should the exposed parts of these callbacks be named using past-tense verbiage?: child_added child_moved child_removed

I don't think so, these are methods after all, not signals. But I'm not sure leaving them as *_notify for public facing API is good either. It's a bit of jargon for internal use, but it's not a clear name for users to understand.

[Spartan322]

I can agree with replacing the _notify with something else, only question is what the new names should be, comparing to most of the names of the other virtual methods would just do _add_child, but then it it be too easy to confuse with the regular child methods since its only a one character difference. How about something like _on_child_added then? Still clearly denotes its purpose and requires a four or five character typo at the beginning and end of the name to call over the child methods at least. (only possible confusion otherwise would be with the default style for signal calls, but since the default signal names are much longer and more specific, I'd say that makes it way more reasonable, unless we consider adding a signal version in which case that name is right out the window)

[Diddykonga]

Should the exposed parts of these callbacks be named using past-tense verbiage?: child_added child_moved child_removed

I don't think so, these are methods after all, not signals. But I'm not sure leaving them as *_notify for public facing API is good either. It's a bit of jargon for internal use, but it's not a clear name for users to understand.

While you are correct that these are methods, they are purposely callback methods, which for all intents of purposes is called in response to a Signal(an internal one). Therefore the naming convention for when auto-creating a callback method in GDScript for a Signal using the Editor could be used, to keep consistency:

ready() -> on_node_2d_ready() The type-name isnt required here, so its best without it.
ready() -> on_ready() This is clean and precise, and it is a common idiom to use the prefix on_ to denote a callback.

add_child() -> on_add_child()
move_child() -> on_move_child()
remove_child() -> on_remove_child()

[Spartan322]

While you are correct that these are methods, they are purposely callback methods, which for all intents of purposes is called in response to a Signal(an internal one).

Not really, even for all intents and purposes the calls to these methods are directly called by all the child manipulation methods, there's no signal system involved and that makes a big difference, that's really all a virtual method is supposed to be. And that's aside from the fact that all the virtual methods you use in gdscript are actually callbacks, the only real difference with these methods to say _ready is that you can call the method that calls this method directly, (whereas for _ready it needs to be added to a node of the scene tree) otherwise they act indistinguishable from any other virtual method.

Therefore the naming convention for when auto-creating a callback method in GDScript for a Signal using the Editor could be used

The standards for signals does not apply because they're not signals and they don't operate like signals, being a signal and being a callback are completely different even if they provided a similar purpose (and even when many signals are treated like one, but a callback is a simpler concept for which you could build signals from) and it will be muddying expectations and the style to name them as though they are signals. (especially when there is a similar signal that exists but only for something already in the scene tree)

Not to mention that it would be trivial for someone to add a signal through these functions already, perhaps someone contributes even to a PR that adds a signal to the notify methods (which wouldn't be such a bad idea given how many other virtual methods have a signal version, though maybe that could get confused with the scene tree specific signal) and now we're directly conflicting.

[YuriSizov]

We've actually discussed this PR in the today's review meeting, and the names were deemed fine as is.

What we have problem with, and it's not a fault of the author here, is that we currently have like 6 ways to react on children being added and removed, and one of them was even added just recently by @reduz. Juan also mentioned before that he was okay with this particular PR because it covers some cases not covered by other methods of extending related functionality. But we overall weren't convinced that we don't have a mess on our hands, a mess that can be simplified and streamlined to a few options instead of many.

So we are waiting another look from Juan at the moment.