Add loadout manager for copying, renaming and deleting loadouts - #10224
Add loadout manager for copying, renaming and deleting loadouts#10224canuysal wants to merge 2 commits into
Conversation
* Add "Manage Loadouts" option to the loadouts dropdown, replacing "Sync" * Manager supports New, Copy, Rename and Delete across all four associated sets, plus double-click to activate * Copy creates the new loadout automatically with a unique name * Move Sync into the manager popup * Update help.txt and add tests
* Add "New/Copy Custom" button, which creates a loadout choosing per set whether to start fresh or copy an existing one * Default the choices to the currently active sets, so saving immediately copies the active loadout * Split the per-set copy and delete logic out of Copy/Delete so both paths share it * Add tests for copying the untitled default loadout, deleting a loadout, keeping shared sets, and resetting the dropdown selection
|
New changes, reflecting #9647 |
| self.selIndex = nil | ||
| self.selValue = nil | ||
| for index, loadout in ipairs(self.list) do | ||
| if loadout.specId == specId then |
There was a problem hiding this comment.
For identifier loadouts, specId is not unique: a tree titled Linked {A,B} produces {A} and {B} rows with the same passive tree but different linked sets. After double-clicking {B}, the active tuple remains {B}, but SyncLoadouts() rebuilds the rows and this condition selects {A}. Copy/Rename then consume {A}'s tuple, and Delete presents {A} and can remove its A-only item/skill/config sets. A focused repro observed linkId == "A" while {B} was current. A stable full key—linkId plus the four set IDs, or the equivalent active tuple—would preserve the selected linked tuple across SyncLoadouts().
AI-assisted review disclosure: This finding was identified during a review using OpenAI Codex and confirmed with a focused reproduction.
| controls.save.enabled = buf:match("%S") | ||
| end) | ||
| controls.save = new("ButtonControl"):ButtonControl(nil, {-45, 70, 80, 20}, "Save", function() | ||
| local newName = controls.edit.buf |
There was a problem hiding this comment.
The new Rename path only validates non-whitespace for non-identifier loadouts. Renaming Second to an existing First leaves two First trees; because SyncLoadouts() indexes item/skill/config sets by title, both rows resolve to the last matching tuple and the original First component sets become unresolved (not deleted). A focused repro observed exactly those 2 First rows after the rename. A possible guard is to reject an existing loadout name while allowing the current name for a no-op rename, with the same check on the new Custom path before any set is mutated.
AI-assisted review disclosure: This finding was identified during a review using OpenAI Codex and confirmed with a focused reproduction.
|
|
||
| -- Every set defaults to "New"; if a loadout is selected in the manager, its sets are | ||
| -- preselected instead so they can be kept or swapped out one at a time | ||
| local selected = self.selValue |
There was a problem hiding this comment.
Opening the manager does not select a row, so self.selValue is nil here and all four dropdowns intentionally remain on the New sentinel. Manage → New/Copy Custom → Save therefore creates four fresh sets instead of copying the active tuple as explicitly described in the PR and help text; a focused test observed four nil selection IDs instead of the four active IDs. Selecting a row does preselect that highlighted row, but that is different from the documented active-tuple default. One possible approach is to initialize these selectors from the active tree/item/skill/config IDs, with an explicit row selection overriding them afterward.
AI-assisted review disclosure: This finding was identified during a review using OpenAI Codex and confirmed with a focused reproduction.
2 participants
Description of the problem being solved:
Hey guys, long time no see!
Previous loadout management was bugging me, whenever I wanted to clone a loadout for some changes, I have to copy tree/skills/itemset/config, rename each, then sync to get a new set to play around. If I have a live build to import from poe, it means losing the subtle changes I made to the skills/items, i.e making raise zombie count 17, managing spectres (another issue on that, will send a PR soon) and so on. This fixes that by adding instant clone, new, delete and rename buttons for loadouts in a similar manage dialog as individual sets use.
Sync button is moved inside the dialog. Reason is it will be used less and less from now on, but it's still there in the dialog for backwards compatibility, organizing individual sets into one.
New Loadouts are left in the quick access menu for convenience.
Steps taken to verify a working solution:
docker compose up: 525 successes / 0 failures (522 pre-existing plus 3 new loadout tests inspec/System/TestLoadouts_spec.lua, covering loadout creation, automatic copy with unique naming, and{identifier}link resolution){1}style identifiers, confirmed the identifier is preserved on each set and the loadout still resolvesLink to a build that showcases this PR:
Any build would do:
https://pobb.in/H_y7eOag7gaK
Before screenshot:
After screenshot: