Skip to content

feat: Add sorting options for menu items in MenuSpell - #1106

Open
DragonsAscent wants to merge 1 commit into
TheComputerGeek2:mainfrom
DragonsAscent:menu-sorting
Open

feat: Add sorting options for menu items in MenuSpell#1106
DragonsAscent wants to merge 1 commit into
TheComputerGeek2:mainfrom
DragonsAscent:menu-sorting

Conversation

@DragonsAscent

Copy link
Copy Markdown
Collaborator

Add sort-mode to Menu Spell options allowing first or last which sorts the item in the first available slot that isnt occupied starting from the first slot or from the last slot.

Copilot AI lite review requested due to automatic review settings September 9, 2026 00:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are localized and consistent with existing behavior, with only a minor improvement suggested around surfacing invalid config values.

Pull request overview

Adds a new sort-mode option for MenuSpell menu entries to control how an item is placed into configured slots when some are already occupied, enabling “fill first available” or “fill last available” behavior.

Changes:

  • Parse a new sort-mode config option per menu option (none/first/last).
  • Update inventory placement logic to support FIRST/LAST behaviors in addition to the existing “fill all empty defined slots” behavior.
File summaries
File Description
core/src/main/java/com/nisovin/magicspells/spells/MenuSpell.java Adds SortMode parsing/storage and updates slot-filling logic to support first/last placement modes.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread core/src/main/java/com/nisovin/magicspells/spells/MenuSpell.java
FIRST,
LAST;

private static SortMode fromConfigValue(String value) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably get rid of this, and use a normal Util#enumValueSafe. Unfortunately, you still have to uppercase, but feel free to change that.

This is not just to reduce redundancy, and localise the "default/fallback" value, but because it simplifies it too. Ignore the AI suggestion because that's even more of a roundabout way. Even a simple try/catch to parse the enum would've been enough.

Keep the config default, pass it to enumValueSafe, check if null was returned, then report that it was invalid and have it fall back to none.

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.

3 participants