GregTechCE
Fix getItemStackDisplayName on Servers
What:
Currently getItemStackDisplayName etc. are currently marked as CLIENT only.
That means that computer based mods (and other mods using that) can not get a proper name of an GTCE item.
How solved: This PR removes the CLIENT only check and implemnts a LocalisationUtils that uses the legacy I18n class on servers and the new I18n on clients.
Outcome:
The display name is now properly set.
Additional info:
As a draft I replaced all I18n usages with LocalisationUtils for easy of use.
In most cases this is not required and I can change that back where i needed if necessary.
Possible compatibility issue:
None
This likely will crash on server-side, when LocalisationUtils gets defined it won't find net.minecraft.client.resources.I18n.
This likely will crash on server-side, when
LocalisationUtilsgets defined it won't findnet.minecraft.client.resources.I18n.
@Rongmario I quickly tested this on a server and it fixed the issue and didn’t crash. Will do further testing later today.
I don't get it. Why would the language/locale settings on the server be relevant?
If I am running a server in German but with US clients, the US clients don't want German tooltips or German number formatting.
Unless your new server methods are always doing things in the context of a player and there is some network processing to send the language id to the server for each player, I don't see how this could ever work properly unless all clients are using the same language as the server?
Also, AFAIK Mojang don't ship language files with the server so this won't work for vanilla items will it?
@warjort
I don't get it. Why would the language/locale settings on the server be relevant?
This is required for ComputerCraft / OpenComputer / etc. automation, where the server itself is the "client".
Computers use the getItemStackDisplayName on the server to get the item name.
For example: I am currently developing a ComputerCraft script that interacts with a Discord Bot.
Before those changes ComputerCraft would always report the GregTech name as item.meta_item.name etc.
While all other mods (I tested) report the displayName correctly.
With those changes it correctly reports the item name.
Also, AFAIK Mojang don't ship language files with the server so this won't work for vanilla items will it?
he legacy I18n class that is uses the english language file on servers.
The screenshot above is taken from a server where previously it just showed item.meta_item.name as displayName
Before those changes ComputerCraft would always report the GregTech name as item.meta_item.name etc. While all other mods (I tested) report the displayName correctly.
That is the translation key of the item. GTCE (along with other mods like Forestry and even some vanilla items) uses the additional "meta" to implment many items in one real minecraft item. This is partly to work around the 65535 limit of number of different items before minecraft 1.13, but it also allows many items to be implemented together.
Additionally items are allowed to override getItemStackDisplayName to go beyond a simple translation key.
If you (or CC) are using the translation key directly then that is wrong in principle.
If you (or CC) are using the translation key directly then that is wrong in principle.
CC (and other Computer based mods) use getItemStackDisplayName to get the items name (there is no other/better way to get item names [on a server] in 1.12 as far as I am aware).
The issue in GTCE is that this function is implemented as CLIENT only in many cases therefore CC will fallback to the the translation key (the base implementation and in most mods getItemStackDisplayName it is not client only).
By removing that CLIENT only check and implementing it in a way that works (and doesn't crash) on servers Computer based mods are able to get the items name (see screenshot above).
This is required for ComputerCraft / OpenComputer / etc. automation, where the server itself is the "client". Computers use the getItemStackDisplayName on the server to get the item name.
I am not sure you understood my question. Let me rephrase it a bit.
Why would the language/locale settings of the CC code running on the minecraft server be relevant to the player using a different language/locale settings - the one actually viewing the screen?
he legacy I18n class that is uses the english language file on servers.
To me this looks like something that "works for my use case" but is probably broken for everybody else?
The issue in GTCE is that this function is implemented as CLIENT only in many cases therefore CC will fallback to the the translation key (the base implementation and in most mods getItemStackDisplayName it is not client only).
Ok. But I can see opening this up to be used on the server will lead to subtle bugs where people call that method on the server when they should be deferring it to the client.
Why would the language/locale settings of the CC code running on the minecraft server be relevant to the player using a different language/locale settings - the one actually viewing the screen?
I don't think CC can do localization like that.
It's more about getting a readable name instead of getting nothing.
In my use case i query an AE2 system from an Discord Client and I then display the displayName in Discord.
Missing 5x item.meta_item.item and 2x item.meta_item.item is not really helpful for example.
Displaying 5x xx dust and 2x yy ingots is at least helpful even if the user doesn't speak english.
To me this looks like something that "works for my use case" but is probably broken for everybody else?
Clients still use the new I18n class. Only when it's called from a server (so from Computers) the legacy class is used. So this doesn't affect clients at all.
Clients still use the new I18n class. Only when it's called from a server (so from Computers) the legacy class is used. So this doesn't affect clients at all.
I am definitely against you changing every usage of I18n to your new LocalisationUtils (even if it uses I18n internally). If this hack to allow translation on the server is to be allowed at all, it should be restricted to where it is needed (the item stack display name).
Currently if a programmer attempts to use I18n in the wrong place they will get a crash when running in server mode. They immediately know they have a bug to fix during initial testing.
With your change it will display the wrong output until somebody notices it and reports it as a bug. The programmer is unlikely to be aware unless they go the extra mile and run the client/server in different languages while testing.
You should only use it in places you explicitly do NOT mark as client side only, like getStackDisplayName
It would also make sense to mark LocalisationUtils as deprecated and put some javadoc in it to explain when/why it should be used. That way people won't try to use it without thinking first.
@Archengius @warjort I update the PR to only use LocalisationUtils when actually needed. In addidion I marked LocalisationUtils as deprecated with an explanation in the docs.
I would change the javadoc to make the following points clear:
- It is intended that translations should be done using I18n on the client
- For setting up translations on the server you should use TextComponentTranslatable
- LocalisationUtils is only for cases where some kind of a translation is required on the server and there is no client/player in context
- LocalisationUtils is "best effort" and will probably only work properly with en-us
While you are there can you change the parameter names/descriptions to something like: first parameter -> translation key in the language file second parameter -> array of substitutions values to be inserted into the formatted translated text
I been using this PR for the past 7 days on my server. And it works as expected.