search

ChestShop-authors / ChestShop-3

ChestShop - the chest & sign shop plugin for Minecraft Servers running Bukkit/Spigot/Paper
https://dev.bukkit.org/projects/chestshop
GNU Lesser General Public License v2.1
269 stars 175 forks source link

Fix ItemAlias #552

Open Fyrinlight opened 1 year ago

Fyrinlight commented 1 year ago

Since ItemUtil already performs MaxWidth handling, and the full length key is needed for ItemAliasModule, remove MaxWidth from ItemStringListener.

Phoenix616 commented 1 year ago

I don't think this is a good solution. Removing that makes it impossible to call the event with other max widths also the ItemAliasModule listener itself uses the max width from the event.

What issue exactly are you trying to solve here? Simply configuring the string with the sign max width in the aliases config should already solve this in most cases no?

I feel like the better solution would be having the alias module generate the aliases directly from the item and not the string code that was previously generated. Potentially that should also be done before the ItemStringListener (so with a lower priority) to not generate the name twice.

Fyrinlight commented 1 year ago

The change doesn't remove max width from the event, only prevents it from being truncated too early.

The itemalias can still use the max width from the event, but the issue was that the item code was being truncated before it arrived at the alias module so it could not look up the name in the bimap.

Phoenix616 commented 1 year ago

The change doesn't remove max width from the event, only prevents it from being truncated too early.

Yes it does? It changes it so that the string representing any non-aliased item is no longer shortened to the length set in the event.

The length in the event needs to be applied, the fact that the rest doesn't break with the change seems to actually a bug/wrong implementation but you shouldn't rely on that...

If you really believe that this is the correct approach then at least ensure that the item string can't be longer than the max width when the event is done being called e.g. by adding a new listener with highest priority which shorts any string in the event down to the correct length after it was calculated. I feel like that should've been the approach to begin with instead of shortening the name in the name getter listener as well as potentially again in the method that calls the event in the plugin itself (which other plugins calling the event might not do so they would get broken info right now...)