AcceptDialog.add_button() "action" argument is optional but has useless default value

[jgodfrey]

Godot version

v3.5.stable.official [991bb6a]

System information

Windows 10

Issue description

The signature of the add_button() method of the AcceptDialog is documented as follows:

Button add_button ( String text, bool right=false, String action="" )

And the action argument is documented as:

action will be passed to the custom_action signal when pressed.

So, it seems to be an optional argument with a default value of "". However, unless a non-empty string value is supplied for the action argument, the newly added button won't do anything. Looking at the code, that's because of this (code link):

if (p_action != "") {
    button->connect("pressed", this, "_custom_action", varray(p_action));
}

So, the button's pressed signal is only connected if the action argument is not an empty string (which, again, is the default).

I assume that argument is passed into the callback to allow differentiation of multiple custom actions that all filter through the same function. However, if only a single button is being created, it seems unnecessary.

I'm not sure which of the following is best here, though I'd lean towards no. 2...

1. Update the documentation to make it clear that the argument must be defined (and be non-empty)
2. Update the code to allow the button's pressed event to be wired regardless of the value of action
3. Update the code to define a non-empty default value for the argument, so it works "out of the box"

With the current documentation, the fact that it doesn't work unless a value is supplied for an apparently "optional" argument is confusing - as noted in this question.

Steps to reproduce

• Run the attached project
• Notice that pressing the Custom 1 button does nothing
• Notice that pressing the Custom 2 button prints a message to the console

The difference between the buttons is that the Custom 1 call to add_button does not define the action argument.

Minimal reproduction project

AcceptDialogIssue.zip

[heppocogne]

I'm agree with you, no.2 would be better.
By the way, we do not always have to use AcceptDialog's custom_action signal; we can also use Button's pressed signal even if action is empty.

[YuriSizov]

I don't understand what doesn't work here. This argument is tied specifically to the custom action signal. You need to pass a value to trigger this signal, yes. But you can also just use the button itself, which is returned by the method, and connect to it and do whatever you want.

The docs may need to be improved to reinforce the point that this is only a name of an action for the standardized custom action signal.

[Puzzling]

You have an explicitly optional parameter (has a default value) with unexpected results when you fail to supply a value. Yes, you can work around the issue as you mention but the combination of the declaration and the documentation leads to unexpected behaviour. At least it was certainly unexpected to me.

As the issue records, they can just update the documentation if they feel this is correct behaviour. At least that will avoid future support questions.

[YuriSizov]

@Puzzling What is unexpected about this behavior? If you create a button and do nothing with it, there is simply no behavior. If you want to have some behavior, you have to do something with the button. It is returned to you by this method, you have full control to do with it as you wish, just like any other button in the engine. The custom hook is only if you want to use the custom action behavior, but it is not required, which is why it is optional.

I'm not sure what would suggest here to anyone that doing nothing with the button should produce any effect, and why the lack of such effect is unexpected.

[Puzzling]

We disagree. The docs say that the passed action will be passed to the custom_action signal when pressed. A default value is still a value. The docs nothing about the action having to be non-empty or that the signal invocation is dependent on it being non-empty.

[YuriSizov]

Sure, you are technically correct, and as I mentioned we can clarify this technicality in the documentation.