Apply review fixes and replace CoX spell toggles with a dropdown#13
Open
mcscoobe wants to merge 6 commits into
Open
Apply review fixes and replace CoX spell toggles with a dropdown#13mcscoobe wants to merge 6 commits into
mcscoobe wants to merge 6 commits into
Conversation
Address review items #4-6, #9-11 and resolve the perpetual gradlew.bat "modified" state. Validation logic: - Generalize Kodai-only infinite-rune handling into a single INFINITE_RUNE_SOURCES mechanism covering elemental staves, combination staves, and charged Tome of Fire/Water; scans inventory + worn equipment (#4) - Replace EQUIPMENT_CONTAINER_ID literal (94) with InventoryID.WORN (#9) Config / docs: - Correct Thralls cost descriptions (10 Fire, 5 Blood, 1 Cosmic; Aether substitution) and Blood Barrage (4 Death runes, was "1 Death rune") (#5) - Mention staves/tomes in Ice Barrage, Humidify, and Thralls descriptions - Fix hasResurrectGreaterGhostRunes Javadoc (5 Blood runes, was "2") (#6) Plugin behavior: - Block unsatisfiable dual-spellbook CoX config by reverting the second conflicting toggle to off and posting feedback, instead of warn-only (#11) - Close first-tick menu-protection gap by validating on demand in onMenuEntryAdded when no cached result exists (#10) Line endings: - Add "*.bat text eol=crlf" to .gitattributes so batch files are legitimately CRLF, and renormalize gradlew.bat. The committed blob previously held CRLF while "* text eol=lf" mandated LF, leaving the file eternally "modified" (#8) Tests: - Add 6 property tests for infinite-rune sources (staves, charged vs uncharged tomes, equipped sources, non-elemental exclusion) - Rework CoX conflict tests to drive onConfigChanged and assert the toggle is reverted via ConfigManager
Follow-up to the review of PR #13. Code-quality fixes: - Remove stale @SuppressWarnings("unused") on the now-used configManager field - Add AccessDeniedConfig.CONFIG_GROUP constant and route the @ConfigGroup annotation and both plugin call sites through it, removing the three duplicated "accessdenied" literals - Replace every raw rune/item literal in PlayerStateValidator with net.runelite.api.gameval.ItemID constants (FIRERUNE, WATERRUNE, ..., BOOK_OF_THE_DEAD) across the infinite-source map and all per-spell requirement maps, keeping the two provably consistent - Add Twinflame staff to the infinite rune sources (covers fire + water) - Switch the six parameterless infinite-rune tests from @Property to @example New tests (108 total, up from 95): - Blood Barrage runes: pass/fail/Aether, including a 3-Death-rune regression guard locking the 1 -> 4 Death requirement - onMenuEntryAdded first-tick on-demand validation: reorders when invalid, not when valid, skipped during an active CoX raid - Rune pouch: runes count toward requirements, are ignored when no pouch item is present (stale-varbit guard), and all four pouch variants are recognized - Spellbook discrimination across varbit 0-3 - Chug Jug / Saturated Heart presence checks - Uncharged Tome of Fire negative (symmetry with the Tome of Water case)
The Twinflame staff is a special-attack weapon whose rune behavior does not map cleanly to the "carrying this item grants an unlimited supply of rune X" model used here, and it was the only entry with no test coverage. Removing it avoids wrongly suppressing fire/water requirements for Ice Barrage, Humidify, and Thralls.
Enabling a CoX spell from one spellbook now switches off any enabled spells from the other spellbook, keeping the just-enabled toggle, instead of reverting the new toggle. The feedback message names the spells that were disabled. Also clarify the on-demand validation path in onMenuEntryAdded and name the affected toggle in conflict messaging. Fixes #14
Modelling the Chambers of Xeric spell requirement as a single CoxSpellRequirement enum (rendered as a dropdown) makes the Arceuus/Lunar cross-spellbook conflict impossible to express. This removes the runtime conflict-resolution code and the stale-checkbox lag it caused, since RuneLite's config panel does not refresh on programmatic config changes. Validation now maps the enum to the required spells; the old four boolean toggles, the conflict handler, and the ConfigManager dependency are removed. Adds a CoxSpellRequirement enum test (including the no-value-requires-both-spellbooks invariant) and enum-driven validation tests.
The warning fired for all five locations but named 'Thralls or Death Charge', which is wrong for Inferno (Ice/Blood Barrage) and for the CoX dropdown. Use a location-agnostic message.
mcscoobe
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies the outstanding review fixes and refactors the Chambers of Xeric spellbook handling.
Validation logic
INFINITE_RUNE_SOURCESmechanism covering elemental staves, combination staves, and the charged Tome of Fire/Water; scans inventory + worn equipment (Conventions performance review #4)EQUIPMENT_CONTAINER_IDliteral (94) withInventoryID.WORN(Close: #6 #9)Chambers of Xeric spellbook requirement (config change)
coxRequireSpell/coxRequireDeathCharge/coxRequireHumidify/coxRequireVengeance) with a single mutually-exclusiveCoxSpellRequirementdropdown.ConfigManagerdependency it needed.coxSpellRequirementkey is distinct from the old boolean keys, so existing CoX spell settings reset toNoneafter this update.Config / docs
hasResurrectGreaterGhostRunesJavadoc (5 Blood runes, was "2") (New spell #6)Plugin behavior
onMenuEntryAddedwhen no cached result exists (Plugin now checks equipped items for book of the dead #10)Line endings
*.bat text eol=crlfto.gitattributesand renormalizegradlew.bat. The committed blob held CRLF while* text eol=lfmandated LF, leaving the file perpetually "modified" (CLOSE #6 #8)Tests
CoxSpellRequirementTest, including the invariant that no enum value requires both spellbooks