I am not sure this makes a lot of sense. We use the underscore as a prefix to notify a function is private or protected, not if it's virtual or not. The Control::can_drop_data for example is public, I don't think it should be changed.

I mean, we can discuss if we want to change our naming rules, but right now it seems to me that this PR makes things less consistent than they are now.

I am not sure this makes a lot of sense. We use the underscore as a prefix to notify a function is private or protected, not if it's virtual or not.

In GDScript it denotes virtual methods. Check methods like _process() or _input(). They aren't private, they exist only in bindings. The PR changes only binding names.

Also some classes bind these methods for internal use, which results in duplicate docs that shouldn't exist. See #47260 (comment) for example. Prefixing with underscore fixes this problem, because non-virtual methods starting with underscore are hidden by default.

In GDScript it denotes virtual methods. Check methods like _process() or _input(). They aren't private, they exist only in bindings. The PR changes only binding names.

Well, yeah, in a way they are private. Of course in GDscript there's no real notion of public or private, but the goal of those functions here is internal to the node, and thus they are not supposed to be called from outside of it. Which kind of make them "private".

On the other hand, Control::can_drop_data is supposed to be called from outside the node, so it makes sense it has no underscore prefix.

The documentation issue is another problem though, maybe we should have usage flag that does not put the function in the documentation?

On the other hand, Control::can_drop_data is supposed to be called from outside the node, so it makes sense it has no underscore prefix.

Well, in such cases we separate the public method that is called vs the class specific implementation that is prefixed. See Object's get_property_list/_get_property_list. There is of course a little bit more going under the hood, but as far as API design goes, this is a good example.

Well, in such cases we separate the public method that is called vs the class specific implementation that is prefixed. See Object's get_property_list/_get_property_list. There is of course a little bit more going under the hood, but as far as API design goes, this is a good example.

Indeed. Though that kind of makes my point, as I believe it makes sense to keep methods without the underscore prefix as public, and internal stuff with a prefix. Here both function are clearly different (even if the naming is similar), since one will return all properties (get_property_list) and the other should only return the properties of the given class/script (_get_property_list).

We discussed this in a PR review meeting today.

We had a lengthy discussion on the public/protected status of those methods and whether there should be public (non virtual) methods bound that match the protected (virtual, with underscore prefix), so that you still have e.g. can_drop_data() exposed as the public method you can call from scripts, and _can_drop_data() as the virtual method to implement which is called by can_drop_data().

But it turns out that all the methods which are bound as virtual are actually not public methods and can't be called from script, so this change is correct to make it even more obvious (you can't call some_control.can_drop_data() currently).

There are still some cases where a public non-virtual method is bound, such as get_property_list() (which also calls _get_property_list() if defined). But those are already bound properly and not changed in this PR.

TL;DR: The change is good.

Thanks!

Uh , I just noticed that set_window_layout has no prefix. Looks like some new methods were added and I didn't catch it in rebase ;_;