Skip to content

ExprLore - Cleanup and Changes #7743

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: dev/feature
Choose a base branch
from
Open

ExprLore - Cleanup and Changes #7743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e3a0ed6
ExprLore.java - Rewrite File
Fusezion Mar 23, 2025
5a02b5a
ExprLore.java - Fix hasItemMeta issue
Fusezion Mar 23, 2025
d3f80d1
ExprLore.sk - Basic Test Implementation
Fusezion Mar 23, 2025
06b514f
Update some time state change mode support
Fusezion Mar 23, 2025
e195f06
Fix pattern indention?
Fusezion Mar 24, 2025
b0255a7
Merge branch 'dev/feature' into enhancement/expr-lore-changes
Fusezion Mar 31, 2025
2f978e0
Apply suggestions from code review
Fusezion Mar 31, 2025
5bcdaf6
Merge branch 'dev/feature' into enhancement/expr-lore-changes
Fusezion Mar 31, 2025
a2fd3e9
Update src/main/java/ch/njol/skript/expressions/ExprLore.java
Fusezion Mar 31, 2025
1a73569
Merge branch 'dev/feature' into enhancement/expr-lore-changes
Fusezion Apr 7, 2025
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
309 changes: 129 additions & 180 deletions src/main/java/ch/njol/skript/expressions/ExprLore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,236 +1,181 @@
package ch.njol.skript.expressions;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.bukkit.Bukkit;
import org.bukkit.Material;
import org.bukkit.event.Event;
import org.bukkit.inventory.ItemStack;
import org.bukkit.inventory.meta.ItemMeta;
import org.jetbrains.annotations.Nullable;

import ch.njol.skript.Skript;
import ch.njol.skript.SkriptConfig;
import ch.njol.skript.aliases.ItemType;
import ch.njol.skript.bukkitutil.ItemUtils;
import ch.njol.skript.classes.Changer.ChangeMode;
import ch.njol.skript.classes.Changer.ChangerUtils;
import ch.njol.skript.doc.Description;
import ch.njol.skript.doc.Examples;
import ch.njol.skript.doc.Name;
import ch.njol.skript.doc.Since;
import ch.njol.skript.doc.*;
import ch.njol.skript.lang.Expression;
import ch.njol.skript.lang.ExpressionType;
import ch.njol.skript.lang.SkriptParser.ParseResult;
import ch.njol.skript.lang.SyntaxStringBuilder;
import ch.njol.skript.lang.util.SimpleExpression;
import ch.njol.skript.registrations.EventValues;
import ch.njol.util.Kleenean;
import ch.njol.util.Math2;
import ch.njol.util.StringUtils;
import ch.njol.util.coll.CollectionUtils;
import org.bukkit.event.Event;
import org.bukkit.inventory.ItemStack;
import org.bukkit.inventory.meta.ItemMeta;
import org.jetbrains.annotations.Nullable;

import java.util.ArrayList;
import java.util.List;

/**
* TODO make a 'line %number% of %text%' expression and figure out how to deal with signs (4 lines, delete = empty, etc...)
*
* @author joeuguce99
*/
@Name("Lore")
@Description("An item's lore.")
@Examples("set the 1st line of the item's lore to \"<orange>Excalibur 2.0\"")
@Example("""
set {_item} to diamond named "Bling Diamond" with lore "&bThe blingiest of diamonds"
add "With extra steps" to lore of {_item}
remove line 1 of lore of {_item} from lore of {_item}
set line 3 of lore of {_item} to "-----"
""")
@Since("2.1")
public class ExprLore extends SimpleExpression<String> {

private static final boolean IS_RUNNING_1_21_5 = Skript.isRunningMinecraft(1,21,5);

static {
Skript.registerExpression(ExprLore.class, String.class, ExpressionType.PROPERTY,
"[the] lore of %itemstack/itemtype%", "%itemstack/itemtype%'[s] lore",
"[the] line %number% of [the] lore of %itemstack/itemtype%",
"[the] line %number% of %itemstack/itemtype%'[s] lore",
"[the] %number%(st|nd|rd|th) line of [the] lore of %itemstack/itemtype%",
"[the] %number%(st|nd|rd|th) line of %itemstack/itemtype%'[s] lore");
Skript.registerExpression(ExprLore.class, String.class, ExpressionType.PROPERTY,
"[the] lore of %itemstack/itemtype%",
"%itemstack/itemtype%'[s] lore",
"line %number% of [the] lore of %itemstack/itemtype%",
"line %number% of %itemstack/itemtype%'[s] lore",
"[the] %number%(st|nd|rd|th) line of [the] lore of %itemstack/itemtype%",
"[the] %number%(st|nd|rd|th) line of %itemstack/itemtype%'[s] lore");
}

@Nullable
private Expression<Number> lineNumber;

@SuppressWarnings("null")
private @Nullable Expression<Number> lineNumber;
private Expression<?> item;

@SuppressWarnings({"unchecked", "null"})
@Override
public boolean init(final Expression<?>[] exprs, final int matchedPattern, final Kleenean isDelayed, final ParseResult parseResult) {
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
//noinspection unchecked
lineNumber = exprs.length > 1 ? (Expression<Number>) exprs[0] : null;
item = exprs[exprs.length - 1];
return true;
}

@Override
@Nullable
protected String[] get(final Event e) {
final Object i = item.getSingle(e);
final Number n = lineNumber != null ? lineNumber.getSingle(e) : null;
if (n == null && lineNumber != null)
return null;
if (i == null || i instanceof ItemStack && ((ItemStack) i).getType() == Material.AIR)
protected String @Nullable [] get(Event event) {
if (!validateItem(item.getSingle(event)))
return new String[0];
final ItemMeta meta = i instanceof ItemStack ? ((ItemStack) i).getItemMeta() : (ItemMeta) ((ItemType) i).getItemMeta();
if (meta == null || !meta.hasLore())
ItemStack itemStack = ItemUtils.asItemStack(item.getSingle(event));
assert itemStack != null; // Validated in validateItem
ItemMeta itemMeta = itemStack.getItemMeta();
//noinspection deprecation
List<String> itemLore = itemMeta.getLore();
if (itemLore == null || itemLore.isEmpty())
return new String[0];
final List<String> lore = meta.getLore();
assert lore != null; // hasLore() called before
if (n == null)
return lore.toArray(new String[0]);
final int l = n.intValue() - 1;
if (l < 0 || l >= lore.size())

if (lineNumber == null) {
return itemLore.toArray(String[]::new);
}

int loreIndex = this.lineNumber.getOptionalSingle(event).orElse(0).intValue() -1;
if (loreIndex < 0 || loreIndex >= itemLore.size()) {
return new String[0];
return new String[]{lore.get(l)};
}
return new String[]{itemLore.get(loreIndex)};
}

@Override
@Nullable
public Class<?>[] acceptChange(final ChangeMode mode) {
boolean acceptsMany = lineNumber == null;
switch (mode) {
case REMOVE:
case REMOVE_ALL:
case DELETE:
acceptsMany = false;
case SET:
case ADD:
if (ChangerUtils.acceptsChange(item, ChangeMode.SET, ItemStack.class, ItemType.class)) {
return CollectionUtils.array(acceptsMany ? String[].class : String.class);
}
return null;
case RESET:
default:
return null;
public Class<?> @Nullable [] acceptChange(ChangeMode mode) {
if (this.item.getTime() != EventValues.TIME_NOW && !ChangerUtils.acceptsChange(item, ChangeMode.SET, ItemStack.class, ItemType.class)) {
// Allows past/future expressions only if they accept the set change mode
return null;
}
boolean acceptsMany = this.lineNumber == null;
return switch (mode) {
case SET -> CollectionUtils.array(acceptsMany ? String[].class : String.class);
case DELETE -> CollectionUtils.array();
case ADD, REMOVE, REMOVE_ALL -> {
if (!acceptsMany) {
Skript.error("You cannot add to or remove from the lore from a single line.");
yield null;
}
yield CollectionUtils.array(String[].class);
}
default -> null;
};
}

@Override
public void change(final Event e, final @Nullable Object[] delta, final ChangeMode mode) throws UnsupportedOperationException {
Object i = item.getSingle(e);

String[] stringDelta = delta == null ? null : Arrays.copyOf(delta, delta.length, String[].class);

// air is just nothing, it can't have a lore
if (i == null || i instanceof ItemStack && ((ItemStack) i).getType() == Material.AIR)
return;

ItemMeta meta = i instanceof ItemStack ? ((ItemStack) i).getItemMeta() : (ItemMeta) ((ItemType) i).getItemMeta();
if (meta == null)
meta = Bukkit.getItemFactory().getItemMeta(Material.STONE);

Number lineNumber = this.lineNumber != null ? this.lineNumber.getSingle(e) : null;
List<String> lore = meta.hasLore() ? new ArrayList<>(meta.getLore()) : new ArrayList<>();

if (lineNumber == null) {
// if the condition below is true, the pattern with the line %number% expression was used,
// but the line number turned out to be null at runtime, meaning we should ignore it
if (this.lineNumber != null) {
return;
}

public void change(Event event, Object @Nullable [] delta, ChangeMode mode) {
Object item = this.item.getSingle(event);
if (!validateItem(item))
return; // Validates to ensure it is a valid item and has item meta.
ItemStack modifiedItem = ItemUtils.asItemStack(item);
assert modifiedItem != null; // validateItem has already run a check against this
ItemMeta itemMeta = modifiedItem.getItemMeta();
//noinspection deprecation
List<String> modifiedLore = itemMeta.hasLore() ? itemMeta.getLore() : new ArrayList<>();
assert modifiedLore != null; // lore can never be null here, if it's unset we create an empty list

if (this.lineNumber == null) {
switch (mode) {
case SET:
assert stringDelta != null;
List<String> newLore = new ArrayList<>();
for (String line : stringDelta) {
if (line.contains("\n")) {
Collections.addAll(newLore, line.split("\n"));
continue;
}
newLore.add(line);
}
lore = newLore;
break;
case ADD:
assert stringDelta != null;
List<String> addLore = new ArrayList<>();
for (String line : stringDelta) {
if (line.contains("\n")) {
Collections.addAll(addLore, line.split("\n"));
continue;
}
addLore.add(line);
case SET, DELETE -> modifiedLore = (delta != null) ? List.of((String[]) delta) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Casting delta is not guaranteed to be safe unfortunately, so you may need a different method of accomplishing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestions for this? SInce delta can only ever be string unless someone does an unsafe call manually through new ExprClass().change() or Changer#change(). The only other way I could think of going through is

List<String> providedStrings = new ArrayList<>();
if (delta != null) {
  for (Object object : delta) {
    if (!(object instanceof String stringDelta)
      continue;
    providedStrings.add(stringDelta);
  }
} else {
  providedStrings = null;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes what you did is safer — loop through the array and check the looped element

case ADD -> {
assert delta != null;
modifiedLore.addAll(List.of((String[]) delta));
}
case REMOVE, REMOVE_ALL -> {
boolean isAll = mode == ChangeMode.REMOVE_ALL;
if (isAll) {
//noinspection DataFlowIssue
Copy link
Member

Choose a reason for hiding this comment

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

What is the warning you're getting here?

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 believe the issue there was to do with casting delta

modifiedLore.removeAll(List.of((String[]) delta));
} else {
//noinspection DataFlowIssue
for (String string : (String[]) delta)
modifiedLore.remove(string);
}
lore.addAll(addLore);
break;
case DELETE:
lore = null;
break;
case REMOVE:
case REMOVE_ALL:
assert stringDelta != null;
lore = Arrays.asList(handleRemove(
StringUtils.join(lore, "\n"), stringDelta[0], mode == ChangeMode.REMOVE_ALL).split("\n"));
break;
case RESET:
assert false;
return;
}
}
} else {
// Note: line number is changed from one-indexed to zero-indexed here
int lineNum = Math2.fit(0, lineNumber.intValue() - 1, 99); // TODO figure out the actual maximum

// Fill in the empty lines above the line being set with empty strings (avoids index out of bounds)
while (lore.size() <= lineNum)
lore.add("");
int loreIndex = this.lineNumber.getOptionalSingle(event).orElse(0).intValue() -1;
if (loreIndex < 0)
return; // Cannot change anything in lore if it's negative, therefor we return
switch (mode) {
case SET:
assert stringDelta != null;
lore.set(lineNum, stringDelta[0]);
break;
case ADD:
assert stringDelta != null;
lore.set(lineNum, lore.get(lineNum) + stringDelta[0]);
break;
case DELETE:
lore.remove(lineNum);
break;
case REMOVE:
case REMOVE_ALL:
assert stringDelta != null;
lore.set(lineNum, handleRemove(lore.get(lineNum), stringDelta[0], mode == ChangeMode.REMOVE_ALL));
break;
case RESET:
assert false;
return;
case SET -> {
for (int line = modifiedLore.size()-1; line < loreIndex; line++) {
modifiedLore.add("");
}
//noinspection DataFlowIssue
modifiedLore.set(loreIndex, (String) delta[0]);
}
case DELETE -> {
if (loreIndex > modifiedLore.size() || !itemMeta.hasLore())
return; // Cannot change anything in lore, therefor we return
modifiedLore.remove(loreIndex);
}
}
}

meta.setLore(lore == null || lore.size() == 0 ? null : lore);
if (i instanceof ItemStack) {
((ItemStack) i).setItemMeta(meta);
} else {
((ItemType) i).setItemMeta(meta);
if (modifiedLore != null && !modifiedLore.isEmpty()) {
if (IS_RUNNING_1_21_5) {
// The maximum amount of lore an item can have is 256
// this change was made in 1.21.5
// source: https://minecraft.wiki/w/Data_component_format#lore
modifiedLore = modifiedLore.stream().limit(256).toList();
Copy link
Member

Choose a reason for hiding this comment

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

Is there an error if too much lore is passed?

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'm honestly not sure, I didn't test it without the limitation as I just went off previous behavior. I'll keep a note for this to test it and see if we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I was thinking if we can remove it that would be good. If not, this is fine as-is.

Copy link
Contributor Author

@Fusezion Fusezion Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Didn't feel like loading a server to quickly test lore limit, it appears we're safer adding this paper's CraftMetaItem will throw an exception if invalid amount of lore is provided

https://github.com/PaperMC/Paper/blob/9b1798d6438107fdf0d5939b79a8cf71f4d16e2c/paper-server/src/main/java/org/bukkit/craftbukkit/inventory/CraftMetaItem.java#L1386-L1399

While I was here went and checked it doesn't seem like there was a previous lore limit prior to 1.21.5 internally however I don't want to take the chance there to remove it, so I believe we're safer keeping this implemented as is, I could maybe implement a static field for max lore instead of splitting it as I did here

} else {
modifiedLore = modifiedLore.stream().limit(99).toList();
}
}

if (ChangerUtils.acceptsChange(item, ChangeMode.SET, i.getClass())) {
Object[] itemDelta = i instanceof ItemStack ? new ItemStack[]{(ItemStack) i} : new ItemType[]{(ItemType) i};
item.change(e, itemDelta, ChangeMode.SET);
} else {
Object[] itemDelta = i instanceof ItemStack ? new ItemType[]{new ItemType((ItemStack) i)} :
new ItemStack[]{((ItemType) i).getRandom()};
item.change(e, itemDelta, ChangeMode.SET);
//noinspection deprecation
itemMeta.setLore(modifiedLore);
if (item instanceof ItemType itemType) {
itemType.setItemMeta(itemMeta);
} else if (item instanceof ItemStack itemStack) {
itemStack.setItemMeta(itemMeta);
}
}

private String handleRemove(String input, String toRemove, boolean all) {
if (SkriptConfig.caseSensitive.value()) {
if (all) {
return input.replace(toRemove, "");
} else {
// .replaceFirst requires the regex to be quoted, .replace does it internally
return input.replaceFirst(Pattern.quote(toRemove), "");
}
} else {
final Matcher m = Pattern.compile(Pattern.quote(toRemove),
Pattern.CASE_INSENSITIVE | Pattern.UNICODE_CASE).matcher(input);
return all ? m.replaceAll("") : m.replaceFirst("");
}
private boolean validateItem(Object item) {
ItemStack itemStack = ItemUtils.asItemStack(item);
return itemStack != null && itemStack.getItemMeta() != null;
}

@Override
Expand All @@ -244,7 +189,11 @@ public Class<? extends String> getReturnType() {
}

@Override
public String toString(final @Nullable Event e, final boolean debug) {
return (lineNumber != null ? "the line " + lineNumber.toString(e, debug) + " of " : "") + "the lore of " + item.toString(e, debug);
public String toString(@Nullable Event event, boolean debug) {
SyntaxStringBuilder syntaxBuilder = new SyntaxStringBuilder(event, debug);
if (lineNumber != null)
return syntaxBuilder.append("line", lineNumber, "of the lore of", item).toString();
return syntaxBuilder.append("the lore of", item).toString();
}

}
Loading
Loading