fabric
fabric copied to clipboard
FabricMC
Add module to load default mod language files on the dedicated server
~This PR adds a ServerLanguages API which does two things:~ ~1) Loads all present mods en_us lang files into the Language instance on the dedicated server. 2) Allows mods to set what language they would like their translations keys to map to on the dedicated server, in case a mod that adds commands would like their commands to be translatable on the actual server console.~
See comments below: We've agreed the API was messy and not a good solution for what it was trying to solve. Now this PR is only for loading mods' en_us lang files on the dedicated server. As this seems like something mods could benefit from having done for them.
The motivation for this PR came from an issue I had while contributing to CC: Restitched, a CC: Tweaked port for fabric. We needed the server to be able to translate text before sending it to the client, but to our surprise the dedicated server only ever loads minecraft's en_us lang file, so our translation keys were being sent raw. CC: Restitched also has a management command that uses translatable text, which shows raw translation keys in the dedicated server console without a fix like this. So, rather than just it fix it for ourselves I thought I'd put some sort of solution out there that could work in the Fabric API.
~I imagine most mod authors who benefit from this change will never actually interact with this API.~ Their translation keys will load on the dedicated server (for the server's default language, en_us) and they won't have do that work themselves. ~But for some added functionality I put in the ability for mods to change which language they want their mod to use on the dedicated server in case they need to localize management commands.~
Hope this is helpful or can be adapted to suite broader needs.
Wouldn't it be better to have a single language for the entire server, preferably exposed in the server.properties file?
I think an entire module for this is overkill, maybe resource loader can work for this?
This PR has many many issues.
First of all, this does not respect the language loading at all, when languages are loaded it's loaded from every namespaces and resource packs present, so assuming that a mod's language file is in its namespace is wrong.
Secondly, this uses a getResourceAsStream which is extremely terrible due to the fact that there is resource leaking with the class loader, it must use NIO.
Thirdly, there's similar libraries that already exist in the Fabric environment and goes much farther, and does a better job like https://github.com/arthurbambou/Server-Translations, this one will attempt to translate in the language of the players instead of a single common language specified on the server.
I originally had it as a global language but I thought it sent the wrong message to mod authors who might think that the entire server's language could be changed (it can't because Mojang only ships the en_us lang file in the server jar, and all the logging text is not translatable anyway). Making it per mod might better communicate to mod authors that it's their responsibility provide configuration options for localization?
Also I agree that having this whole module for really just one important change (loading mods' en_us lang files on the dedicated server) is pretty awkward. I'm not very familiar with the structure of fabric api but I didn't see anywhere for just "patches".
This PR has many many issues.
First of all, this does not respect the language loading at all, when languages are loaded it's loaded from every namespaces and resource packs present, so assuming that a mod's language file is in its namespace is wrong.
Secondly, this uses a getResourceAsStream which is extremely terrible due to the fact that there is resource leaking with the class loader, it must use NIO.
Thirdly, there's similar libraries that already exist in the Fabric environment and goes much farther, and does a better job like https://github.com/arthurbambou/Server-Translations, this one will attempt to translate in the language of the players instead of a single common language specified on the server.
Is there an avenue to having just something more basic than those libraries in the fabric api itself? I figured for many mod authors adding a whole library just to have lang files loaded on the dedicated server wouldn't seem appealing.
Yes that's the main feature I'm looking for. I probably shouldn't have tried to shoehorn in something api like but fabric doesn't seem to have a designated place for small enhancements like that.
Is there an avenue to having just something more basic than those libraries in the fabric api itself? I figured for many mod authors adding a whole library just to have lang files loaded on the dedicated server wouldn't seem appealing.
Language stuff is about accessibility, providing something that is a bit half-assed compared to the alternatives is not better. And why would it be not appealing? It's literally 2 lines in the build.gradle? Like for FAPI it's also lines in the build.gradle.
Fabric has literally a feature called Jar-In-Jar too to solve the dependency problem.
And as I know if "just having something basic" is important then we would already have a Fluid API merged months ago, a config API, #1371 would be merged.
Yes that's the main feature I'm looking for. I probably shouldn't have tried to shoehorn in something api like but fabric doesn't seem to have a designated place for small enhancements like that.
I think that would be suitable in resource loader v0, especially without API changes.
Is there an avenue to having just something more basic than those libraries in the fabric api itself? I figured for many mod authors adding a whole library just to have lang files loaded on the dedicated server wouldn't seem appealing.
Language stuff is about accessibility, providing something that is a bit half-assed compared to the alternatives is not better. And why would it be not appealing? It's literally 2 lines in the build.gradle? Like for FAPI it's also lines in the build.gradle.
Fabric has literally a feature called Jar-In-Jar too to solve the dependency problem.
And as I know if "just having something basic" is important then we would already have a Fluid API merged months ago, a config API, #1371 would be merged.
Most people and most servers already require Fabric API, having to add another dependency for this is not great.
I'd wait to see what @sfPlayer1 and @modmuss50 would think of loading en_us mod files on the server, to check if this feature is wanted.
I think that would be suitable in resource loader v0, especially without API changes.
This goes beyond resource loader imo, resource loader is, and should stay as "load mod resource packs, provide virtual resource packs capabilities (in the future), provide built-in resource pack capabilities, provide programmer art resource pack injection, and listen for resource reloads"
Thank you I'll look into that. And @LambdAurora could you point me to some resources on what's wrong with getResourceAsStream? I'm doing exactly as minecraft vanilla code does to load these files so I would have thought everything would be fine there.
Edit: I looked into this myself and I think I understand some of the issues now. I'm pretty new to this, sorry.
Most people and most servers already require Fabric API, having to add another dependency for this is not great.
But having 500 config APIs is fine though, ironic.
As already said, Fabric provide library shadowing (include) capabilities already so this should not be a problem.
Is there an avenue to having just something more basic than those libraries in the fabric api itself? I figured for many mod authors adding a whole library just to have lang files loaded on the dedicated server wouldn't seem appealing.
Language stuff is about accessibility, providing something that is a bit half-assed compared to the alternatives is not better.
And why would it be not appealing? It's literally 2 lines in the build.gradle? Like for FAPI it's also lines in the build.gradle.
Fabric has literally a feature called Jar-In-Jar too to solve the dependency problem.
And as I know if "just having something basic" is important then we would already have a Fluid API merged months ago, a config API, #1371 would be merged.
Okay I guess I've stepped into some organizational politics so I'll thank you for your feedback and take my leave.
Thank you I'll look into that. And @LambdAurora could you point me to some resources on what's wrong with getResourceAsStream? I'm doing exactly as minecraft vanilla code does to load these files so I would have thought everything would be fine there.
the tl;dr is you demonstrate exactly what's wrong with it as you're using the Minecraft's class loader to get resources of mods, it means that mods can unpredictably override resources they should not, so you need to use Path's NIO to access resources, please look into ModNioResourcePack.
Hell I would recommend to get a hold of resource loader's ModResourcePackCreator to be able to include built-in resource packs too.
@LambdAurora Can we calm down please? This is not the right place for a rant.
@Toad-Dev sorry about the inconvenience, as I said I want the mainteners (modmuss and Player) to give their feedback on this feature. They have the final say whether it is in scope or not.
Okay I guess the last thing I'll say is that just loading those en_us files is the main thing I'd like to find a solution for (it happens automatically on a certain other patch heavy loader). Please don't judge the contents of this commit too harshly I'm willing to work with reviewers to find a solution. This was just the result of some tinkering.
@LambdAurora Can we calm down please? This is not the right place for a rant.
I'm pointing out the facts and how there's a double game played (unsure how to phrase this but I'm sure you understand what I mean). Where do I continue my "rant" then btw?
So about the pr itself, this seems to add two things:
- Loading of en_us language content from mods on dedicated server
- Loading of other language contents on dedicated server (exposed in the API)
The implementation approach (general idea) looks good. However, this pull request is affected by a few minor problems, including unconventional test (shouldn't system exit in any case; if test fails, should throw exception instead), wrong versioning (should be v1, as v0 are code from prior to modularization), missing license headers, constants like mod id don't have field named MOD_ID, etc.
To me, lambdaurora suggests integrating this with the resource loader system. From what I see, vanilla's server resource loading is dissociated with the resource system or data packs totally; this hardcoding of language path for now is good, and given vanilla doesn't allow dedicated server to have resource packs supplied to override language content, there will be many other facets about integrating into resource system to consider, which IMO is beyond this pr's scope. We can always have this module and integrate it with resources later.
Loading of other language contents on dedicated server (exposed in the API)
Imo this part is very debatable, I would focus on en_us lang files. The question would then be where to put it, as it would be an implementation-only change. I don't think a new module for this is worth it, I would prefer this to go in resource loader if we have no better choice of existing module.
The implementation approach (general idea) looks good. However, this pull request is affected by a few minor problems, including unconventional test (shouldn't system exit in any case; if test fails, should throw exception instead), wrong versioning (should be v1, as v0 are code from prior to modularization), missing license headers, constants like mod id don't have field named MOD_ID, etc.
I can't comment on the implementation, however for the module naming, etc... I would recommend waiting until we decide under which form this feature should be implemented, so that this doesn't have to be reworked multiple times.
Where do I continue my "rant" then btw?
@LambdAurora If you want to discuss this, please open a new issue, and I'll explain why this situation is imo different from the others you mentioned.
I agree with putting the language switch api for mods on hold; if we do add that, the resource loader would be a good home to it, as the resource loader and minecraft's resource system has a comprehensive api for selecting the active resources and handling overrides; but that's another story.
So my proposal:
This pr should be minimal and just loads English language from assets/<mod_id>/lang/en_us.json on dedicated servers; the module can model after the crash-reports module, which also has no code api but adds information of fabric mods to crash reports.
~Reflecting on this I realize the api functionality I've tried to expose is a bit silly and fraught with problems. It's confusing that one mod could have translation mappings loaded for a different language than another within the same language instance. And hard coding right-to-left obviously makes no sense in a global server language environment but even less in a mixed language environment. I was trying to be targeted with my approach, which I why I avoided resource pack code. (I think a lot of it is stripped from the server jar anyway but my information on that might be out of date). Can we strip out the api stuff and just focus on having en_us loaded whereever that code would be most at home?~
Oops was typing this out when liach answered my questions :)
Okay I removed the API as we've agreed it's bad. Now it just loads the en_us lang files. When decisions are made about where to put this, if it's appropriate for fabric api, and if the implementation needs changes I will update again. Thanks for the feedback.
I decided to squash the previous commits just to remove any confusion about the bad API code that was included then ripped out.
Player confirmed on Discord that this change is fine, you can go ahead! :)
If anyone has input on what to call this module I'd like to hear it. Server Language*s* doesn't feel right now that the scope is the default language only.
And I might need some help clearing that checkstyle error about package names. That's quite the regex pattern 👀.
The regex partially verifies the package name, it should be as follows:
base package name: net.fabricmc.fabric
+ api/implementation/mixin subpackage: api/impl/mixin
+ client/dedicated server/common env subpackage: client/server/<nothing>
+ module name subpackage, singular, may contain multiple .-separated parts
+ api only: module major version with v prefix (e.g. v1)
+ other subpackages as needed, all singular
@sfPlayer1 Can you approve running workflows (right above the comment box) so ci can check for checkstyle violations etc?
Thank you @liach for the quick review. 😄 I'll look into those changes.
To get an idea of if this collision resolution strategy is sufficient, I put this code into a test mod with some logging and loaded it up in AOF3. Here is the output:
[16:31:09] [main/INFO]: Loaded 21400 keys.
[16:31:09] [main/INFO]: Translation key collision for "block.minecraft.nether_wart_block"
[16:31:09] [main/INFO]: among mods [minecraft, blockus]
[16:31:09] [main/INFO]: picked value "Crimson Wart Block" from mod "blockus"
[16:31:09] [main/INFO]: Translation key collision for "block.minecraft.barrel"
[16:31:09] [main/INFO]: among mods [minecraft, blockus]
[16:31:09] [main/INFO]: picked value "Spruce Barrel" from mod "blockus"
[16:31:09] [main/INFO]: Translation key collision for "_comment"
[16:31:09] [main/INFO]: among mods [roughlyenoughitems, techreborn]
[16:31:09] [main/INFO]: picked value "Machines" from mod "techreborn"
So among ~200 mods only three duplicates occurred. I think it chose the right value in those instances too. Given duplicate keys are so rare, and mod authors should already be using caution with TranslatableText on the server: perhaps this basic strategy is enough?
I agree this current strategy is enough, given this strategy is quite deterministic. Since you say conflicts are rare, I wish you can just put the logging process into the language module itself (so users and pack makers can better debug, and it won't be spammy).
Requesting another look. The main advantage of this is that without server languages, console-typed commands will print translation keys for their localized text feedbacks.