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: Fix filtering UNIT_* events by unitId #200

Merged
merged 2 commits into from
Jun 23, 2024

Conversation

oomshroom
Copy link
Contributor

Fix filtering by unit in UNIT_* events.
Fixes event filters such as "UNIT_HEALTH boss1" not working at all.

@oomshroom oomshroom force-pushed the unit-event-filtering-fix branch from 788998f to aca8cc1 Compare February 29, 2024 10:25
@Zidras
Copy link
Owner

Zidras commented Feb 29, 2024

@oomshroom, can you briefly explain why this is needed? I'd have to check on some recent logs but I have no ticket open for unit health not working

@oomshroom
Copy link
Contributor Author

@oomshroom, can you briefly explain why this is needed? I'd have to check on some recent logs but I have no ticket open for unit health not working

Currently all timers/warnings which use "UNIT_HEALTH boss1" are broken. These are mostly "Next Phase Soon"-style warnings, such as Deathbringer's Frenzy and Sindragosa's P2.
In practice UNIT_HEALTH is the only event affected, but it is a commonly used event.

In DBM:ADDON_LOADED(), DBM registers "UNIT_HEALTH mouseover target focus player" on the main frame. In the existing code, handleEvent(self, "UNIT_HEALTH", "boss1") checks the first entry in registeredEvents, which for UNIT_HEALTH is the DBM main frame, and immediately returns because boss1 is not wanted there. Other entries in registeredEvents, such as those from a boss mod, are never checked!

@Zidras
Copy link
Owner

Zidras commented Mar 1, 2024

@oomshroom, it is a valid issue, thanks for bringing this up and PR'ing a fix.
I will try to look into it during next week

@Zidras Zidras self-assigned this Mar 1, 2024
@Zidras Zidras added the bug Something isn't working label Mar 1, 2024
@Zidras Zidras self-requested a review June 10, 2024 11:39
@Zidras Zidras merged commit 49cc770 into Zidras:main Jun 23, 2024
1 check passed
@Zidras
Copy link
Owner

Zidras commented Jun 23, 2024

Merged at last! Thank you @oomshroom for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants