Skip to content

Commit e0586ad

Browse files
committed
Move GUI:CloseAllMenus back resident, with its registry
Four lines, and having them in the companion split a three-part unit across an addon boundary for no benefit: the registry table and RegisterMenu were resident while the only thing that reads the registry was not. Resident code could add a menu but not close one, and resident CreateDropdown does register (the mover panel's anchor dropdown is a resident caller). Nothing was broken -- both callers are companion-side, so reader and callers loaded together -- but the failure mode if anyone had reached for it from resident code was the bad kind: a bulk 'dismiss whatever is open' call that is nil half the time fails SILENTLY, leaving a menu floating, which is the exact symptom the registry exists to prevent. Taking the fix rather than documenting the wart, which is also the lesson from the two ORDER-IS-LOAD-BEARING claims this review found to be false: a comment describing a split is worse than not having the split. gui-conventions.md updated to match -- it now says all three pieces are resident and should stay together, keeps the reasoning as a note on why, and still distinguishes CloseAllMenus from CloseOpenDropdown (resident, on _priv, the single-open-dropdown case most call sites actually want).
1 parent 78719df commit e0586ad

3 files changed

Lines changed: 39 additions & 35 deletions

File tree

DandersFrames/GUI/Widgets.lua

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1822,6 +1822,20 @@ end
18221822
GUI._menus = GUI._menus or {}
18231823
function GUI:RegisterMenu(frame) self._menus[frame] = true end
18241824

1825+
-- ☠ Lives with the registry it iterates, deliberately. This was in the
1826+
-- companion for a while, which left resident code able to REGISTER a menu but
1827+
-- not close one -- and resident CreateDropdown below does register (the mover
1828+
-- panel's anchor dropdown is a resident caller). Nothing broke, because both
1829+
-- callers happened to be companion-side, but a bulk "dismiss whatever is open"
1830+
-- call that is nil half the time fails SILENTLY: the menu just stays floating,
1831+
-- which is the exact symptom this registry exists to prevent. Four lines is
1832+
-- not worth an addon boundary.
1833+
function GUI:CloseAllMenus()
1834+
for f in pairs(self._menus) do
1835+
if f:IsShown() then f:Hide() end
1836+
end
1837+
end
1838+
18251839
function GUI:CreateDropdown(parent, label, options, dbTable, dbKey, callback, customGet, customSet, opts)
18261840
opts = opts or {}
18271841
local accentColor = opts.accent

DandersFrames_Options/GUI/SettingsWidgets.lua

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1922,12 +1922,6 @@ function GUI:CreateColorPicker(parent, label, dbTable, dbKey, hasAlpha, callback
19221922
return container
19231923
end
19241924

1925-
function GUI:CloseAllMenus()
1926-
for f in pairs(self._menus) do
1927-
if f:IsShown() then f:Hide() end
1928-
end
1929-
end
1930-
19311925
function GUI:CreateOutlineDropdown(parent, label, dbTable, dbKey, callback, inheritKey)
19321926
local options = {
19331927
NONE = L["None"],

docs/gui-conventions.md

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ one surface behave unlike the rest of the addon.
182182

183183
| Need | Helper | ☠ What breaks if you skip it |
184184
|------|--------|------------------------------|
185-
| A dropdown-style popup menu | `GUI:RegisterMenu(frame)` — resident; `GUI:CloseAllMenus()`**companion-only, see below** | **Register every menu frame at creation.** A menu that isn't registered stays open through a page change, mode switch or window close, and floats over whatever comes next |
185+
| A dropdown-style popup menu | `GUI:RegisterMenu(frame)`, `GUI:CloseAllMenus()` | **Register every menu frame at creation.** A menu that isn't registered stays open through a page change, mode switch or window close, and floats over whatever comes next |
186186
| A widget that changes height | `GUI:RelayoutHost(widget, slotHeight)` | Updates the group's stored slot, re-lays the group, **then bubbles to the page** so sibling groups re-anchor. Without the bubble a grown group's backdrop overshoots the next group's anchor and paints an empty rectangle above it |
187187
| A border on a bespoke frame | `GUI:ApplyPixelBorder(frame, {r,g,b,a}, weight)`, `GUI:HidePixelBorder(frame)`, `GUI:RefreshPixelBorders()` | Our two-device-pixel border. A `SetBackdrop` edge instead gives you the thinning / vanishing hairline the whole system exists to avoid — see "The pixel grid" |
188188
| Whole-pixel geometry | `GUI.SnapLen`, `GUI.SnapLenUp` | Right/top edges land mid-pixel and clip whatever they contain |
@@ -192,36 +192,32 @@ one surface behave unlike the rest of the addon.
192192
| Colour-picker interop | `GUI:InstallColorPickerHook()` / `Uninstall…` / `IsColorPickerHookInstalled()` / `MarkColorPickerCall()` | Keeps Blizzard's picker from stomping our own; mark your call or the hook can't tell whose it is |
193193
| Section collapse state | `GUI:GetCollapsedGroups()` | Persisted collapse state — read it, don't track your own |
194194

195-
### The menu registry straddles the addon boundary
195+
### The menu registry — all three pieces are resident
196196

197-
The three pieces of the menu system do not live together, which the table above
198-
cannot show:
197+
The registry table, its writer and its reader live together in
198+
`DandersFrames/GUI/Widgets.lua`, and they should stay that way:
199199

200-
| Piece | Where | Loads |
201-
|---|---|---|
202-
| `GUI._menus` (the registry table) | `DandersFrames/GUI/Widgets.lua` | always |
203-
| `GUI:RegisterMenu(frame)` | `DandersFrames/GUI/Widgets.lua` | always |
204-
| `GUI:CloseAllMenus()` | `DandersFrames_Options/GUI/SettingsWidgets.lua` | with the settings panel |
205-
206-
So **resident code can register a menu but cannot close one.** Resident
207-
`GUI:CreateDropdown` registers its menu frame, and it has one resident caller —
208-
the mover panel's anchor dropdown (`Frames/Position.lua`), which exists with
209-
the settings panel unloaded.
210-
211-
Nothing is broken today: both `CloseAllMenus` callers are companion-side
212-
(`AuraDesigner/UI/Cards.lua`), so the reader and its callers load together. But
213-
if you are writing resident code and reach for `CloseAllMenus`, **it will be
214-
nil until someone has opened `/df`** — and because it is a bulk "tidy up"
215-
call, a nil-guard that skips it fails silently by leaving a menu floating,
216-
which is exactly the symptom the registry exists to prevent.
217-
218-
If you need it resident, move the function rather than guarding the call: it is
219-
four lines and it belongs with the registry it iterates. Guarding would
220-
document the split instead of removing it.
221-
222-
☠ Not the same thing as `CloseOpenDropdown` (resident, published on
223-
`GUI._priv`), which closes the *single currently-open* dropdown. That one is
224-
available on both sides and is what most call sites actually want.
200+
| Piece | Where |
201+
|---|---|
202+
| `GUI._menus` (the registry table) | resident |
203+
| `GUI:RegisterMenu(frame)` | resident |
204+
| `GUI:CloseAllMenus()` | resident |
205+
206+
`CloseAllMenus` briefly lived in the companion during the load-on-demand split.
207+
That left resident code able to *register* a menu but not close one — and
208+
resident `GUI:CreateDropdown` does register, with a resident caller (the mover
209+
panel's anchor dropdown). Nothing broke, because both `CloseAllMenus` callers
210+
happened to be companion-side, but the failure mode was the bad kind: a bulk
211+
"dismiss whatever is open" call that is nil half the time fails **silently**,
212+
leaving a menu floating — the exact symptom the registry exists to prevent.
213+
214+
☠ Four lines are not worth an addon boundary. If you ever find yourself
215+
nil-guarding a call like this, move the function instead — a guard documents
216+
the split rather than removing it.
217+
218+
Not the same thing as `CloseOpenDropdown` (resident, published on `GUI._priv`),
219+
which closes the *single currently-open* dropdown. That is what most call sites
220+
actually want.
225221

226222
## Popups — never Blizzard's
227223

0 commit comments

Comments
 (0)