Make `BaseButton::set_pressed()` emit `BaseButton`'s `pressed` signal

[kleonc]

Describe the project you are working on

Godot.

Describe the problem or limitation you are having in your project

(Noticed while investigating https://github.com/godotengine/godot/issues/53831, read comments in there for more context).

There's inconsistency with emitting signals when calling BaseButton::set_pressed() - it never emits BaseButton's pressed signal but it can emit other signals:		BaseButton::set_pressed()
can emit BaseButton::pressed	✖
can emit BaseButton::toggled	✔
can emit ButtonGroup::pressed	✔

As @pycbouh mentioned in https://github.com/godotengine/godot/issues/53831#issuecomment-944910582 it could have been desired to not auto-emit signal by calling BaseButton::set_pressed() to give more control to the developer. However, lately BaseButton::set_pressed_no_signal() was added and it seems to fulfill such use case:

BaseButton::set_pressed()	BaseButton::set_pressed_no_signal()
can emit BaseButton::pressed	✖	✖
can emit BaseButton::toggled	✔	✖
can emit ButtonGroup::pressed	✔	✖

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Hence I'm opting for making things consistent by changing the behavior to:		BaseButton::set_pressed()	BaseButton::set_pressed_no_signal()
can emit BaseButton::pressed	✔	✖
can emit BaseButton::toggled	✔	✖
can emit ButtonGroup::pressed	✔	✖

Things being consistent are in general more intuitive and easier to use so I think it's an improvement in the long term.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Currently:

• If I'd want both pressed and toggled to be emitted:

  func set_pressed(button: BaseButton, pressed: bool) -> void:
  var should_emit_pressed: bool = button.toggle_mode and not button.pressed and pressed
  button.pressed = pressed
  if should_emit_pressed:
      button.emit_signal("pressed")

• If I'd want neither pressed nor toggled to be emitted:

  func set_pressed(button: BaseButton, pressed: bool) -> void:
  button.set_pressed_no_signal(pressed)

  After suggested changes:

• If I'd want both pressed and toggled to be emitted:

  func set_pressed(button: BaseButton, pressed: bool) -> void:
  button.pressed = pressed

• If I'd want neither pressed nor toggled to be emitted:

  func set_pressed(button: BaseButton, pressed: bool) -> void:
  button.set_pressed_no_signal(pressed)

If this enhancement will not be used often, can it be worked around with a few lines of script?

Sure, see the snippets above. But it's all about the consistency, not about shortening the code.

Is there a reason why this should be core and not an add-on in the asset library?

It changes the behavior of in-core class.

[groud]

I am against this change as that is not consistent with what we usually do with our API. Signals notifying a changes are only triggered if they are caused by a user interaction, and not by API calls.

This helps a lot when implementing complex behaviors, as you may not want to execute the exact same things in the two different situations. In the current state, all of this can be worked around by simply manually calling BaseButton::emit_signal(...), after BaseButton::set_pressed(...), while with your proposal, distinguishing the two (without an added boolean) would not be possible.

[kleonc]

I am against this change as that is not consistent with what we usually do with our API. Signals notifying a changes are only triggered if they are caused by a user interaction, and not by API calls.

This helps a lot when implementing complex behaviors, as you may not want to execute the exact same things in the two different situations. In the current state, all of this can be worked around by simply manually calling BaseButton::emit_signal(...), after BaseButton::set_pressed(...), while with your proposal, distinguishing the two (without an added boolean) would not be possible.

The thing is the current state is also not consistent with the no auto-emission convention as other signals (BaseButton::toggled, ButtonGroup::pressed) can be trigerred by calling BaseButton::set_pressed(). Only BaseButton::pressed is not being auto-emitted. So currently if you'd be handling BaseButton::toggled you'd have the same problem with distinguishing whether it's trigerred by the user action or by you calling BaseButton::pressed(). As I've mentioned in https://github.com/godotengine/godot/issues/53831#issuecomment-944930194 I'd be fine with no auto-emitting if such convention would be indeed followed consistenly (currently it's not the case).

So an alternative change which would also make things consistent would be: making BaseButton::set_pressed() not trigger any signal emission, and removing BaseButton::set_pressed_no_signal() (added not so long ago so it shouldn't be problematic) as it would be redundant:

BaseButton::set_pressed()
can emit BaseButton::pressed	✖
can emit BaseButton::toggled	✖
can emit ButtonGroup::pressed	✖

[rcorre]

I agree with @kleonc. The fact that set_pressed_no_signal() exists suggests that set_pressed() will emit signals, and it is pretty surprising when it doesn't (depending on which signal you care about).

I ran into this when creating a group of "radio" buttons. Each one connects pressed to a function setting some data. I didn't use toggled because I don't care when one is released, since another pressed will always be called. I call set_pressed when creating the dialog to set a default button in each group, and was surprised this didn't signal (which meant my data structure didn't match the state of the buttons).