GH-971 Jail system refactor #1141

igoyek · 2025-08-21T09:06:28Z

The entire prison system has been improved and fixes from #971 have been applied.
All game changes and migrations have been tested.

Introduced JailCommandRestrictionType enum to support both whitelist and blacklist command restrictions for jailed players. Updated JailConfig and JailSettings to use 'restrictedCommands' and 'restrictionType' instead of 'allowedCommands'. Modified JailController logic to handle both restriction types. Took 12 minutes

Added a record of the player's previous location. Configuration values and messages have been migrated. Added constant permission values and used static imports for better appearance. Other cosmetic or quality improvements have been made. Took 49 minutes

gemini-code-assist

Code Review

This pull request refactors the jail system, introducing features like storing a player's last location before being jailed and a more flexible command restriction system (whitelist/blacklist). The refactoring also includes centralizing permission constants and cleaning up configuration keys and message structures. My review has identified a couple of issues: a critical bug where a new configuration migration is not registered, which could lead to data loss for users, and a logical flaw in the releaseAllPlayers method where event cancellations are not fully respected. I've also suggested a minor improvement to a configuration comment for better clarity. Overall, this is a solid refactoring with significant improvements, and addressing these points will make it even better.

eternalcore-core/src/main/java/com/eternalcode/core/configuration/migrations/Migrations.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailServiceImpl.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailConfig.java

P1otrulla

Good job, follow gemini suggestions 🙌

P1otrulla · 2025-08-21T09:18:00Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java

@@ -10,12 +13,19 @@ public class JailedPlayer {
    private final Instant detainedAt;
    private final Duration prisonTime;
    private final String detainedBy;
+    @Nullable
+    private final Location lastLocation;


Location -> Position, We don't keep the location because it creates memory leaks and has a lot of world dependencies

Ok but I think I will be obliged to use another serializer for Position class in database?

Jakubk15

Other than that, please apply Gemini's suggestions. It gave valuable feedback

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/messages/JailMessages.java

Refactored JailServiceImpl.releaseAllPlayers to avoid clearing all jailed players and prisoners at once, now only releasing non-cancelled events and handling null players. Updated JailConfig comments for clarity and registered new migration for jail section in Migrations. Took 8 minutes

Took 15 minutes

releasePrivate -> released

CitralFlo

Nice

Replaced custom Position class with Bukkit's Location for storing and handling player locations in jail-related classes. Updated constructors, field names, and database persister to reflect this change, improving compatibility and simplifying location management. Took 36 minutes

vLuckyyy · 2025-08-21T22:19:50Z

eternalcore-api/build.gradle.kts

@@ -6,5 +6,6 @@ plugins {

 dependencies {
    compileOnly("org.spigotmc:spigot-api:${Versions.SPIGOT_API}")
+    compileOnly("com.eternalcode:eternalcode-commons-bukkit:${Versions.ETERNALCODE_COMMONS}")


Position usage in API

trochę słabo będzie wystawiać API które jest relokowane

...eternalcode/core/configuration/migrations/Migration_0010_Move_jail_to_dedicated_section.java

vLuckyyy · 2025-08-21T22:23:08Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/JailPermissionConstant.java

+
+import com.eternalcode.annotations.scan.permission.PermissionDocs;
+
+public class JailPermissionConstant {


Only permissions that don’t come from commands should be documented here. Command annotations generate documentation automatically. @PermissionDocs is needed only when a permission doesn’t belong to a command, for example in a listener.

Took 58 minutes

Deleted PositionArgument class and removed references to invalidPosition messages in ArgumentMessages, ENArgumentMessages, and PLArgumentMessages. Updated HomeAdminCommand to use Location instead of Position for home setting, simplifying argument handling. Took 25 minutes

When setting a home for a user, the location now explicitly uses the sender's world, yaw, and pitch to avoid inconsistencies if a custom location is provided without these values. Took 8 minutes

Renamed migration class and references from 'Move_jail_to_dedicated_section' to 'Rename_jail_section' for clarity. Updated PositionPersister to use Position's toString method and added type safety. Minor import and formatting adjustments in JailedPlayer and PrisonerTable. Took 35 minutes

Took 2 minutes

Rollczi · 2025-08-26T20:34:30Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeCreateEvent.java

@@ -16,15 +18,16 @@ public class HomeCreateEvent extends Event implements Cancellable {
    private final UUID playerUniqueId;
    private final UUID homeUniqueId;
    private String homeName;
-    private Location location;
+    private Position position;


Rollczi · 2025-08-26T20:34:37Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeCreateEvent.java

@@ -43,12 +46,12 @@ public UUID getPlayerUniqueId() {
        return this.playerUniqueId;
    }

-    public Location getLocation() {
-        return this.location;
+    public Position getPosition() {


Rollczi · 2025-08-26T20:34:42Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/HomeOverrideEvent.java

@@ -17,16 +18,16 @@ public class HomeOverrideEvent extends Event implements Cancellable {
    private final UUID playerUniqueId;
    private final UUID homeUniqueId;
    private String homeName;
-    private Location location;
+    private Position position;


Rollczi · 2025-08-26T20:34:47Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/home/event/PreHomeTeleportEvent.java

@@ -16,27 +16,27 @@ public class PreHomeTeleportEvent extends Event implements Cancellable {

    private final UUID playerUniqueId;
    private final Home home;
-    private Location location;
+    private Position position;


Rollczi · 2025-08-26T20:37:02Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/jail/JailedPlayer.java


-    public JailedPlayer(UUID player, Instant detainedAt, Duration prisonTime, String lockedUpBy) {
+    public JailedPlayer(UUID player, Instant detainedAt, Duration prisonTime, String detainedBy, Position lastPosition) {


zmieniasz JailedPlayer na interface i robisz jakąś impl np. JailedCorePlayer/JailedPlayerImpl, metody w API mają nie wystawiać Position. (ze względu na to że to libka a nie część API)

Rollczi

Zmiany w home do wywalenia, migracje były testowane? I czy ten persister do DB był testowany? I czy API z jakrą było testowane? Nie mówię, że nie było albo że trzeba wszystko testować ale widzę parę miejsc które słabo rokują

szczególnie te zmiany w wystawianiu libki jako API #1141 (comment)

Rollczi · 2025-08-26T20:39:21Z

...core/configuration/migrations/Migration_0009_Rename_allowed_to_restricted_jail_commands.java

+
+    Migration_0009_Rename_allowed_to_restricted_jail_commands() {
+        super("Rename allowed to restricted jail commands",
+            move("jail.allowedCommands", "jail.restrictedCommands")


a jak to działa skoro mamy jailSection a nie jail

Rollczi · 2025-08-26T20:42:35Z

eternalcore-core/src/main/java/com/eternalcode/core/database/persister/PositionPersister.java

+        String s = (String) sqlArg;
+
+        if (s == null) {
+            return null;
+        }
+
+        String[] params = s.split("/");
+
+        if (params.length != 6) {
+            throw new IllegalArgumentException("Invalid position format: " + s);
+        }
+
+        return new Position(
+            Double.parseDouble(params[1]),
+            Double.parseDouble(params[2]),
+            Double.parseDouble(params[3]),
+            Float.parseFloat(params[4]),
+            Float.parseFloat(params[5]),
+            params[0]
+        );


szczerze zastanawia mnie czy ten kod działa, bo Position#toString zwraca format który parsuje Position.parse a nie po /

Rollczi · 2025-08-26T20:43:32Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomeImpl.java


 public class HomeImpl implements Home {

    private final UUID uuid;
    private final UUID owner;
    private final String name;
-    private final Location location;
+    private final Position position;


Rollczi · 2025-08-26T20:43:52Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomeManager.java

        Map<String, Home> homes = this.userHomes.computeIfAbsent(playerUniqueId, k -> new HashMap<>());

        Home home = homes.get(name);

        if (home != null) {
-            HomeOverrideEvent event = this.eventCaller.callEvent(new HomeOverrideEvent(playerUniqueId, name, playerUniqueId, location));
+            HomeOverrideEvent event = this.eventCaller.callEvent(new HomeOverrideEvent(playerUniqueId, name, playerUniqueId, position));


Rollczi · 2025-08-26T20:43:56Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/HomeManager.java

        this.eventCaller.callEvent(event);

        if (event.isCancelled()) {
            return null;
        }

-        Home homeInEvent = new HomeImpl(playerUniqueId, event.getHomeName(), event.getLocation());
+        Home homeInEvent = new HomeImpl(playerUniqueId, event.getHomeName(), event.getPosition());


Rollczi · 2025-08-26T20:44:02Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/command/SetHomeCommand.java

@@ -54,7 +55,7 @@ private void setOrOverrideHome(User user, Player player, String homeName) {
        UUID uniqueId = user.getUniqueId();

        if (this.homeService.hasHome(uniqueId, homeName)) {
-            this.homeService.createHome(uniqueId, homeName, player.getLocation());
+            this.homeService.createHome(uniqueId, homeName, PositionAdapter.convert(player.getLocation()));


Rollczi · 2025-08-26T20:44:54Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/home/database/HomeTable.java

-    @DatabaseField(columnName = "location", persisterClass = LocationPersister.class)
-    private Location location;
+    @DatabaseField(columnName = "location", persisterClass = PositionPersister.class)
+    private Position position;


unrelated i potencjalnie rozwala kompatybilność jeśli użyjemy poprawnie zaimplmentowanego PositionPersister

Rollczi · 2025-08-26T20:45:02Z

...nalcore-core/src/main/java/com/eternalcode/core/feature/home/homeadmin/HomeAdminCommand.java

@@ -122,7 +129,8 @@ void home(@Context Player player, @Arg("player home") PlayerHomeEntry playerHome
            return;
        }

-        PaperLib.teleportAsync(player, homeOption.get().getLocation());
+        Location homeLocation = PositionAdapter.convert(homeOption.get().getPosition());


igoyek added 2 commits August 21, 2025 09:31

igoyek requested review from vLuckyyy, Jakubk15, P1otrulla and CitralFlo August 21, 2025 09:06

igoyek self-assigned this Aug 21, 2025

igoyek requested a review from a team as a code owner August 21, 2025 09:06

igoyek added this to EternalCore Aug 21, 2025

igoyek added 🆕 feature New feature or request 🔧 enhancement Enhances existing function labels Aug 21, 2025

github-project-automation bot moved this to 📋 Backlog in EternalCore Aug 21, 2025

igoyek moved this from 📋 Backlog to 👀 In review in EternalCore Aug 21, 2025

igoyek linked an issue Aug 21, 2025 that may be closed by this pull request

Refactor jail system #971

Open

4 tasks

gemini-code-assist bot reviewed Aug 21, 2025

View reviewed changes

P1otrulla approved these changes Aug 21, 2025

View reviewed changes

Jakubk15 reviewed Aug 21, 2025

View reviewed changes

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/messages/JailMessages.java Outdated Show resolved Hide resolved

eternalcore-core/src/main/java/com/eternalcode/core/feature/jail/messages/JailMessages.java Outdated Show resolved Hide resolved

igoyek and others added 3 commits August 21, 2025 13:50

Need tests

5bf5d52

Took 15 minutes

detainPrivate -> detained

526e344

releasePrivate -> released

CitralFlo approved these changes Aug 21, 2025

View reviewed changes

igoyek requested a review from Jakubk15 August 21, 2025 20:57

vLuckyyy approved these changes Aug 21, 2025

View reviewed changes

igoyek added 3 commits August 22, 2025 14:05

Location -> Position refactor

ef0a5cb

Took 58 minutes

Ensure home location uses sender's world and orientation

d640f4c

When setting a home for a user, the location now explicitly uses the sender's world, yaw, and pitch to avoid inconsistencies if a custom location is provided without these values. Took 8 minutes

P1otrulla self-requested a review August 22, 2025 13:07

igoyek added 2 commits August 22, 2025 15:20

Remove unused import

6c38f6f

Took 2 minutes

igoyek assigned P1otrulla Aug 22, 2025

Jakubk15 approved these changes Aug 23, 2025

View reviewed changes

P1otrulla removed their request for review August 23, 2025 22:22

P1otrulla removed their assignment Aug 23, 2025

Rollczi reviewed Aug 26, 2025

View reviewed changes

Rollczi requested changes Aug 26, 2025

View reviewed changes

github-project-automation bot moved this from 👀 In review to 🏗 In progress in EternalCore Aug 26, 2025


		import com.eternalcode.annotations.scan.permission.PermissionDocs;

		public class JailPermissionConstant {


		public JailedPlayer(UUID player, Instant detainedAt, Duration prisonTime, String lockedUpBy) {
		public JailedPlayer(UUID player, Instant detainedAt, Duration prisonTime, String detainedBy, Position lastPosition) {

Uh oh!

GH-971 Jail system refactor #1141

Are you sure you want to change the base?

GH-971 Jail system refactor #1141

Conversation

igoyek commented Aug 21, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

P1otrulla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jakubk15 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CitralFlo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rollczi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rollczi left a comment •

edited

Loading