Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new API for teleportation request events (PreTeleportRequestEvent and TeleportRequestEvent) and integrates them into the existing tpa and tpahere commands. The changes are logical and the new event API is well-defined.
However, I've found a few issues:
- There are duplicate registrations of a command and a listener in the example plugin, which could lead to bugs.
- More critically, there are several instances where a synchronous Bukkit event is being called from an asynchronous thread. This is a significant threading issue that can cause server instability and must be addressed.
My review includes detailed comments on these points with suggestions for how to resolve them.
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/TpaCommand.java
Outdated
Show resolved
Hide resolved
...ore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/self/TpaHereCommand.java
Outdated
Show resolved
Hide resolved
...ore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/self/TpaHereCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-api-example/src/main/java/com/eternalcode/example/EternalCoreApiExamplePlugin.java
Outdated
Show resolved
Hide resolved
eternalcore-api-example/src/main/java/com/eternalcode/example/EternalCoreApiExamplePlugin.java
Outdated
Show resolved
Hide resolved
| final Player sender = event.getSender(); | ||
| final Player target = event.getTarget(); |
There was a problem hiding this comment.
Why is there a need to explicitly declare final?
| final Player sender = event.getSender(); | |
| final Player target = event.getTarget(); | |
| Player sender = event.getSender(); | |
| Player target = event.getTarget(); |
...rc/main/java/com/eternalcode/example/feature/teleportrequest/ApiTeleportRequestListener.java
Outdated
Show resolved
Hide resolved
eternalcore-api-example/src/main/java/com/eternalcode/example/EternalCoreApiExamplePlugin.java
Outdated
Show resolved
Hide resolved
eternalcore-api-example/src/main/java/com/eternalcode/example/EternalCoreApiExamplePlugin.java
Outdated
Show resolved
Hide resolved
...ore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/self/TpaHereCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/TpaCommand.java
Outdated
Show resolved
Hide resolved
Rollczi
left a comment
There was a problem hiding this comment.
trzeba te eventy ogarnąć bo one są wołane async, a deklaracja jest false w konstruktorze - pytanie czy w sumie chcemy async może wolimy to sync dać
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleportrequest/TpaCommand.java
Outdated
Show resolved
Hide resolved
| /* | ||
| if (!countryService.areInSameCountry(sender, target)) { | ||
| sender.sendMessage("You are not in the same country!"); | ||
| event.setCancelled(true); | ||
| } | ||
| */ |
There was a problem hiding this comment.
??? remove dead code
| /* | |
| if (!countryService.areInSameCountry(sender, target)) { | |
| sender.sendMessage("You are not in the same country!"); | |
| event.setCancelled(true); | |
| } | |
| */ |
No description provided.