ExprLore - Cleanup and Changes

[Fusezion]

Description

This PR aims to rewrite cleanup the ExprLore class as well as make a test file explicitly for itself.

In addition to all the amazing changes below, I've added early support for 1.21.5's new lore cap, so long as their server is running 1.21.5 the lore will cap itself out at 256

Breaking changes

Yes, I know the team hates them (I don't), but the behaviors were just wrong, and I can't see a reason to deprecate it if nothing else in skript behaves like this. Please read the information below, before reviewing this PR, I don't want to cause too many repeated answers that is already answered below.

The changes here were done to help make skript better and more flexible, I'm well aware of skript's love to lock event-item from modifications, but this should change if the ability isn't overly harmful to the player's experience.

Click for information about changer breaking changes

---

add, remove and remove all changers have been fully reworked to work only on a list bases than a single string line bases.

ADD

In the past add "test" to line 1 of player's tool's lore internally was just doing line 1 of lore + "test" this made zero sense since add is not something you would expect for concatenation, now that skript has both String + String and concat(String...) additionally join x with y I believe it's best to fully strip out this behavior in favor of only accepting add in add x to lore of y

REMOVE/REMOVE_ALL

Similar to ADD the REMOVE and REMOVE_ALL also had wonky behavior when used along side both lore of x and line x of lore of y

The remove method previously would convert a list like "My", "String", "Coolio" into "My\nString\nCoolio" and use .replace() and/or .replaceAll(), this lead to lore not actually being removed but instead just becoming blank i.e.
remove "String" from lore of {_item} -> "My\n\nCoolio" -> "My", "", "Coolio" this has been changed to now act upon list removal making it explicitly only when doing lore of x and is no longer an accepted change mode for line x of lore of y

I've fully stripped out the creation of blanks in lore besides when using set line x of lore of y to "blank", outside of it's current lore size, I believe this behavior was well received and made sense on a user end of things. However remove will no longer create a blank line when used, meaning someone doing

loop 5 times:
    remove "x" from line loop-counter of lore of {_item}

Would instead need to be

loop 5 times:
    replace "x" with "" in line loop-counter of lore of {_item}

Click for information about the less strict ChangerUtils#acceptsChange

---

As someone will eventually point out the internal code no longer runs ChangerUtils.acceptsChange(item, ChangeMode.SET, ItemStack.class, ItemType.class) any time someone does a change, this is now exclusive to when item.getTime() is not EventValues.TIME_NOW. this allows for people to now do set lore of event-item to "hahahha" which was previously unsupported.

From the testing I've done there was no reason for this to be done for TIME_NOW, these *should* be the existing items meaning changes made to them should still apply to 90% of event-items in skript.

People doing set lore of past event-item to ... and set lore of future event-item to ... will still be checked against the ChangerUtils.acceptsChange and correctly return when the new event-value changers are active. Below is a complete list of events I tested this in to showcase the utility in it as well as how much more it opens to the user base. While it is regrettable that events which don't support it will not error, there are far fewer of those than events that it works in.

The events tested in these are events that had event-itemstack and event-itemtype on the Skripthub site, so new 2.11 events were not tested.

Tested events that worked

https://docs.skriptlang.org/events.html?search=#anvil_prepare
https://docs.skriptlang.org/events.html?search=#book_edit
https://docs.skriptlang.org/events.html?search=#book_sign
https://docs.skriptlang.org/events.html?search=#click
https://docs.skriptlang.org/events.html?search=#drop
https://docs.skriptlang.org/events.html?search=#elytra_boost
https://docs.skriptlang.org/events.html?search=#prepare_craft
https://docs.skriptlang.org/events.html?search=#enchant_prepare
https://docs.skriptlang.org/events.html?search=#enchant
https://docs.skriptlang.org/events.html?search=#riptide
https://docs.skriptlang.org/events.html?search=#stop_using_item
https://docs.skriptlang.org/events.html?search=#player_pickup_arrow
https://docs.skriptlang.org/events.html?search=#inventory_pickup
https://docs.skriptlang.org/events.html?search=#inventory_item_move
https://docs.skriptlang.org/events.html?search=#inventory_click
https://docs.skriptlang.org/events.html?search=#item_mend
https://docs.skriptlang.org/events.html?search=#item_damage
https://docs.skriptlang.org/events.html?search=#item_spawn
https://docs.skriptlang.org/events.html?search=#pick_up
https://docs.skriptlang.org/events.html?search=#spawn

Tested events that didn't work

https://docs.skriptlang.org/events.html?search=#craft
https://docs.skriptlang.org/events.html?search=#armor_change
https://docs.skriptlang.org/events.html?search=#dispense
https://docs.skriptlang.org/events.html?search=#entity_breed
https://docs.skriptlang.org/events.html?search=#item_merge

Niche Special cases

https://docs.skriptlang.org/events.html?search=#place - This works but causes items to become well infinite
https://docs.skriptlang.org/events.html?search=#inventory_drag - This works but I am labeling special as it's only for cursor slot when using event-item

NOTE: I am writing this with zero sleep, and if things aren't making sense please let me know so I can update the message.

---

Target Minecraft Versions: any
Requirements: none
Related Issues: #6245

[Fusezion]

Example of old behavior can be seen as

Example of new behavior can be seen as

[Efnilite]

while i agree with the changes made, i think this is probably too much of a breaking change to be worth it with no syntax change, since items are used everywhere in skript. i'd be okay with locking the new behaviour behind an extra syntax, so the new behaviour would be applied with remove all "s" from >all< lore of item for example.

there is precedent for locking better behaviour behind syntax, e.g. #6930. wdyt?

[Fusezion]

while i agree with the changes made, i think this is probably too much of a breaking change to be worth it with no syntax change, since items are used everywhere in skript. i'd be okay with locking the new behaviour behind an extra syntax, so the new behaviour would be applied with remove all "s" from >all< lore of item for example.

there is precedent for locking better behaviour behind syntax, e.g. #6930. wdyt?

Efy, Efy, our non responsive Efy, I truly hope I did not read you saying remove acting like replace was "better behaviour", remove is by no means replace. This is truly disappointing some of the most disappointment I've felt since the day of my birth.

Your suggestion for adding all is not enough for the little change it offered, it wouldn't fix the breaking issue which I have zero intention of doing so. all is used in places where list are expected, so all lore of item is still just lore of item, example being all items -> items, all players -> players, all %classinfo with supplier%s -> %classinfo with supplier%s (all entity types -> entity types)

The little effort people need to do for supporting replacing from remove is overly easy and simple. As I've said users only need to change remove "blank" from lore of player's tool -> replace "blank" in lore of player's tool with ""

Old behavior broke current behavior conventions, nothing else in skript new or old (not confirmed) did behavior like this.
Now I can acknowledge that while remove is easily replaced, add might be a bit more effort on the end user's end.

add "test" to line 1 of player's lore -> set line 1 of item's lore to line 1 of item's lore + "test", however I believe there's future improvement that could be added in place of it. Such as ChangeMode.APPEND, append %object% [on[ ]]to %~object% for append "test" on to line 1 of player's tool

I'm confident that most people did not know you can add onto a specific line of lore since even I didn't until I made this PR so for 7-8 years I've never known this. I've also never seen it used in help channels, but that could also be me entirely missing them.

---

I won't say it's amazing to have breaking changes, but there's a time when they are needed, I believe this is one of those times where the team/people shouldn't be dragging their feet in making a decision, #7712 is a time where taking a step back and deciding is fair and likely a good call. Lore of an item is a list, it's not a single lined string where replace should be used. Skript needs to follow a convention in handling changers and a list is not a single entry it's multiple. using remove "hello" from lore of player's tool should never make Hello! -> !, it should fail

As a further point, when skript eventually drops spigot and swaps to paper, the team will likely also have to convert to components later down the line. Converting components to string and back into a component will cause data loss depending on how they were made. You the team will likely have to make the decision I'm doing here eventually I believe now is better than never so when you eventually go through with this you don't have to rewrite the whole class..

[Pikachu920]

In my opinion, this seems like something that should start as a discussion. I think we are getting ahead of ourselves by making a PR right now.

[Absolutionism]

In my opinion, this seems like something that should start as a discussion. I think we are getting ahead of ourselves by making a PR right now.

I believe the PR being made, regardless of it's status of being accepted or not, is beneficial. It helps all to know what changes that should be considered and be a point of reference.

[Efnilite]

Your suggestion for adding all is not enough for the little change it offered, it wouldn't fix the breaking issue which I have zero intention of doing so. all is used in places where list are expected, so all lore of item is still just lore of item, example being all items -> items, all players -> players, all %classinfo with supplier%s -> %classinfo with supplier%s (all entity types -> entity types)

the exact syntax does not matter, as long as you are able to differentiate it from the old way of removing.

The little effort people need to do for supporting replacing from remove is overly easy and simple. As I've said users only need to change remove "blank" from lore of player's tool -> replace "blank" in lore of player's tool with ""

this is still a change users need to do. they may need to replace hundreds of these occurrences across years of scripts, which is why a non-breaking change is, in my opinion, the only acceptable solution here.

add "test" to line 1 of player's lore -> set line 1 of item's lore to line 1 of item's lore + "test"

the second option here is literally exactly what the point of using add is: to remove having to reference the same expression twice. why is the second option better?

As a further point, when skript eventually drops spigot and swaps to paper, the team will likely also have to convert to components later down the line. Converting components to string and back into a component will cause data loss depending on how they were made. You the team will likely have to make the decision I'm doing here eventually I believe now is better than never so when you eventually go through with this you don't have to rewrite the whole class.

i highly doubt components are going to be required as soon as they are implemented. we can't just break every chat string and item string. also, if we're trying to future-proof, why would we accept this pr if we're going to make it use components later on anyway?

Efy, Efy, our non responsive Efy, I truly hope I did not read you saying remove acting like replace was "better behaviour", remove is by no means replace. This is truly disappointing some of the most disappointment I've felt since the day of my birth.

finally, please refrain from rhetoric like this. how am i supposed to enjoy reviewing your prs if this is the level of respect i get for spending my voluntary free time on skript? if you do this again, i will not be reviewing your prs anymore. we should all respect each other as volunteers.

I will be taking over

[APickledWalrus]

Good work so far. Although breaking, I think these changes are for the better. I am undecided on the ADD/REMOVE changer support for individual lines. I think it can help shorten the lines, though I do agree it is a bit odd for Skript (compared to other syntax). I will ask around to gather some other opinions.

[APickledWalrus]

Is there an error if too much lore is passed?

[Fusezion]

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.

[APickledWalrus]

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

[Fusezion]

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

[APickledWalrus]

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

[Fusezion]

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;
}

[NotSoDelayed]

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

[APickledWalrus]

What is the warning you're getting here?

[Fusezion]

I believe the issue there was to do with casting delta

[Fusezion]

lazy fix for test