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

Fix ItemAlias #552

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Fyrinlight
Copy link

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

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

Phoenix616 commented May 9, 2023

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
Copy link
Author

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
Copy link
Member

Phoenix616 commented May 9, 2023

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants