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. Could this reselect by a stable full key (linkId plus the four set IDs, or the equivalent active tuple) rather than by passive tree alone?
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. Could Rename reject an existing loadout name, while allowing the current name for a no-op rename, and apply the same guard to 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. Could these selectors initialize directly 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: