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 and improve translation strings #2471

Merged
merged 15 commits into from Sep 12, 2019

Conversation

@sfan5
Copy link
Member

commented Sep 10, 2019

Notable:

  • stairs API: register_stair_inner, register_stair_outer gain an additional parameter
  • cart & screwdriver description were changed to use newline for consistency
  • boats chat message was adjusted to remove []
mods/butterflies/init.lua Outdated Show resolved Hide resolved
mods/butterflies/init.lua Outdated Show resolved Hide resolved
@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

👎 👎 👎 :-(

Pretty much all changes that are not trivial are wrong:

  description = S(desc .. " Butterfly"),
  description = S("Hidden " .. desc .. " Butterfly"),
  description = S(description .. " Dye"),
          description = S(desc .. " Wool"),

Ugh. No! That's the mortal sin of translation: String concatenation (in the middle of something). Now you force all languages to use the same word order as English. This is wrong and even worse than the previous approach. The correct way would have been to collect each string “White Butterfly”, “Red Butterfly” and “Violet Butterfly” individually, in their FULL form. That way, translators can apply whatever weird grammar rules would change the words and don't have to hack around. If you try to be clever and basically construct the strings programmatically, you make assumptions about other languages and make some translations simply impossible. The word “Butterfly” might not always be translated the same depending on weird grammar rules in other languages.

   {"white",  "White"},
   {"red",    "Red"},
   {"violet", "Violet"}

And now, the butterfly color will not even be collected by scripts …

A proper way would have been:

   {"white",  S("White Butterfly")},
   {"red",    S("Red Butterfly")},
   {"violet", S("Violet Buterfly")}

   -- ...

    local desc = butter_list[i][2]
    minetest.register_node("butterflies:butterfly_"..name, {
  description = desc,

The same complaints apply for the other string concatenations …

+       stairs.register_stair_inner(subname, recipeitem, groups, images,
+              S("Inner " .. desc_stair), sounds, worldaligntex)
-       S("Wooden Stair"),
-       S("Wooden Slab"),
+       "Wooden Stair",
+       "Wooden Slab",

Nooooo! Whyyyyy? That's not how it works! This translation will not work because …

  1. Adding a variable inside S() will mean that translation tools to generate the template (like findtext.lua) will fail. For that reason, text inside S() should always be constant. Or did you intend to just write the template by hand? :)
  2. You removed the S() for the base names "Wooden Stair" and "Wooden Slab" and all other slabs and stairs. This means these string will also not be collected by tools, and no proper template can be generated …
  3. It's just another string concatenation

breaks stair API compatibility on two occasions:

Sigh. 👎

register_stair_inner, register_stair_outer no longer prepend "Inner ", "Outer " to description (unavoidable)

It's completely avoidable. Just add 2 more optional arguments for explicitly specifying the full inner and outer node description. If missing, fall back to the old behaviour (i.e. prepend "Inner"/"Outer"). That's how I did it in MCL2. And obviously, mods are still strongly adviced to provide ALL arguments, but things won't break when they don't.

register_stair_and_slab uses the "stairs" translator on the given description (could be avoided)

Yes, it could be avoided. Just don't use the S() in the actual stair API anywhere. Instead, mods that actually call the stair register function are supposed to use the S(). That's how I did it in MCL2.

@sfan5

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Ugh. No! [...]

Great job writing such a negative review when you have no idea what you are talking about.

S(desc .. " Butterfly") ends up as S("Red Butterfly") which can be properly translated in any word order (e.g. Red Butterfly=Butterfly but with a different order (red)), try it yourself.

And now, the butterfly color will not even be collected by scripts …
The same complaints apply for the other string concatenations …

I'm aware of this limitation, I explicitly preferred doing this instead of excessively repeating strings everywhere.
The collection could be solved using a dummy file with the correct S() invocations, but I don't know if others would be fine with that.

It's completely avoidable. Just add 2 more optional arguments for explicitly specifying the full inner and outer node description.

That's a good idea, I'll do that.

(like findtext.lua)

By the way, where do I find this tool?

@sfan5 sfan5 force-pushed the sfan5:trans_fix branch from e847812 to 8f2d752 Sep 10, 2019

@paramat paramat added this to the 5.1.0 milestone Sep 10, 2019

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

S(desc .. " Butterfly") ends up as S("Red Butterfly") which can be properly translated in any word order (e.g. Red Butterfly=Butterfly but with a different order (red)), try it yourself.

In theory, yes. In practice, no, because the script won't see it.

The collection could be solved using a dummy file with the correct S() invocations, but I don't know if others would be fine with that.

Well, but then you just wrote the exact string you wanted to avoid … I don't see the benefit.

where do I find

Well, the script is not very good and is not finished and lacks features. And you can find multiple versions of it floating around GitHub. Here's one of the versions:

https://github.com/Ganome/OPSkyBlock/tree/7127da760c85ed56f4a128b744aa3e4d004c64a7/mods/intllib/tools

We really need proper and OFFICIAL tools to collect strings. But that's another issue, unrelated to MTG.

@sfan5

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

Well, but then you just wrote the exact string you wanted to avoid … I don't see the benefit.

True, the actual benefit would be that the code looks cleaner, not that you don't have to type the strings out.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

The script that I used for my client-side translations: #6325, improved: https://notabug.org/pgimeno/minetest/src/translation-toolchain/util/findtext.lua

@SmallJoker
Copy link
Member

left a comment

Did not test, but the code looks good including the translation strings.

@sfan5 sfan5 merged commit e4adb01 into minetest:master Sep 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sfan5 sfan5 deleted the sfan5:trans_fix branch Sep 12, 2019

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

This looks fine now. I am happy with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.