Conversation
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements an auto unclaim feature that mirrors the existing auto claim functionality. When enabled via /faction unclaim auto, faction members with appropriate permissions can walk through their faction's territory to automatically unclaim chunks.
Changes:
- Added database migration to add
autounclaimboolean column tomf_factiontable - Extended
MfFactiondata model and repository with autounclaim field - Refactored
MfFactionUnclaimCommandto support subcommands, creating separateMfFactionUnclaimAutoCommandandMfFactionUnclaimCircleCommandclasses - Added autounclaim trigger logic to
PlayerMoveListenerto detect and delete claims when players enter their faction's territory - Added
TOGGLE_AUTOUNCLAIMfaction permission and corresponding Bukkit permissions - Added translations for all new messages across 5 supported languages (English US/GB, French, Portuguese BR, German)
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| V9__Add_autounclaim.sql | Database migration adding autounclaim boolean column with default false |
| MfFaction.kt | Added autounclaim boolean field to faction data model |
| JooqMfFactionRepository.kt | Updated persistence layer to save/load autounclaim field |
| MfFactionUnclaimCommand.kt | Refactored to delegate to subcommands (auto/circle pattern) |
| MfFactionUnclaimAutoCommand.kt | New command to toggle autounclaim for faction |
| MfFactionUnclaimCircleCommand.kt | Extracted original unclaim logic to separate class |
| PlayerMoveListener.kt | Added autounclaim trigger logic when entering owned chunks |
| MfFactionPermissions.kt | Added TOGGLE_AUTOUNCLAIM permission |
| MfFactionHelpCommand.kt | Added autounclaim help text to command list |
| lang_*.properties (x5) | Added translations for autounclaim messages in all 5 languages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CommandFactionHelpFaction=/faction - Faction management commands | ||
| CommandFactionHelpFactionAlly=/faction ally [faction] - Ally with a given faction | ||
| CommandFactionHelpFactionAutoclaim=/faction autoclaim - Enable autoclaim for your faction | ||
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Enable autounclaim for your faction |
There was a problem hiding this comment.
The help text shows /faction autounclaim but the actual command structure is /faction unclaim auto based on the implementation in MfFactionUnclaimCommand.kt. The help text should be corrected to /faction unclaim auto to match the actual command structure.
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Enable autounclaim for your faction | |
| CommandFactionHelpFactionAutounclaim=/faction unclaim auto - Enable autounclaim for your faction |
There was a problem hiding this comment.
Fixed in cc7dfac - Changed help text to /faction unclaim auto
| CommandFactionHelpFaction=/faction - Faction management commands | ||
| CommandFactionHelpFactionAlly=/faction ally [faction] - Ally with a given faction | ||
| CommandFactionHelpFactionAutoclaim=/faction autoclaim - Enable autoclaim for your faction | ||
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Enable autounclaim for your faction |
There was a problem hiding this comment.
The help text shows /faction autounclaim but the actual command structure is /faction unclaim auto. The help text should be corrected to /faction unclaim auto to match the actual command structure.
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Enable autounclaim for your faction | |
| CommandFactionHelpFactionAutounclaim=/faction unclaim auto - Enable autounclaim for your faction |
There was a problem hiding this comment.
Fixed in cc7dfac - Changed help text to /faction unclaim auto
| CommandFactionHelpFaction=/faction - Commandes de gestion de faction | ||
| CommandFactionHelpFactionAlly=/faction ally [faction] - S''allier � une faction donn�e | ||
| CommandFactionHelpFactionAutoclaim=/faction autoclaim - Activer l''autoclaim pour votre faction | ||
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Activer l''autounclaim pour votre faction |
There was a problem hiding this comment.
The help text shows /faction autounclaim but the actual command structure is /faction unclaim auto. The help text should be corrected to /faction unclaim auto to match the actual command structure.
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Activer l''autounclaim pour votre faction | |
| CommandFactionHelpFactionAutounclaim=/faction unclaim auto - Activer l''autounclaim pour votre faction |
There was a problem hiding this comment.
Fixed in cc7dfac - Changed help text to /faction unclaim auto
| CommandFactionHelpFaction=/faction - Comandos de gerenciamento de facção | ||
| CommandFactionHelpFactionAlly=/faction ally [faction] - Aliar-se a uma determinada facção | ||
| CommandFactionHelpFactionAutoclaim=/faction autoclaim - Ative a reivindicação automática para sua facção | ||
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Ative a desreivindicação automática para sua facção |
There was a problem hiding this comment.
The help text shows /faction autounclaim but the actual command structure is /faction unclaim auto. The help text should be corrected to /faction unclaim auto to match the actual command structure.
| CommandFactionHelpFactionAutounclaim=/faction autounclaim - Ative a desreivindicação automática para sua facção | |
| CommandFactionHelpFactionAutounclaim=/faction unclaim auto - Ative a desreivindicação automática para sua facção |
There was a problem hiding this comment.
Fixed in cc7dfac - Changed help text to /faction unclaim auto
| CommandFactionHelpFactionAutoclaim=/fraktion autoanspruch - Aktiviere den Autoanspruch f�r deine Fraktion | ||
| CommandFactionHelpFactionAlly=/fraktion verb�ndeter [faction] - Verb�nde dich mit einer bestimmten Fraktion | ||
| CommandFactionHelpFactionAutoclaim=/fraktion autoanspruch - Aktiviere den Autoanspruch f�r deine Fraktion | ||
| CommandFactionHelpFactionAutounclaim=/fraktion autoaufhebung - Aktiviere die Autoaufhebung f�r deine Fraktion |
There was a problem hiding this comment.
The help text shows /fraktion autoaufhebung but the actual command structure is /fraktion deklarieren auto (based on the English equivalent /faction unclaim auto). The help text should be corrected to match the actual command structure.
| CommandFactionHelpFactionAutounclaim=/fraktion autoaufhebung - Aktiviere die Autoaufhebung f�r deine Fraktion | |
| CommandFactionHelpFactionAutounclaim=/fraktion deklarieren auto - Aktiviere die Autoaufhebung f�r deine Fraktion |
There was a problem hiding this comment.
Fixed in cc7dfac - Changed help text to /fraktion deklarieren auto
| if (newChunkClaim != null && newChunkClaim.factionId.value == playerFaction.id.value && playerFaction.autounclaim) { | ||
| val playerRole = playerFaction.getRole(mfPlayer.id) ?: return@Runnable | ||
| val unclaimPermissionValue = playerRole.getPermissionValue(plugin.factionPermissions.unclaim) ?: return@Runnable | ||
| if (!unclaimPermissionValue || !event.player.hasPermission("mf.unclaim")) { |
There was a problem hiding this comment.
The permission check on line 95 verifies both the faction permission (unclaimPermissionValue) and the Bukkit permission (mf.unclaim). However, this check returns early silently if the player lacks permissions. When autounclaim is enabled but the player lacks permissions, they will walk through their territory without any feedback that autounclaim failed due to insufficient permissions. Consider adding a message to inform the player why autounclaim didn't work, similar to how autoclaim shows messages when it's automatically disabled due to power limits or contiguity issues.
| if (!unclaimPermissionValue || !event.player.hasPermission("mf.unclaim")) { | |
| if (!unclaimPermissionValue || !event.player.hasPermission("mf.unclaim")) { | |
| plugin.server.scheduler.runTask( | |
| plugin, | |
| Runnable { | |
| event.player.sendMessage("${RED}You do not have permission to automatically unclaim territory.") | |
| } | |
| ) |
| if (playerFaction != null) { | ||
| if (newChunkClaim != null && newChunkClaim.factionId.value == playerFaction.id.value && playerFaction.autounclaim) { | ||
| val playerRole = playerFaction.getRole(mfPlayer.id) ?: return@Runnable | ||
| val unclaimPermissionValue = playerRole.getPermissionValue(plugin.factionPermissions.unclaim) ?: return@Runnable | ||
| if (!unclaimPermissionValue || !event.player.hasPermission("mf.unclaim")) { | ||
| return@Runnable | ||
| } | ||
| claimService.delete(newChunkClaim).onFailure { | ||
| plugin.logger.log(SEVERE, "Failed to delete chunk claim: ${it.reason.message}", it.reason.cause) | ||
| return@Runnable | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The null check for playerFaction on line 91 is redundant. The variable playerFaction is already checked for null on line 40, and between lines 40-90, there's no code that could make it null again (it's a val, not reassigned). The autounclaim block could be merged into the same if-block as the autoclaim logic on line 40, or at minimum, this redundant null check should be removed.
There was a problem hiding this comment.
Fixed in cc7dfac - Removed redundant null check by merging autounclaim logic into the same if-block as autoclaim (line 40)
|
@copilot address comments |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
/faction unclaim autoinstead of/faction autounclaimacross all 5 language filesplayerFactionby merging autounclaim logic into same if-blockImplementation Notes
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.