Stabilize ExtraIcons layout anchoring#101
Conversation
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/cc94c4eb-cf3c-4001-a8f3-16502ae1b350 Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/7ac040cd-97f3-4a61-954d-c015be57e7f1 Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/6b29a3f7-f340-4028-9490-51841f7126af Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Test Results891 tests - 5 891 ✅ - 5 2s ⏱️ ±0s Results for commit 1452260. ± Comparison against base commit 9591303. This pull request removes 55 and adds 50 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This pull request stabilizes ExtraIcons layout anchoring by switching repeated ClearAllPoints/SetPoint work to the shared FrameUtil.LazySetAnchors helper, reducing unnecessary re-anchoring (and potential flicker) during frequent layout passes. It also adds explanatory “ELI5” comments to make the layout flow easier to follow for future maintenance.
Changes:
- Use
FrameUtil.LazySetAnchorsfor Blizzard viewer, extra-icon container, and pooled icon anchoring to avoid reapplying unchanged anchors. - Avoid anchoring extra icons to hidden “active” Blizzard item frames; fall back to anchoring to the viewer frame instead.
- Extend the real-source spec coverage with new tests for hidden-active-frame anchoring and for avoiding redundant anchor reapplication.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Modules/ExtraIcons.lua |
Replaces eager anchor resets with LazySetAnchors, tightens anchor selection to ignore hidden active frames, and adds clarifying comments around the layout algorithm. |
Tests/Modules/ExtraIcons_spec.lua |
Loads real FrameUtil (with a stubbed ApplyFont) and adds regression tests covering the anchoring stability scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Top of function comments must be changed to luadocm, not inlined below the signature. |
|
There are too many "resolve*" functions that are too similar. I feel like they can be refactored and compressed. It's not clear how they're all different just from their name alone. |
|
Remove the "ELI5:" comments. That was not in the intention of that instruction. It was to make the comments make sense, and not sound like an AI wrote it. |
There was a problem hiding this comment.
I don't see why VIEWERS and BLIZZ_KEY are necessary. The keys aren't used. The comment (below) also makes no sense - They do not move together in edit mode - they are completely independent.
There was a problem hiding this comment.
Removed BLIZZ_KEY, removed the misleading VIEWERS comment, and kept only the viewer metadata that is still used for iteration/config lookup in 97fbdf9.
| local canAccessTable = _G.canaccesstable | ||
|
|
||
| -- Some racial abilities have different spell IDs for different races/factions. | ||
| -- This lookup lets a saved spell ID find its whole family of equivalent IDs. |
There was a problem hiding this comment.
Change:
Some racial abilities have different spell IDs. For example, Dracthyr evokers have a more potent wing buffet compared to other classes.
There was a problem hiding this comment.
Updated the racial spell ID wording and moved the alias mapping into constants in 97fbdf9.
| -- ExtraIcons adds the user's chosen potions, trinkets, racials, and spells | ||
| -- directly after Blizzard's cooldown viewer rows. Think of each Blizzard row | ||
| -- as a train: this module adds a small extra car to the end without moving the | ||
| -- train station anchor that other ECM modules use. |
There was a problem hiding this comment.
Removed this top-level explanatory block in 97fbdf9.
| @@ -23,7 +29,9 @@ for _, racial in pairs(RACIAL_ABILITIES) do | |||
| end | |||
| end | |||
There was a problem hiding this comment.
Can this be stored in the ideal format in constants instead of computed?
There was a problem hiding this comment.
Moved the racial spell alias mapping into Constants.lua instead of computing it in ExtraIcons.lua in 97fbdf9.
| vs.originalPoint = { point, relativeTo, relativePoint, x or 0, y or 0 } | ||
| end | ||
|
|
||
| local function applyPoint(vs, blizzFrame, offsetX) |
There was a problem hiding this comment.
Is this method required? LazySetAnchors is supposed to guard against no-op changes.
There was a problem hiding this comment.
Removed the helper and now call FrameUtil.LazySetAnchors directly at the offset application sites in 97fbdf9.
| -- Count text is optional. Nil means "hide the number"; any value means | ||
| -- "show this number on top of the icon." |
There was a problem hiding this comment.
Removed the obvious count-text comments in 97fbdf9.
|
|
||
| local function updateIconCountText(icon, globalConfig, config) | ||
| -- Item icons can show stack counts and spell icons can show charges. Only | ||
| -- show useful numbers: one item or a non-charge spell should stay clean. |
There was a problem hiding this comment.
Remove this second sentence - pointless.
There was a problem hiding this comment.
Removed the extra sentence along with the ELI5 comment cleanup in 97fbdf9.
| --- Caches and returns the cooldown number font from a sibling Blizzard icon. | ||
| --- ELI5: copy Blizzard's number style so ECM's extra icons look like they | ||
| --- belong in the same row. | ||
| local function getSiblingFont(viewer) |
There was a problem hiding this comment.
This doesn't seem to work. Just remove it.
There was a problem hiding this comment.
Removed getCombatState; error logging already enriches diagnostics with combat state in 97fbdf9.
| @@ -385,6 +443,8 @@ local function getItemFramesCount(itemFrames) | |||
| end | |||
|
|
|||
| local function getCombatState() | |||
| @@ -421,6 +483,9 @@ local function getViewerDiagnostics(blizzFrame, viewerKey, why, itemFrames) | |||
| end | |||
|
|
|||
| local function getAccessibleItemFrames(blizzFrame, viewerKey, why) | |||
There was a problem hiding this comment.
Removed the unused utility anchor creation; anchors are now only created for viewer configs that own an anchor in 97fbdf9.
|
@copilot address feedback |
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/4c2a168f-7f02-404f-a7ac-065bd406849a Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/4c2a168f-7f02-404f-a7ac-065bd406849a Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Agent-Logs-Url: https://github.com/argium/EnhancedCooldownManager/sessions/4c2a168f-7f02-404f-a7ac-065bd406849a Co-authored-by: argium <15852038+argium@users.noreply.github.com>
Removed the inline top-of-function comments and kept remaining function docs in LuaDoc style in 97fbdf9.
Renamed and compressed the entry-resolution helpers so item, spell, equip-slot, and configured stack paths are clearer in 97fbdf9 and 1b418e7.
Removed the ELI5 comments in 97fbdf9.
Addressed the feedback across 97fbdf9, 4974c13, and 1b418e7; full tests and luacheck pass. |
ExtraIcons could repeatedly clear and reapply the same anchors while laying out its appended cooldown icons, which made the flicker-sensitive layout path harder to keep stable.
ExtraIcons could repeatedly clear and reapply the same anchors while laying out its appended cooldown icons, which made the flicker-sensitive layout path harder to keep stable. The fix uses the shared FrameUtil guarded anchor helper, avoids anchoring extra icons to hidden active Blizzard item frames by falling back to the viewer frame, and includes regression coverage for the anchoring behavior.
Follow-up cleanup removes the overdone ELI5 comments, simplifies helper naming/structure, moves racial spell aliases into constants, and moves the shared safe frame diagnostic helper into
ECM.lua.