Skip to content

#1989 Restore the Patron Tab spell list in the Sequence Editor - #1990

Merged
TimothyLuke merged 4 commits into
TimothyLuke:masterfrom
LarryThiessen:fix-1989-tab-spell-list
Aug 21, 2026
Merged

#1989 Restore the Patron Tab spell list in the Sequence Editor#1990
TimothyLuke merged 4 commits into
TimothyLuke:masterfrom
LarryThiessen:fix-1989-tab-spell-list

Conversation

@LarryThiessen

Copy link
Copy Markdown
Contributor

Fixes #1989

Problem

Pre-#1914, the Patron QoL wired Tab in an Action block's spell field and macro box to a menu: Insert Spell (player spellbook) + Insert GSE Variable. The AceGUI-removal restructure (a32c1c6) removed that code, its playerSpells cache, and the L["Insert Spell"] locale key when the Macro Insertion Toolbar moved to the external GSE_MacroToolbar addon — Tab in a MacroBlock has done nothing since.

Fix

3 files, +77/−0, in the modern hook style (matches the surviving OnEditorBooleanTab):

  • QoL.luagetPlayerSpells(): stateless on-demand spellbook enumeration (active/non-passive/non-offspec, sorted; replaces the old 4-event cache). GSE.OnEditorSpellTab (spell field, applies via callback so the field's handlers store) + GSE.OnEditorMacroBlockTab (macro box, cursor insert; the box's OnTextChanged owns storage — the old stray action.spell write is deliberately not restored).
  • Editor.lua — both hooks invoked (guarded, no-op without GSE_QoL) at the end of the spell/macro editbox factory.
  • ModL_enUS.lua — restore L["Insert Spell"] = true (absence raised AceLocale "Missing entry" on menu open).

Verified

In-game: Tab pops the menu in both fields, spells insert at cursor / set the field, no locale error. luac -p clean on all three files.

🤖 Generated with Claude Code

…itor

The pre-TimothyLuke#1914 QoL module wired Tab in an Action block's spell field and
macro-commands box to a context menu offering Insert Spell (the player's
spellbook) and Insert GSE Variable. The AceGUI-removal restructure
(a32c1c6) dropped that code, its playerSpells cache, and the
L["Insert Spell"] locale entry when the Macro Insertion Toolbar moved to
the external GSE_MacroToolbar addon -- Tab in a MacroBlock has done
nothing since.

Restore it in the modern hook pattern (matches OnEditorBooleanTab):

- GSE_QoL/QoL.lua: getPlayerSpells() enumerates the spellbook on demand
  (active, non-passive, non-offspec, sorted) -- no cache or event
  plumbing to go stale. GSE.OnEditorSpellTab applies the pick via
  callback so the spell field's own handlers store it;
  GSE.OnEditorMacroBlockTab inserts at the cursor and leaves storage to
  the box's OnTextChanged (the old stray action.spell write when
  inserting into the macro box is intentionally not restored).
- GSE_GUI/Editor.lua: invoke both hooks (guarded; no-op without
  GSE_QoL) at the end of the spell/macro editbox factory.
- GSE/Localization/ModL_enUS.lua: restore L["Insert Spell"] = true;
  its absence raised an AceLocale "Missing entry" error on menu open.

Fixes TimothyLuke#1989

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@TimothyLuke

Copy link
Copy Markdown
Owner

while this works for Retail it fails on some classic variants which dont implement C_SpellBook. Reworking to cover both

getPlayerSpells returned an empty table when C_SpellBook was missing, so on
Classic the Tab menu opened with an Insert Spell heading and nothing beneath
it. Worse, the guard tested the TABLE: TBC Classic Anniversary and MoP
Classic expose C_SpellBook without all of its functions, so they passed the
check and then found no skill-line API — the same trap documented on
spellIDIsInSpellBook in GSE/API/translator.lua, which is why the choice is
now made per FUNCTION, requiring every call the modern loop makes.

The Classic path walks tabs rather than skill lines: GetNumSpellTabs /
GetSpellTabInfo for the offsets, GetSpellBookItemName for the name, and
IsPassiveSpell plus GetSpellBookItemInfo for the filtering the modern API
does with fields on an info table. Three Classic-specific behaviours the
Retail loop has no equivalent for:

  - ranks. Classic lists one row per rank, so names are deduped. A macro
    wants the name anyway — /cast <name> already picks the highest rank
    known.
  - FUTURESPELL rows, which are the greyed-out not-yet-learned entries, are
    excluded; only "SPELL" is castable.
  - offSpecID from GetSpellTabInfo, Cata+ only and nil before it, stands in
    for isOffSpec so another spec's book is skipped.

A client exposing neither API returns an empty list rather than erroring,
which is what happened before on every Classic client.

.luacheckrc gains the five Classic globals; without them the build fails on
undefined-variable warnings.

spec/spellbook_spec.lua covers both APIs, the partial-C_SpellBook client, the
off-spec tab, a client with no item-info call, and no spellbook API at all —
none of which can be reached from a Retail test run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wmCKbEwmEgWW8SpWnXJhC
@TimothyLuke

Copy link
Copy Markdown
Owner

Pushed 847784f to this branch — the Classic path was missing.

getPlayerSpells returned an empty table whenever C_SpellBook was absent, so on Classic the menu opened with an Insert Spell heading and nothing under it. And the guard tested the table, which is the sharper edge: TBC Classic Anniversary and MoP Classic expose C_SpellBook without all of its functions, so they pass C_SpellBook and C_SpellBook.GetNumSpellBookSkillLines… and then find no working skill-line API. That's the same trap already documented on spellIDIsInSpellBook in GSE/API/translator.lua — the one that produced ~179 errors per save in #1925. The choice is now made per function, requiring every call the modern loop actually makes.

The Classic branch walks tabs rather than skill lines — GetNumSpellTabs / GetSpellTabInfo for offsets, GetSpellBookItemName for the name, IsPassiveSpell and GetSpellBookItemInfo for the filtering the modern API does with fields on an info table. Three things there have no Retail equivalent:

  • Ranks. Classic lists one row per rank, so names are deduped — a macro wants the name anyway, since /cast <name> already picks the highest rank known.
  • FUTURESPELL rows are the greyed-out not-yet-learned entries; only "SPELL" is castable.
  • offSpecID from GetSpellTabInfo (Cata+, nil before it) stands in for isOffSpec, so another spec's book is skipped.

A client with neither API now returns an empty list rather than erroring — which is what every Classic client hit before.

.luacheckrc gains the five Classic globals; without them the build fails on undefined-variable warnings.

spec/spellbook_spec.lua covers both APIs plus the cases a Retail test run can't reach: the partial-C_SpellBook client, the off-spec tab, ranks collapsing, a client with no item-info call, and no spellbook API at all. 156 tests, luacheck clean across all 67 files.

The rest of the PR I left as-is — restoring the hooks in the modern pattern rather than the old cache is the right call, and deliberately not re-writing action.spell from the macro box matches what the field's own handlers expect.

TimothyLuke and others added 2 commits August 21, 2026 10:55
CI runs busted under Lua 5.1, where `load` takes a reader FUNCTION — a string
chunk goes through loadstring, and its environment is attached with setfenv.
5.2+ removed both and added load(chunk, name, mode, env), which is what this
used, so all six tests errored with "bad argument TimothyLuke#1 to 'load' (function
expected, got string)" while passing locally on 5.4.

Both forms are supported now, chosen on whether setfenv exists rather than on
which interpreter happens to be installed. Verified by running the same
extraction under lua5.1 and lua5.4 directly: identical spell lists from both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wmCKbEwmEgWW8SpWnXJhC
Local busted is installed for 5.4 only; CI is PUC Lua 5.1.5
(leafo/gh-actions-lua@v10 + luarocks busted). So a spec can use anything 5.2
added, pass every local check, and fail in CI. That has now happened twice on
the same construct — load(chunk, name, mode, env), where 5.1's load takes a
reader FUNCTION and a string chunk needs loadstring + setfenv.

    lua5.1 spec/run51.lua      # matches CI, exits 1 on failure
    busted                     # full luassert, 5.4

Run both. Verified by reintroducing the exact bug: run51 fails with CI's
error verbatim while busted stays green.

Deliberately NOT a busted replacement — it implements the globals the GSE
specs use (describe/it/setup/before_each and the luassert matchers), so a
spec reaching for more will fail here and needs busted too.

Two details that matter:
  - one spec per PROCESS. Specs leak globals — CreateFrame is defined only by
    keydownbinding_spec — so a shared interpreter makes results depend on
    file order.
  - checksum_spec reports SKIPPED, not failed: it needs luabitop's `bit`
    global, which cannot be installed here (no luarocks binary). CI and
    busted both still cover it, and a runner that always shows a failure is
    a runner nobody reads.

Do NOT substitute luajit for lua5.1. LuaJIT extends 5.1 and accepts
load(string, ...), so it passes the very bug this exists to catch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wmCKbEwmEgWW8SpWnXJhC
@TimothyLuke
TimothyLuke merged commit 3bed250 into TimothyLuke:master Aug 21, 2026
1 check passed
@LarryThiessen
LarryThiessen deleted the fix-1989-tab-spell-list branch August 21, 2026 01:22
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.

[BUG] #1914 regression: Patron Tab spell list gone from the Sequence Editor (Insert Spell menu on the spell field / macro box)

2 participants