Conversation
There was a problem hiding this comment.
Pull request overview
This PR bumps the plugin to version 3.11.2 and refines island/home teleport and primary-island handling, while updating the build to target newer Paper and Java 21 settings.
Changes:
- Extend
IslandsManager.getIsland(World, UUID)to prefer the island the (online) player is currently on, and introduce a newhomeTeleportAsync(Island, User, boolean)overload plusgetPrimaryIsland(World, UUID)convenience method. - Adjust island creation (
NewIsland) and/is gologic to use the new APIs (including support for island-name destinations) and to better manage spawn/home locations and primary islands. - Update Gradle build configuration for version
3.11.2, newer Paper API, and consistent Java 21 compilation; adapt tests to the updated method overloads.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/world/bentobox/bentobox/api/commands/island/IslandExpelCommandTest.java | Tightens the Mockito verification to the homeTeleportAsync(World, Player) overload after new overloads were added. |
| src/main/java/world/bentobox/bentobox/managers/island/NewIsland.java | After island creation, now sets the island-based home location and primary island, and uses the new homeTeleportAsync(Island, User, boolean) for first-time teleports. |
| src/main/java/world/bentobox/bentobox/managers/IslandsManager.java | Changes getIsland(World, UUID) to check the player’s current island when online, adds getPrimaryIsland(World, UUID) and homeTeleportAsync(Island, User[, boolean]), and wires these into existing home-location logic. |
| src/main/java/world/bentobox/bentobox/database/objects/Island.java | Simplifies setSpawnPoint to avoid unnecessary setChanged() calls and to clone the stored Location. |
| src/main/java/world/bentobox/bentobox/api/commands/island/IslandGoCommand.java | Refactors /is go to distinguish between island names and home names, setting primary islands accordingly and using either the existing world-based teleport or the new island-based teleport. |
| build.gradle.kts | Bumps plugin version to 3.11.2, updates Paper API version, drops the Spigot compile-only dependency, and standardizes Java 21 compiler options for main and test sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public CompletableFuture<Void> homeTeleportAsync(Island island, User user, boolean newIsland) { | ||
| Location loc = island.getHome(""); | ||
| user.sendMessage("commands.island.go.teleport"); | ||
| goingHome.add(user.getUniqueId()); | ||
| readyPlayer(user.getPlayer()); | ||
| return Util.teleportAsync(Objects.requireNonNull(user.getPlayer()), loc).thenAccept(b -> { | ||
| // Only run the commands if the player is successfully teleported |
There was a problem hiding this comment.
homeTeleportAsync(Island, User, boolean) calls island.getHome("") directly and passes the result to Util.teleportAsync(...) without any null check or fallback. For islands that do not yet have a default home (e.g., no-paste creations or legacy islands without homes), island.getHome("") can be null, which will lead to a null Location being used for teleportation and likely a runtime exception. To keep behavior consistent with the existing world-based homeTeleportAsync overload, this method should resolve a safe home location (e.g., via getHomeLocation(island) or equivalent safe-location logic) and handle the case where no home exists before attempting teleport.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| public CompletableFuture<Void> homeTeleportAsync(Island island, User user) { | ||
| return homeTeleportAsync(island, user, false); | ||
| } | ||
|
|
||
| /** | ||
| * Teleport the user home | ||
| * @param island island | ||
| * @param user user | ||
| * @param newIsland true if this is a new island first time teleport | ||
| * @return future when it is done | ||
| */ | ||
| public CompletableFuture<Void> homeTeleportAsync(Island island, User user, boolean newIsland) { | ||
| Location loc = island.getHome(""); | ||
| user.sendMessage("commands.island.go.teleport"); | ||
| goingHome.add(user.getUniqueId()); | ||
| readyPlayer(user.getPlayer()); | ||
| return Util.teleportAsync(Objects.requireNonNull(user.getPlayer()), loc).thenAccept(b -> { | ||
| // Only run the commands if the player is successfully teleported | ||
| if (Boolean.TRUE.equals(b)) { | ||
| teleported(island.getWorld(), user, "", newIsland, island); | ||
| this.setPrimaryIsland(user.getUniqueId(), island); | ||
| } else { | ||
| // Remove from mid-teleport set | ||
| goingHome.remove(user.getUniqueId()); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The new overload homeTeleportAsync(Island, User, boolean) in IslandsManager is not covered by existing tests, whereas other teleport-related behaviors (e.g., the world-based homeTeleportAsync overloads and listeners using them) do have tests. Consider adding unit tests for this method, particularly for cases where the island has and does not have a default home, to ensure it behaves correctly and stays aligned with the existing safe-teleport logic.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@tastybento I've opened a new pull request, #2792, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@tastybento I've opened a new pull request, #2793, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: tastybento <4407265+tastybento@users.noreply.github.com>
Fix Javadoc @link syntax in getPrimaryIsland method
|
|
@tastybento I've opened a new pull request, #2794, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.