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

Implement gettext translation file generation #2356

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jrb0001
Copy link
Contributor

@jrb0001 jrb0001 commented Aug 2, 2024

This PR allows selecting gettext as an alternative to the CSV format in the translation settings.

It also fixes the following issues:

  • Glossary can show wrong entry if the name of one entry is used as a translation of another entry.
  • "Collect translations" added non-dialogic translations in various situations.
  • "Collect translations" didn't remove translations that did no longer exist on the filesystem.
  • "Remove translations" didn't delete timeline translations created with the PER_TIMELINE + NEXT_TO_TIMELINE combination.
  • All characters used the same translation keys.
  • Order of characters/glossaries/timelines within a file (Per Type mode) was unstable.
    • This is solved by sorting based on source path. Or would you prefer sorting by translation id or something else?

Things I noticed while testing:

  • Choice nodes have a disabled_text translation, even if the choice has no condition. Wouldn't it be better to only add it if it is actually used?
  • The "Default locale" and "Testing locale" dropdowns only allow languages, not locales. It makes sense not to show every single locale by default, but why is it filtering the locales collected from translation files too?

@CakeVR
Copy link
Collaborator

CakeVR commented Aug 4, 2024

Thank you for your PR.
I will review this in the following days.

I want to quickly talk about the Glossary. I have not looked at the code, so this is only a quick exchange of information.

The Glossary, unlike Timeline Text Events, does not fall back and that is intended by design. If there is no translation for the target locale, they won't do anything. This ensures no wrong Glossary is displayed between languages.

Let me know if this is what you encountered.

@jrb0001
Copy link
Contributor Author

jrb0001 commented Aug 4, 2024

Not sure what I did different this time, it works on the old version.

Available translations of the timelines: en, it
Available translations of the glossaries: en, de

Testing locale: en

  • Old: timeline: en, glossary: en
  • New: timeline: en, glossary: en

Testing locale: it

  • Old: timeline: it, glossary: en
  • New: timeline: it, glossary: en

Testing locale: de

  • Old: timeline: en, glossary: de
  • New: timeline: en, glossary: de

Testing locale: fr

  • Old: timeline: en, glossary: en
  • New: timeline: en, glossary: en

So the behavior is still the same and it always used the fallback logic of godot.

There is still one issue in the old version and fixed in this PR:

  • Use the original locale name of one glossary entry in a translation of another glossary entry.
  • Run the game with the original locale.
  • The text box highlights the correct words.
  • The tooltip shows the wrong entry.

@jrb0001
Copy link
Contributor Author

jrb0001 commented Aug 4, 2024

Bypassing the fallback logic of godot completely would be possible, but the new code would still need some sort of fallback. Otherwise the game would need to have the glossary in every single language+country+variant+... combination a player could have.

I am not sure if the large complexity of building our own fallback logic is really worth it. The only situation where it makes a difference is if timelines, but not glossary, are translated for a specific locale. And it would also be extremely difficult to test it because locales have so many corner cases.

@Jowan-Spooner Jowan-Spooner added the Needs testing More feedback/testing would be good label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs testing More feedback/testing would be good
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants