-
-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-835 Add permission-based access to warps #856
Changes from all commits
fca57f8
65e7564
374fc89
f1a6ada
7eb3bfc
1dbc78f
2e80db7
0aaf226
bf890c7
d11676f
2804b53
a8ac2e2
708224f
2a67403
95f8ed3
60b4c11
f86cc47
a84633d
e68afcc
a67693c
c9d7daf
c6115be
cec9d90
72feb45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2,10 +2,26 @@ | |||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import org.bukkit.Location; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import java.util.List; | ||||||||||||||||||||||||||||||||||||||
import org.bukkit.permissions.Permissible; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
public interface Warp { | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
Location getLocation(); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
String getName(); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
List<String> getPermissions(); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
default boolean hasPermissions(Permissible permissible) { | ||||||||||||||||||||||||||||||||||||||
List<String> permissions = this.getPermissions(); | ||||||||||||||||||||||||||||||||||||||
if (permissions.isEmpty()) { | ||||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
return permissions | ||||||||||||||||||||||||||||||||||||||
.stream() | ||||||||||||||||||||||||||||||||||||||
.anyMatch(permission -> permissible.hasPermission(permission)); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+16
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider using anyMatch directly instead of stream The current implementation works well, but for better performance you could use: - return permissions
- .stream()
- .anyMatch(permission -> permissible.hasPermission(permission));
+ return permissions.stream().anyMatch(permissible::hasPermission); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,18 +4,23 @@ | |
|
||
import java.util.Collection; | ||
import java.util.Optional; | ||
import org.jetbrains.annotations.ApiStatus.Experimental; | ||
|
||
public interface WarpService { | ||
|
||
Warp createWarp(String name, Location location); | ||
|
||
void removeWarp(String warp); | ||
|
||
boolean warpExists(String name); | ||
@Experimental | ||
Warp addPermissions(String warp, String... permissions); | ||
|
||
Optional<Warp> findWarp(String name); | ||
@Experimental | ||
Warp removePermissions(String warp, String... permissions); | ||
|
||
vLuckyyy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
boolean isExist(String name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider renaming This name is more conventional and readable. |
||
|
||
Collection<String> getNamesOfWarps(); | ||
Optional<Warp> findWarp(String name); | ||
|
||
boolean hasWarps(); | ||
Collection<Warp> getWarps(); | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,10 +1,14 @@ | ||||||||||
package com.eternalcode.core.feature.warp.event; | ||||||||||
|
||||||||||
import com.eternalcode.core.feature.warp.Warp; | ||||||||||
import com.google.common.base.Preconditions; | ||||||||||
import java.time.Duration; | ||||||||||
import org.bukkit.Location; | ||||||||||
import org.bukkit.entity.Player; | ||||||||||
import org.bukkit.event.Cancellable; | ||||||||||
import org.bukkit.event.Event; | ||||||||||
import org.bukkit.event.HandlerList; | ||||||||||
import org.jetbrains.annotations.Nullable; | ||||||||||
|
||||||||||
/** | ||||||||||
* Called before teleportation to warp. | ||||||||||
|
@@ -16,12 +20,15 @@ public class PreWarpTeleportEvent extends Event implements Cancellable { | |||||||||
private final Player player; | ||||||||||
private Warp warp; | ||||||||||
private boolean cancelled; | ||||||||||
private Duration teleportTime; | ||||||||||
private @Nullable Location destination; | ||||||||||
Comment on lines
+23
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider making teleportTime final. Since teleportTime is a required parameter in the constructor and represents a core property, consider making it final for better immutability. - private Duration teleportTime;
+ private final Duration teleportTime; 📝 Committable suggestion
Suggested change
|
||||||||||
|
||||||||||
public PreWarpTeleportEvent(Player player, Warp warp) { | ||||||||||
public PreWarpTeleportEvent(Player player, Warp warp, Duration teleportTime) { | ||||||||||
super(false); | ||||||||||
|
||||||||||
this.player = player; | ||||||||||
this.warp = warp; | ||||||||||
this.teleportTime = teleportTime; | ||||||||||
} | ||||||||||
|
||||||||||
public Player getPlayer() { | ||||||||||
|
@@ -33,9 +40,28 @@ public Warp getWarp() { | |||||||||
} | ||||||||||
|
||||||||||
public void setWarp(Warp warp) { | ||||||||||
Preconditions.checkNotNull(warp, "Warp cannot be null"); | ||||||||||
this.warp = warp; | ||||||||||
} | ||||||||||
|
||||||||||
public Duration getTeleportTime() { | ||||||||||
return this.teleportTime; | ||||||||||
} | ||||||||||
|
||||||||||
public void setTeleportTime(Duration teleportTime) { | ||||||||||
Preconditions.checkNotNull(teleportTime, "Teleport time cannot be null"); | ||||||||||
this.teleportTime = teleportTime; | ||||||||||
} | ||||||||||
Comment on lines
+47
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider removing setTeleportTime if field becomes final. If you make teleportTime final as suggested earlier, this setter would need to be removed. The getter can stay. |
||||||||||
|
||||||||||
public Location getDestination() { | ||||||||||
return this.destination != null ? this.destination : this.warp.getLocation(); | ||||||||||
} | ||||||||||
|
||||||||||
public void setDestination(Location destination) { | ||||||||||
Preconditions.checkNotNull(destination, "Destination cannot be null"); | ||||||||||
this.destination = destination; | ||||||||||
} | ||||||||||
Comment on lines
+56
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Smart fallback in getDestination. Good implementation of the destination getter with fallback to warp location. The null check in the setter is also appropriate. However, consider documenting the fallback behavior in a Javadoc comment: /**
* Gets the destination location for this teleport.
* @return the custom destination if set, otherwise falls back to the warp's location
*/ |
||||||||||
|
||||||||||
@Override | ||||||||||
public boolean isCancelled() { | ||||||||||
return this.cancelled; | ||||||||||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,16 +2,22 @@ | |
|
||
import com.eternalcode.commons.bukkit.position.Position; | ||
import com.eternalcode.commons.bukkit.position.PositionAdapter; | ||
import java.util.ArrayList; | ||
import org.bukkit.Location; | ||
|
||
class WarpImpl implements Warp { | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
public class WarpImpl implements Warp { | ||
|
||
private final String name; | ||
private final Position position; | ||
private final List<String> permissions; | ||
|
||
WarpImpl(String name, Position position) { | ||
public WarpImpl(String name, Position position, List<String> permissions) { | ||
this.name = name; | ||
this.position = position; | ||
this.permissions = new ArrayList<>(permissions); | ||
Comment on lines
+17
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add a null check for This helps prevent potential |
||
} | ||
|
||
@Override | ||
|
@@ -23,4 +29,10 @@ public String getName() { | |
public Location getLocation() { | ||
return PositionAdapter.convert(this.position); | ||
} | ||
|
||
@Override | ||
public List<String> getPermissions() { | ||
return Collections.unmodifiableList(this.permissions); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider returning an unmodifiable list of permissions
To prevent accidental modifications, wrap the permissions list with Collections.unmodifiableList().