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

Patch unit events #472

Merged
merged 12 commits into from Jan 3, 2019
Merged

Patch unit events #472

merged 12 commits into from Jan 3, 2019

Conversation

Rainrider
Copy link
Member

Basically, deciding silently for the layout which event is unitless is bad.
Events like PARTY_MEMBER_ENABLE, INCOMING_RESURRECT_CHANGED, PLAYER_SPECIALIZATION_CHANGED and others are all unit events but were always wrongly considered unitless.

Side notes:

The following code from a layout:

self:RegisterEvent('PLAYER_TOTEM_UPDATE', Update)

will produce the following error:

3x oUF\events.lua:98: Attempt to register unknown event "PLAYER_TOTEM_UPDATE"
[C]: ?
oUF\events.lua:98: in function `RegisterEvent'
oUF_Layout\elements\custom\totems.lua:75: in function `enable'
oUF\ouf-@project-version@.lua:113: in function `EnableElement'
oUF\ouf-@project-version@.lua:329: in function <oUF\ouf.lua:251>
(tail call): ?
oUF\ouf-@project-version@.lua:685: in function `Spawn'
oUF_Layout\layout.lua:7: in function `func'
oUF\factory.lua:20: in function <oUF\factory.lua:16>
(tail call): ?
...

This is because PLAYER_TOTEM_UPDATE is unitless and cannot be registered as a unit event. Blame Blizz about the wording of the error message.

This is a breaking change. Layouts will have to mark unitless events by using the 4th param of RegisterEvent.

The possibility of mixed registration may be a thing. All event handlers should check if they got the expected unit.

NOTE: the unit check is needed because of the possibility for mixed event registration

- 💄
- fixed early exit for pvpindicator when the event is HONOR_LEVEL_UPDATE
- resurrect and summon indicators now use the event unit
UpdateUnits registers the event for the right units
Blizzard API documentation should not be part of code comments I think
@Rainrider
Copy link
Member Author

I think this is working as intended now. Please test and comment.

To sum it up:

  • Unitless events have to be explicitly marked as such when calling oUF's RegisterEvent. This is not backwards-compatible and thus a breaking change.
  • Blizzard's RegisterUnitEvent behaves strangely and falls back silently to the behavior of RegisterEvent when passing explicit nil for either of the unit arguments. This is also true if only invalid units were passed to it.
  • Unit event registration for header units has to be postponed until the frame units are known.
  • In order to support mixed event registration (unit events can be registered either for specific units or for all), handlers are NOT guarantied to be only run for the frame units. Thus, checking the event unit against the frame unit is advised (this is not new however).

Covers the case where layouts might be calling RegisterEvent after real units have been set
@Rainrider Rainrider added the core label Jan 2, 2019
@Rainrider Rainrider merged commit 8cb0b06 into master Jan 3, 2019
@Rainrider Rainrider deleted the patch-unit-events branch January 3, 2019 09:47
local isOK, _ = pcall(validator.RegisterUnitEvent, validator, 'UNIT_HEALTH', unit)
if isOK then
_, unit = validator:IsEventRegistered('UNIT_HEALTH')
if unit then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing paranteses in this and the above conditional.

doadin pushed a commit to doadin/Elvui that referenced this pull request Jan 7, 2019
doadin pushed a commit to doadin/Elvui that referenced this pull request Jan 7, 2019
Rainrider added a commit that referenced this pull request Apr 17, 2019
Rainrider added a commit that referenced this pull request Apr 17, 2019
Rainrider added a commit that referenced this pull request Apr 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants