Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Add way to check if a signal has any connections #87344

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jan 18, 2024

Added to Object and Signal

Went with any_connected because it felt clear and direct as a description, could use has_connections too if desired

Will see about adding some unit tests for this but we don't have unit tests for is_connected so unsure how to approach that

Unsure if I need to do anything special on the C# side, looked at the Signal class and didn't see anything to add

@Mickeon
Copy link
Contributor

Mickeon commented Jan 18, 2024

Unsure about any_connected because it is more... "distorted" when reading through it in your own codebase, compared to has_connections. To me it does look fine as a Signal method (e.g. $Button.pressed.any_connected()), but not so much for Object ($Button.any_connected("pressed"))

@AThousandShips
Copy link
Member Author

I'd prefer the name to be uniform on both, but I think there's no difference in reading is_connected on Object

@aaronfranke
Copy link
Member

I think has_connections is clearer than any_connected. The latter seems like it would accept an array of Callables or a vararg of Callables.

@AThousandShips
Copy link
Member Author

I'd say that both could be seen that way, but I'll change the naming from the consensus, I've got no strong feelings either way

@AThousandShips AThousandShips force-pushed the signal_connected branch 2 times, most recently from a6b3ce6 to f7a58e6 Compare January 18, 2024 21:20
@AThousandShips
Copy link
Member Author

Might go with is_any_connected instead later, but we'll see

@ShlomiRex
Copy link
Contributor

Suggestion:

Change current function is_connected(name) to is_connected_to(name)

Now you can also give the function any_connected() change to is_connected()

for more elegant naming

@Mickeon
Copy link
Contributor

Mickeon commented Jan 19, 2024

No changing existing function names, it would break compatibility.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is completely fine.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 21, 2024
@akien-mga akien-mga merged commit dd7cb05 into godotengine:master Sep 21, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the signal_connected branch September 21, 2024 10:00
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'is_connected()' function without parameters to check if signal is connected
5 participants