Skip to content
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

Ban Entry Syntaxes #7257

Open
wants to merge 2 commits into
base: dev/feature
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 218 additions & 0 deletions src/main/java/ch/njol/skript/expressions/ExprBanData.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
package ch.njol.skript.expressions;

import ch.njol.skript.Skript;
import ch.njol.skript.classes.Changer;
import ch.njol.skript.doc.*;
import ch.njol.skript.lang.Expression;
import ch.njol.skript.lang.ExpressionType;
import ch.njol.skript.lang.SkriptParser;
import ch.njol.skript.lang.util.SimpleExpression;
import ch.njol.skript.util.Timespan;
import ch.njol.util.Kleenean;
import com.destroystokyo.paper.profile.PlayerProfile;
import io.papermc.paper.ban.BanListType;
import org.bukkit.BanEntry;
import org.bukkit.Bukkit;
import org.bukkit.OfflinePlayer;
import org.bukkit.event.Event;
import org.jetbrains.annotations.Nullable;

import java.net.InetAddress;
import java.net.UnknownHostException;

@Name("Ban Details")
@Description("Returns data about a player or IP ban. " +
"The source can be any string, but it's usually the player's name. " +
"The expiration date won't return a value if the ban is permanent.")
@Examples({"set {_player} to offlineplayer(\"Notch\", false)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Examples({"set {_player} to offlineplayer(\"Notch\", false)",
@Examples({
"set {_player} to offlineplayer(\"Notch\", false)",

"set {_expiration} to the date {_player}'s ban expires",
"set {_time.left} to difference between now and {_expiration}",
"set {_reason} to the reason {_player} was banned",
"send \"There is %{_time.left}% before %{_player}% gets unbanned! They were banned for '%{_reason}%'\" to player"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"send \"There is %{_time.left}% before %{_player}% gets unbanned! They were banned for '%{_reason}%'\" to player"})
"send \"There is %{_time.left}% before %{_player}% gets unbanned! They were banned for '%{_reason}%'\" to player"
})

@Since("INSERT VERSION")
@RequiredPlugins("Spigot 1.20.1+")
public class ExprBanData extends SimpleExpression<Object> {

static {
if (Skript.methodExists(BanEntry.class, "remove"))
Skript.registerExpression(ExprBanData.class, Object.class, ExpressionType.SIMPLE,
"[the] date %offlineplayer/string% was banned",
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax is not a good option, it can be very confusing for readers and might affect the parser. I suggest something like ban date of ... as Efnilite also suggested.

Same goes for the rest of syntaxes, ban X of Y

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current one seems much better than ban x of y to me.
Is there a reason you think it's poor?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date "SkriptDev" was banned seems off to me at first glance, it doesn't immediately gives you a hint that it's a ban date. IMO ban date is easier to quickly recognize it's a ban date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be improved by adding that:
the date that "SkriptDev" was banned

It buries the lead by like 2 words which i don't really think is an issue, especially when it's so much more natural/fluid imo. ban date of "SkriptDev" feels much more stilted and awkward and something i wouldn't actually say irl.

I'm not opposed to having it in addition, but i definitely think the current syntax is higher quality

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so do i add another syntax for ban date or ..?

"[the] date of %offlineplayer/string%'s ban",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%offlineplayer/string%'[s] ban date?


"[the] source of %offlineplayer/string%'s ban",

"[the] date %offlineplayer/string%'s ban expires",
"[the] expiration date of %offlineplayer/string%'s ban",

"[the] reason %offlineplayer/string% was banned",
"[the] reason for %offlineplayer/string%'s ban"
);
}

private BanEntryType entryType;
private Expression<Object> banTargetExpr; // offline player or string (ip)

@Override
public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) {
public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {

entryType = BanEntryType.fromInt(matchedPattern);
banTargetExpr = (Expression<Object>) expressions[0];
return true;
}

@Override
protected Object @Nullable [] get(Event event) {
if (banTargetExpr == null)
return null;

Object banTarget = banTargetExpr.getSingle(event);
BanEntry<?> banEntry;

if (banTarget instanceof String ipTarget)
banEntry = getBanEntry(ipTarget);
else if (banTarget instanceof OfflinePlayer playerTarget)
banEntry = getBanEntry(playerTarget);
else
return null;

if (banEntry == null)
return null;

return switch (entryType) {
case BAN_DATE -> {
java.util.Date creation = banEntry.getCreated();
ch.njol.skript.util.Date skriptCreation = new ch.njol.skript.util.Date(creation.getTime());
yield new Object[]{ skriptCreation };
}
case SOURCE -> new Object[]{ banEntry.getSource() };
case EXPIRE_DATE -> {
java.util.Date expiration = banEntry.getExpiration();
if (expiration == null)
yield null;
ch.njol.skript.util.Date skriptExpiration = new ch.njol.skript.util.Date(expiration.getTime());
yield new Object[]{ skriptExpiration };
}
case REASON -> new Object[]{ banEntry.getReason() };
};
}

@Override
public boolean isSingle() {
return true;
}

@Override
public Class<?> getReturnType() {
return Object.class;
}

@Override
public Class<?> @Nullable [] acceptChange(Changer.ChangeMode mode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Class<?> @Nullable [] acceptChange(Changer.ChangeMode mode) {
public Class<?> @Nullable [] acceptChange(ChangeMode mode) {

if (mode == Changer.ChangeMode.SET) {
return switch (entryType) {
case EXPIRE_DATE -> new Class<?>[]{ ch.njol.skript.util.Date.class };
case SOURCE, REASON -> new Class<?>[]{ String.class };
default -> null;
};
} else if (mode == Changer.ChangeMode.ADD || mode == Changer.ChangeMode.REMOVE) {
if (entryType == BanEntryType.EXPIRE_DATE)
return new Class<?>[]{ Timespan.class };
}
return null;
}

@Override
public void change(Event event, Object @Nullable [] delta, Changer.ChangeMode mode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void change(Event event, Object @Nullable [] delta, Changer.ChangeMode mode) {
public void change(Event event, Object @Nullable [] delta, ChangeMode mode) {

Object banTarget = banTargetExpr.getSingle(event);
BanEntry<?> banEntry;

if (banTarget instanceof String ipTarget)
banEntry = getBanEntry(ipTarget);
else if (banTarget instanceof OfflinePlayer playerTarget)
banEntry = getBanEntry(playerTarget);
else
return;

if (banEntry == null)
return;

if (mode == Changer.ChangeMode.SET) {
switch (entryType) {
case SOURCE -> {
String newSource = (String) delta[0];
if (newSource == null)
return;
banEntry.setSource(newSource);
}
case EXPIRE_DATE -> {
ch.njol.skript.util.Date newDate = (ch.njol.skript.util.Date) delta[0];
if (newDate == null)
return;
banEntry.setExpiration(new java.util.Date(newDate.getTimestamp()));
}
case REASON -> {
String newReason = (String) delta[0];
if (newReason == null)
return;
banEntry.setReason(newReason);
}
}
} else {
Timespan timespan = (Timespan) delta[0];
if (timespan == null)
return;

if (entryType == BanEntryType.EXPIRE_DATE) {
java.util.Date expiration = banEntry.getExpiration();
if (expiration == null)
return;
long newExpirationMillis;
if (mode == Changer.ChangeMode.ADD)
newExpirationMillis = expiration.getTime() + timespan.getAs(Timespan.TimePeriod.MILLISECOND);
else if (mode == Changer.ChangeMode.REMOVE)
newExpirationMillis = expiration.getTime() - timespan.getAs(Timespan.TimePeriod.MILLISECOND);
else
return;
banEntry.setExpiration(new java.util.Date(newExpirationMillis));
}
}
banEntry.save();
}

@Override
public String toString(@Nullable Event event, boolean debug) {
return switch (entryType) {
case BAN_DATE -> "the date " + banTargetExpr.toString(event, debug) + " was banned";
case SOURCE -> "the source of " + banTargetExpr.toString(event, debug) + "'s ban";
case EXPIRE_DATE -> "the date " + banTargetExpr.toString(event, debug) + "'s ban expires";
case REASON -> "the reason " + banTargetExpr.toString(event, debug) + " was banned";
};
}

private BanEntry<InetAddress> getBanEntry(String ipTarget) {
try {
InetAddress address = InetAddress.getByName(ipTarget);
return Bukkit.getBanList(BanListType.IP).getBanEntry(address);
} catch (UnknownHostException ignored) { // this only happens when you pass a url and it performs a lookup
return null;
}
}

private BanEntry<PlayerProfile> getBanEntry(OfflinePlayer playerTarget) {
return Bukkit.getBanList(BanListType.PROFILE).getBanEntry(playerTarget.getPlayerProfile());
}

private enum BanEntryType {
BAN_DATE, SOURCE, EXPIRE_DATE, REASON;

public static BanEntryType fromInt(int value) {
return switch (value) {
case 0, 1 -> BAN_DATE;
case 2 -> SOURCE;
case 3, 4 -> EXPIRE_DATE;
case 5, 6 -> REASON;
default -> null;
};
}
}

}
25 changes: 25 additions & 0 deletions src/test/skript/tests/misc/ban entries.sk
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
test "ban entries" when running minecraft "1.20.1":
ban offlineplayer("Njol", false) due to "creating skript" for 5 seconds
assert the date offlineplayer("Njol", false) was banned is now with "getting ban creation date for offline player doesn't work"
assert the reason offlineplayer("Njol", false) was banned is "creating skript" with "getting ban reason doesn't work"

ban "3.3.3.3" for 3 seconds
assert the date "3.3.3.3" was banned is now with "getting ban creation date for ip doesn't work"
assert the source of "3.3.3.3"'s ban is "Skript ban effect" with "getting ban source doesn't work"
assert the expiration date of "3.3.3.3"'s ban is 3 seconds later with "getting ban expiration date doesn't work"

# test changers
add 2 seconds to the expiration date of "3.3.3.3"'s ban
assert the expiration date of "3.3.3.3"'s ban is 5 seconds later with "adding to ban expiration date doesn't work"

remove 1 second from the expiration date of "3.3.3.3"'s ban
assert the expiration date of "3.3.3.3"'s ban is 4 seconds later with "removing from ban expiration date doesn't work"

set expiration date of "3.3.3.3"'s ban to 1 second later
assert the expiration date of "3.3.3.3"'s ban is 1 second later with "setting ban expiration date doesn't work"

set the source of "3.3.3.3"'s ban to "skript test"
assert the source of "3.3.3.3"'s ban is "skript test" with "setting ban source doesn't work"

set the reason "3.3.3.3" was banned to "hello"
assert the reason "3.3.3.3" was banned is "hello" with "setting ban reason doesn't work"
Loading