Addons: Fix fallback languages

[mcleinman]

Description

When coming up to speed on addon translations as part of VPN-5478, I noticed this quirk. I can't find a reason why it would exist (other than an error), so I'm putting up this patch.

locale_file_fallback is the file for a given fallback language's translations (ts file type), built from translations_fallback[locale]. However, we never do anything with locale_file_fallback after creating it - however we again use the translations_fallback[locale] in creating the final translation pack. (Additionally, for languages with multiple fallback languages, we overwrite locale_file_fallback each time, as we use the source language's abbreviation in creating the filename, not the fallback language - so it ends up overwritten if there are 2 fallback languages.) This all seems like an error.

That said, I can't find user-facing impact in my (limited) testing. lconvert is smart enough to be able to use many different file extension types, and it seems to be working as expected - that last line I updated here is combining different file types as it does its work, rather than multiple ts files as (I believe) we expected it to. The one thing we're losing is the -no-untranslated flag in the interim step. And so that makes this PR seem worthwhile, in my opinion.

Reference

None

Checklist

• My code follows the style guidelines for this project
• I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
• I have performed a self review of my own code
• I have commented my code PARTICULARLY in hard to understand areas
• I have added thorough tests where needed

[oskirby]

It's been a while since I looked at the Qt translation tools, but I think the sneaky detail here is that lcovert updates the output file rather than overwrite it, meaning that the for-loop is actually converting from XLIFF and merging all the fallbacks together into one file at the same time. But, it's really sneaky - and I think your approach is a lot cleaner.

That being said, lcovert can take multiple input files, so that entire for loop could probably just be reduced to:

fallback_output = os.path.join(tmp_path, "i18n", f"locale_fallback_{locale}.ts")
fallback_xliffs = [os.path.join(args.i18npath, x, "addons", manifest["id"], "strings.xliff") for x in translations_fallback[locale]]
os.system(f"{lconvert} -if xlf -no-untranslated -o {fallback_output} {' '.join(fallback_xliffs}")

[mcleinman]

It's been a while since I looked at the Qt translation tools, but I think the sneaky detail here is that lcovert updates the output file rather than overwrite it, meaning that the for-loop is actually converting from XLIFF and merging all the fallbacks together into one file at the same time. But, it's really sneaky - and I think your approach is a lot cleaner.

That being said, lcovert can take multiple input files, so that entire for loop could probably just be reduced to:

fallback_output = os.path.join(tmp_path, "i18n", f"locale_fallback_{locale}.ts")
fallback_xliffs = [os.path.join(args.i18npath, x, "addons", manifest["id"], "strings.xliff") for x in translations_fallback[locale]]
os.system(f"{lconvert} -if xlf -no-untranslated -o {fallback_output} {' '.join(fallback_xliffs}")

Even with appending translations, line 426 uses an output file of locale_file_fallback - and then that file is never touched used. So I still think there is something funky here. Or am I missing something?

And while we could compress this for loop a bit, it actually gets even more complex in #9763 - and I think we need to keep that for loop in order to add that shared string stuff.

[flodolo]

I assume the code worked because of how lconvert operates when importing into an existing file? The current code defines 3 variables (locale_file_fallback, locale_file_en, locale_file) which end up pointing to the same file, so we were importing each locale on top of each other.

If that's the case, I assume this would also work?

if locale in translations_fallback:
    # The fallback translations are computed in reverse order.
    # First "en" where we have 100% of translations by default.
    xliff_path_en = os.path.join(args.i18npath, "en", "addons", manifest["id"], "strings.xliff")
    os.system(f"{lconvert} -if xlf -i {xliff_path_en} -o {locale_file}")

    # Then the fallback languages
    for fallback in translations_fallback[locale]:
        xliff_path_fallback = os.path.join(args.i18npath, fallback, "addons", manifest["id"], "strings.xliff")
        os.system(f"{lconvert} -if xlf -i {xliff_path_fallback} -no-untranslated -o {locale_file}")

    # Finally, the current language
    os.system(f"{lconvert} -if xlf -i {xliff_path} -no-untranslated -o {locale_file}")
else:
    os.system(f"{lconvert} -if xlf -i {xliff_path} -o {locale_file}")

Also, does anyone know anything about this comment?

# When 2.15 will be the min-required version, we can remove
# this block and generate TS files with `no-untranslated'
# option. But to be back-compatible, we need to compute the
# language fallback here instead of in the client.

[flodolo]

I don't think this ever worked, since ' '.join(translations_fallback[locale]) is a list of locale codes, not .ts files 🤔

[mcleinman]

Agreed on that.

[mcleinman]

I assume the code worked because of how lconvert operates when importing into an existing file? The current code defines 3 variables (locale_file_fallback, locale_file_en, locale_file) which end up pointing to the same file, so we were importing each locale on top of each other.

If that's the case, I assume this would also work?

if locale in translations_fallback:
    # The fallback translations are computed in reverse order.
    # First "en" where we have 100% of translations by default.
    xliff_path_en = os.path.join(args.i18npath, "en", "addons", manifest["id"], "strings.xliff")
    os.system(f"{lconvert} -if xlf -i {xliff_path_en} -o {locale_file}")

    # Then the fallback languages
    for fallback in translations_fallback[locale]:
        xliff_path_fallback = os.path.join(args.i18npath, fallback, "addons", manifest["id"], "strings.xliff")
        os.system(f"{lconvert} -if xlf -i {xliff_path_fallback} -no-untranslated -o {locale_file}")

    # Finally, the current language
    os.system(f"{lconvert} -if xlf -i {xliff_path} -no-untranslated -o {locale_file}")
else:
    os.system(f"{lconvert} -if xlf -i {xliff_path} -o {locale_file}")

I believe those append, not overwrite - so it still worked (but possibly by accident). I'm very unclear if naming these files the same thing was intentional or accidental.

Also, does anyone know anything about this comment?

# When 2.15 will be the min-required version, we can remove
# this block and generate TS files with `no-untranslated'
# option. But to be back-compatible, we need to compute the
# language fallback here instead of in the client.

I don't know anything about this. My guess is we updated a Qt version in 2.15, and there was a change in Qt that allowed us to do some stuff in the app. That said, we still have older clients out there, so we should probably keep supporting this for now. Even with those older clients being a small amount of our users, I'm not sure there is an advantage in prematurely removing this.

[mcleinman]

I assume the code worked because of how lconvert operates when importing into an existing file? The current code defines 3 variables (locale_file_fallback, locale_file_en, locale_file) which end up pointing to the same file, so we were importing each locale on top of each other.
If that's the case, I assume this would also work?

if locale in translations_fallback:
    # The fallback translations are computed in reverse order.
    # First "en" where we have 100% of translations by default.
    xliff_path_en = os.path.join(args.i18npath, "en", "addons", manifest["id"], "strings.xliff")
    os.system(f"{lconvert} -if xlf -i {xliff_path_en} -o {locale_file}")

    # Then the fallback languages
    for fallback in translations_fallback[locale]:
        xliff_path_fallback = os.path.join(args.i18npath, fallback, "addons", manifest["id"], "strings.xliff")
        os.system(f"{lconvert} -if xlf -i {xliff_path_fallback} -no-untranslated -o {locale_file}")

    # Finally, the current language
    os.system(f"{lconvert} -if xlf -i {xliff_path} -no-untranslated -o {locale_file}")
else:
    os.system(f"{lconvert} -if xlf -i {xliff_path} -o {locale_file}")

I believe those append, not overwrite - so it still worked (but possibly by accident). I'm very unclear if naming these files the same thing was intentional or accidental.

I've updated this so they all have unique names. While I agree that your code should work, I think it's a bit more straightforward to draw this out (as it currently is) - especially because this is rarely-touched code, and every year or two we have someone jump in and they generally have to re-learn how this all works. (That could be also accomplished with updated comments.) Additionally (and importantly), I'm not quite sure how the ordering works if we append - and it seems like that is very important to get right.