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

Replace callbacks and local events with emitted events #427

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

LucasPickering
Copy link
Owner

@LucasPickering LucasPickering commented Dec 18, 2024

Description

Describe the change. If there is an associated issue, please include the issue link (e.g. "Closes #xxx"). For UI changes, please also include screenshots.

Using events instead of callbacks to trigger behavior guarantees that components will have mutable access, and that the TUI will be rerendered after an update. The emitter trait gives each component that can emit events a unique ID, so we can't mix up events of the same type from different instances. It also ensures each component emits events of a single known type, so we get better type coercion from the event trait objects.

In some ways this adds complexity because we end up doing multiple rounds of event handling, which led to some bugs I had to fix. I think overall it's an improvement though, because it's more consistent.

Known Risks

What issues could potentially go wrong with this change? Is it a breaking change? What have you done to mitigate any potential risks?

This is a major refactor, so there's tons of risk of tiny bugs showing up related to UI events. Existing tests mitigate this a bit, but I'm going to let it soak on master for a bit and use it myself to see if I notice anything.

QA

How did you test this?

Lots of existing tests that I updated

Checklist

  • Have you read CONTRIBUTING.md already?
  • Did you update CHANGELOG.md?
    • Only user-facing changes belong in the changelog. Internal changes such as refactors should only be included if they'll impact users, e.g. via performance improvement.
  • Did you remove all TODOs?
    • If there are unresolved issues, please open a follow-on issue and link to it in a comment so future work can be tracked

Using events instead of callbacks to trigger behavior guarantees that components will have mutable access, and that the TUI will be rerendered after an update. The emitter trait gives each component that can emit events a unique ID, so we can't mix up events of the same type from different instances. It also ensures each component emits events of a single known type, so we get better type coercion from the event trait objects.
@LucasPickering LucasPickering enabled auto-merge (rebase) December 18, 2024 01:08
@LucasPickering LucasPickering merged commit 4c8cf68 into master Dec 18, 2024
15 checks passed
@LucasPickering LucasPickering deleted the emitters branch December 18, 2024 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant