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

ModInfo i18n API #91

Closed
wants to merge 3 commits into from
Closed

Conversation

MattSturgeon
Copy link

@MattSturgeon MattSturgeon commented Feb 15, 2024

This PR provides a general i18n API to IModInfo that could be used in Neoforged to implement neoforged/NeoForge#639.

I've added a unit test to cover the new behavior, however (as a new contributor) I'm unclear on the best way to test the functionality in game.

That said, if these assumptions are correct, then everything should work as intended:

  • The I18NParser implementation that uses I18NExtension will return the i18n key when a translation is not present.
  • I18NParser's i18n map contains (at least) the same entries as Minecraft's Language instance.
  • I18NParser's i18n map is updated by LanguageManager when the language is changed or resource pakcs are reloaded.

Using the API

Translations from mod display name and description can be provided by adding the following keys to lang files:

  • "fml.menu.mods.info.name.[modid]"
  • "fml.menu.mods.info.description.[modid]"
    Where [modid] should be replaced with the actual mod ID.

These can then be accessed using new methods added to IModInfo (names subject to change):

  • IModInfo.getDisplayNameTranslated()
  • IModInfo.getDescriptionTranslated()
    Which return the translated string (if one exists), or the string defined in mods.toml otherwise.

This could instead return an Optional or nullable value if preferred? That way you could easily check if a translated name/description exists and explicitly fallback using Optional::orElseGet.

Tests

I've added a unit test for the new behavior. In order to do so, I had to make some preparatory changes elsewhere, which I've kept to their own commits for clarity.

As unit tests, they cannot check the assumptions I outlined above.

Mockito now supports static mocking, so remove (unused) powermock dependency.

Ignore `log` directories in all locations, as these can be written to by some tests.
@MattSturgeon
Copy link
Author

MattSturgeon commented Feb 22, 2024

I18nExtension#parseMessage (used by I18NParser) calls I18nExtension#parseMessageWithFallback with the i18n key set as the fallback. Therefore the first assumption is correct.

It'd be better if I18NParser provided a method accepting a fallback, as this PR would then be able to pass IModInfo::getDisplayName as the fallback supplier. Seeing as this PR breaks the FML API anyway, maybe that approach would be better. That approach would also make the test effectively pointless.

I18n#setLanguage is hooked into, so when the LanguageManager sets the language, I18nExtension gets updated. Therefore the last two assumptions are also correct.

We cannot depend on `I18nExtension` here, so the test mocks `I18NParser` and assumes that a parser will return the i18n-key when a translation does not exist.

Since it is a unit test, not an integration test, this is acceptable IMO.

The `Bindings` singleton was updated so that it's static methods can be mocked without an instance being created.
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.

1 participant