Fix(NetworkEvents): Don't skip event callbacks in NetworkEvents::removeEvent

[LeeLeahy2]

Description of Change

Modified the four (4) NetworkEvents::removeEvent routines in libraries/Network/src/NetworkEvents.cpp to decrement "i" after the event callback is erased.

Tests scenarios

Requires Fix [10317] to prevent crashes.

Used the submitted sketch for issue 10318. Without this change each actual event generates 4 callbacks initially and 2 callbacks after calling removeEvent. With this change all event handlers are removed and no further callbacks are called.

Related links

Closes issue 10318

[CLAassistant]

All committers have signed the CLA.

[github-actions]

Warnings

⚠️

Some issues found for the commit messages in this PR: the commit message "Issue 10318: Don't skip event callbacks in NetworkEvents::removeEvent": summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello LeeLeahy2, we appreciate your contribution to this project!

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against b778fad

[me-no-dev]

Hi @LeeLeahy2 ! I do disagree with the solution a bit. IMHO the fix in this case should be in onEvent, where the code should check if this same callback is already assigned and not allow to assign it again. You are attaching the same function 4 times, which is not a valid use case. Only exception is to attach the same function only for specific events, which the solution I propose covers as well. Do you agree?

[JAndrassy]

instead of i-- I would add break

[VojtechBartoska]

Hello @LeeLeahy2, thanks for the proposal.

Please sign CLA too, it is needed to unblock this PR. One of the requirements, we will see how the discussion will be shaped.

[me-no-dev]

instead of i-- I would add break

Actually return and then also add log error at the end that callback was not found

[TD-er]

Why even use this i and not an iterator?
erase returns the iterator to the next element (or .end())
So if this is done wrong, why not just use these functions as how it is intended in these stl containers and thus use iterators?

Also I wonder why it is kept as a std::vector instead of std::list, but that may be out of scope of this PR...

[LeeLeahy2]

I made the choice based upon the original implementation which continued scanning the list instead of having the break after the entry was found. As such, it looked like multiple entries were to be allowed, so I provided an example sketch to support this behavior. This also was the minimal change and did change the implementations behavior.

The alternative was a much larger change and was also possibly a change in desired behavior, although this is the behavior that I would have expected:

• Add the breaks in the removeEvent routines
• Possibly issue an error when the entry was not found in removeEvent routines
• Modify the onEvent routines to verify that the entry was not in the list before adding the entry to the list
• Modify the onEvent routines to issue an error when the entry is found on the list

I will submit another patch that implements the alternative behavior above.

[me-no-dev]

Thanks @LeeLeahy2 looking forward the new patch

[me-no-dev]

@LeeLeahy2 we can close this one now, correct?