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

SoundUtils - handle Sound class for enum/interface #7199

Merged
merged 8 commits into from
Nov 8, 2024
31 changes: 26 additions & 5 deletions src/main/java/ch/njol/skript/effects/EffPlaySound.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import ch.njol.skript.lang.Expression;
import ch.njol.skript.lang.SkriptParser.ParseResult;
import ch.njol.util.Kleenean;
import org.bukkit.Keyed;
import org.bukkit.Location;
import org.bukkit.NamespacedKey;
import org.bukkit.Sound;
Expand Down Expand Up @@ -72,6 +73,7 @@ public class EffPlaySound extends Effect {
private static final boolean ENTITY_EMITTER_SOUND = Skript.methodExists(Player.class, "playSound", Entity.class, Sound.class, SoundCategory.class, float.class, float.class);
private static final boolean ENTITY_EMITTER_STRING = Skript.methodExists(Player.class, "playSound", Entity.class, String.class, SoundCategory.class, float.class, float.class);
private static final boolean ENTITY_EMITTER = ENTITY_EMITTER_SOUND || ENTITY_EMITTER_STRING;
private static final boolean SOUND_IS_INTERFACE = Sound.class.isInterface();

public static final Pattern KEY_PATTERN = Pattern.compile("([a-z0-9._-]+:)?([a-z0-9/._-]+)");

Expand Down Expand Up @@ -149,11 +151,8 @@ protected void execute(Event event) {
// validate strings
List<NamespacedKey> validSounds = new ArrayList<>();
for (String sound : sounds.getArray(event)) {
NamespacedKey key = null;
try {
Sound enumSound = Sound.valueOf(sound.toUpperCase(Locale.ENGLISH));
key = enumSound.getKey();
} catch (IllegalArgumentException alternative) {
NamespacedKey key = getSoundKeyFromEnum(sound);
if (key == null) {
sound = sound.toLowerCase(Locale.ENGLISH);
Matcher keyMatcher = KEY_PATTERN.matcher(sound);
if (!keyMatcher.matches())
Expand Down Expand Up @@ -240,4 +239,26 @@ public String toString(@Nullable Event event, boolean debug) {
return builder.toString();
}

@SuppressWarnings({"deprecation", "unchecked", "rawtypes"})
Copy link
Member

Choose a reason for hiding this comment

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

this should be per statement. you should use the noinspection comment (alt enter > arrow down (suppress) > suppress for statement)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the point of this. Why remove 1 line to create 3+ more lines?
And since when did SkriptLang do this? I dont think ive ever done this nor ever seen it, nor is it in the code conventions.

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 using suppresswarnings on an entire method may lead to confusion as to when something generally considered not good is used (like casting raw types etc). using these comments just emphasizes where they actually occur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So is this a SkriptLang preference, or personal preference?
I personally think having a suppression for a block makes more sense than adding a suppression for every line in the block.

Copy link
Member

Choose a reason for hiding this comment

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

It's something we agreed on back in august or so. If it's not in the conventions yet it probably needs to be added. Warning suppression should be as specific as possible to avoid suppressing other warnings that weren't originally intended (see suppressing one deprecated method, but then a second gets deprecated too, it's good to still see the warning for the second.)
Of course, if the warnings are routine and/or the block is very small, like many init() methods, it's not worth suppressing each line individually.
In this case I can see it either way, I don't think it matters much, since the method's quite small.

Copy link
Member

Choose a reason for hiding this comment

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

If it's not in the conventions yet it probably needs to be added.

It is :)

Copy link
Member

Choose a reason for hiding this comment

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

image
ah it's in the nullness category instead of the formatting category

private static @Nullable NamespacedKey getSoundKeyFromEnum(String soundString) {
soundString = soundString.toUpperCase(Locale.ENGLISH);
// Sound.class is an Interface (rather than an enum) as of MC 1.21.3
if (SOUND_IS_INTERFACE) {
try {
Sound sound = Sound.valueOf(soundString);
return sound.getKey();
} catch (IllegalArgumentException ignore) {
}
} else {
try {
Enum soundEnum = Enum.valueOf((Class) Sound.class, soundString);
if (soundEnum instanceof Keyed) {
return ((Keyed) soundEnum).getKey();
}
} catch (IllegalArgumentException ignore) {
}
}
return null;
}

}
49 changes: 0 additions & 49 deletions src/main/java/ch/njol/skript/util/SoundUtils.java

This file was deleted.

3 changes: 3 additions & 0 deletions src/test/skript/tests/syntaxes/effects/EffPlaySound.sk
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
test "play sound effect":
play sound "block.stone.break" with volume 1 at spawn of world "world"
play sound "BLOCK_STONE_BREAK" with volume 1 at spawn of world "world"
ShaneBeee marked this conversation as resolved.
Show resolved Hide resolved