fix: prevent exceptions when multiple connection approval are used [MTT-3496]

[jeffreyrainy]

This logs an error and prevents the addition of further connection approval callbacks.

[MTT-3496]

Changelog

• Changed: NetworkManager.ConnectionApprovalCallback is now a delegate instead of an event. Adding a second one is forbidden and an error is generated if client code attempts it.

[jeffreyrainy]

#1941

[0xFA11]

first of all, if we'd like to use this as a chance to tidy up a few things around connection approval stuff, I'd like to start with suggesting this:

https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-classes-structs-and-interfaces#names-of-common-types

✔️ DO add the suffix "EventHandler" to names of delegates that are used in events.
✔️ DO add the suffix "Callback" to names of delegates other than those used as event handlers.
❌ DO NOT add the suffix "Delegate" to a delegate.

[0xFA11]

but also on the other hand, I'm seeing 2 different ConnectionApprovedDelegate & ConnectionApprovalDelegate delegates defined.

I guess, one is for the actual approve/deny logic and other one is for just post-approval notification, if I understand things correctly.

so in that case, the first one that executes approve/deny logic would be a handler/callback and the other one that notifies listeners would be an event.

nevertheless, I feel like we should have a quick tech discussion and alignment on what we'd like to do here to tidy up connection approval stuff in general and use or very last change to leave public API with a better shape & outlook.

[NoelStephensUnity]

but also on the other hand, I'm seeing 2 different ConnectionApprovedDelegate & ConnectionApprovalDelegate delegates defined.

I guess, one is for the actual approve/deny logic and other one is for just post-approval notification, if I understand things correctly.

so in that case, the first one that executes approve/deny logic would be a handler/callback and the other one that notifies listeners would be an event.

nevertheless, I feel like we should have a quick tech discussion and alignment on what we'd like to do here to tidy up connection approval stuff in general and use or very last change to leave public API with a better shape & outlook.

Yeah, the naming was odd but one is what the user subscribes to and the other is what the user calls to finish approval.
The hesitation in the naming was that the initial public API was ConnectionApprovalCallback so in order to not break anything it makes sense to keep it that way.

But moving forward the naming convention rules are good to review over.

[jeffreyrainy]

Superseded by #1972