Improve addEvent when sharing events over multiple resources #2333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pirulax
reviewed
Sep 3, 2021
Pirulax
reviewed
Sep 3, 2021
…ding empty() method as well
… won't be that many resources and memory layout is preferred over raw speed
Pirulax
reviewed
Sep 3, 2021
Pirulax
reviewed
Sep 3, 2021
|
Totally forgot to respond here, I have changed my implementation according to your suggestions. |
sbx320
approved these changes
Sep 16, 2021
Lpsd
approved these changes
Dec 26, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2448
When adding event handlers for a custom event in multiple resources, you have to use addEvent to make sure the event is already registered when adding the handler. The problem with the existing system happens in a situation where the first resource that used addEvent, is stopped. It is still possible that other resources had handlers for that specific event (and also used addEvent, for that matter), but still the event will never get triggered again. The reason is that the addEvent calls that came after the initial one, immediately returned false and didn't actually do anything.
This PR changes that behaviour, keeping a list of all resources that added a certain event and only removing an event when all resources that used addEvent are stopped. For security reasons, addEvent will return false and not do anything in case the allowRemoteTrigger argument would have been altered between different calls to the function.
Small note: I noticed that the strArguments member of SEvent is never used for anything, perhaps it could be an idea to clean this and remove the unused member?