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

Translations: force json, convert template, default to existing key #2292

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

Moulberry
Copy link

Adds 3 new translation features (I can split into 3 PRs if needed)

  1. Add Force JSON Translation
    For me (and maybe others), the plugin isn't able to detect the Minecraft version and throws "Cannot determine MC version". This option makes it avoid erroring

  2. Add template for the Convert to Translation feature
    net.minecraft.client.resources.I18n isn't always desired, this option allows the template string to be changed

  3. Default to existing key when adding duplicate translation text
    Scans the translation file for an existing key that matches the translation text and suggests it inside the Convert to Translation dialog

Feature 3 only supports json translation files, support for lang files can be added if desired.

Apologies if there are any stylistic issues, I am not a Kotlin developer

src/main/kotlin/MinecraftSettings.kt Outdated Show resolved Hide resolved
src/main/kotlin/translations/TranslationFiles.kt Outdated Show resolved Hide resolved
src/main/kotlin/translations/TranslationFiles.kt Outdated Show resolved Hide resolved
@Earthcomputer
Copy link
Member

We have already dropped support for new features for MC 1.12, so I don't mind support for lang files being unimplemented.

…ld assist in implementing automatic yarn/mojmap I18n templating in the future
src/main/kotlin/MinecraftConfigurable.kt Outdated Show resolved Hide resolved
src/main/kotlin/MinecraftConfigurable.kt Outdated Show resolved Hide resolved
Copy link
Member

@Earthcomputer Earthcomputer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than this one thing, and also formatting needs to be fixed with ktlint

if (translationSettings.isUseCustomConvertToTranslationTemplate) {
translationSettings.convertToTranslationTemplate.replace("\$key", key)
} else {
"net.minecraft.client.resources.I18n.format(\"$key\")"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the point of adding a checkbox for a custom template to use the mappings manager?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from your previous message is that the mappings API isn't ready yet.
I added the checkbox so that when it's ready it can be integrated without requiring users to tick another box when they update.

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.

None yet

4 participants