retro-go icon indicating copy to clipboard operation
retro-go copied to clipboard

Adding Localization support

Open rapha-tech opened this issue 1 year ago • 3 comments

Hey, So I created a new PR and did everything from scratch so that it's clearer. I decided to go for the gettext() method, here is how I implemented it for now :

  • originals messages + translations are hold in a struct :
- typedef struct {
    char *msg;    // Original message in english
    char *fr;    // FR Translated message
} Translation;

Translation translations[] = {
    {"Folder is empty.", "Le dossier est vide."},
    {"yes", "Oui"},
    {"No ", "Non"},
    {"OK", "OK"},
    {NULL, NULL}  // End of array
};
  • We have a very simple linear search lookup function that uses strcmp() function
const char* rg_gettext(const char *text) {
    if (rg_language == 0)
        return text; // in case language == 0 == english -> return the original string

    for (int i = 0; translations[i].msg != NULL; i++) {
        if (strcmp(translations[i].msg, text) == 0) {
            switch (rg_language)
            {
            case 1:
                return translations[i].fr;  // Return the french string
                break;
            }
        }
    }
    return text; // in case no translation exists
}
  • "Language" setting have been adding in the Global settings menu below "timezone"

Now I have a question : should we make a Translation variable for each language or add another field to the struct like char *de; // DE Translated message for example? The first method uses less memory but the second is a bit prettier

rapha-tech avatar Oct 12 '24 13:10 rapha-tech

Also I am thinking about creating some python programs to "dump" all the _(string) in one file so that translation is easier and then be able to convert it back in "c" form

rapha-tech avatar Oct 12 '24 13:10 rapha-tech

should we make a Translation variable for each language or add another field to the struct like char *de; // DE Translated message for example?

My intuition would have been to have one language per struct, then you just have a pointer to the correct language array. But I can see your point. Having all languages in the same struct does have some benefits too. The "key" isn't repeated in every file so it's less prone to mistakes.

It's also nice to see all translations in one file side-by-side.

{"Folder is empty.", "Le dossier est vide.", "<whatever that is in german>"},

Downsides would be that if we have more than 2-3 languages it will become difficult to maintain side-by-side. For longer messages it will be pretty hard to keep track on the line. It can't be split in multiple files.

Though a change of syntax would fix the most downsides, splitting the definition on multiple lines makes things very clear regardless of the number of languages or message length:

{
    .msg = "Folder is empty blah blah blah blah blah blah blah blah .",
    .fr = "Le dossier est vide. blah blah blah blah blah blah blah blah blah blah ", 
    .de = "<whatever that is in german>blah blah blah blah blah blah blah blah>",
},

So I think I'm fine either way, one language per struct or all languages in one struct. Do what you think is best, just make sure it's easy to edit/understand for future translators!

ducalex avatar Oct 13 '24 12:10 ducalex

Also I am thinking about creating some python programs to "dump" all the _(string) in one file so that translation is easier and then be able to convert it back in "c" form

I agree a tool to locate all localized strings (and possibly tell us missing translations) would be great!

ducalex avatar Oct 13 '24 12:10 ducalex

Hey, just a ping to say I have not forgotten/ignored this PR.

I'll be back soon (hopefully) to fully review the the work and continue towards merging it ASAP.

ducalex avatar Oct 23 '24 17:10 ducalex

No problem take your time ;)

rapha-tech avatar Oct 24 '24 12:10 rapha-tech

So I've reviewed the PR and it's looking pretty good!

Some points:

  1. Some strings have trailing spaces. This is an artifact from when our dialog system didn't support alignment, but now it does. You can just strip \s+$ from all strings.
  2. I don't like translations in the libs folder, that's mostly for third party stuff. It should be moved back to retro-go/rg_localization.[ch] or retro-go/rg_i18n.[ch] or something along those lines like you did at first. I don't know what's the most common naming in other projects.
  3. I think the strings should be separate from the code. Moved to translations.h or strings.h or something along those lines, and include it in the localization .c. But if you have a good argument to keep them together I'm happy to hear it, though!
  4. You should run clang-format on your code (maybe except the strings themselves). You can also just try to match the indentation style found in the other rg_ files manually, I don't need perfection just some consistency.
  5. Add an enum for the language ids, eg RG_LANG_EN RG_LANG_FR

ducalex avatar Oct 29 '24 16:10 ducalex

Thanks for the changes, I don't see any obvious problem remaining :)

I should have time to properly test the patch this weekend, after that it can be merged.

ducalex avatar Nov 06 '24 18:11 ducalex

Everything seems to work as intended, nice work!

I've made some changes to the PR (simplifications and catching some edge cases). Can you review and see if there's anything you disagree with? In such case you can revert or we can discuss it here.

Also the lack of diacritics in the fonts is a bit annoying (not your fault). I'll do some tests in adding support for latin-1 I think...

Thanks!

ducalex avatar Nov 10 '24 22:11 ducalex

Thanks ! Alright so after some testing everything seems fine to me too ! Your changes on the code are really nice. Only thing I'm thinking is adding the link to the localization.md in the main Readme but I will let you do it when dev will be merged with main.

Latin-1 would be great ! If you want me to I can try to take a look at it

rapha-tech avatar Nov 13 '24 16:11 rapha-tech

Latin-1 would be great ! If you want me to I can try to take a look at it

If you want there is some explanation of how it works for proportional fonts in fonts/fonts.h. But it's been so long I forgot the details, I remember it was a bit hit-and-miss to generate a good looking font and I think the tooling was windows only. Might be worth making our own font generator in python...

The Basic font works a bit differently, we just need to add the missing glyphs from the latin file here: https://github.com/dhepper/font8x8 .

But we don't need to wait for the better fonts to merge the PR, I'll do a final read and merge it in a few days :)

ducalex avatar Nov 14 '24 20:11 ducalex

Yeah sounds great !

Actually I'm almost done adding Latin-1 support, should I wait for you to merge before starting some commits ?

rapha-tech avatar Nov 15 '24 12:11 rapha-tech

Here are somes previews : 1731685458568 1731685458580

I had to increase the height of the fonts since the overall height of the font was increased because of capital letters with diacritics ex : É. So I was wondering if we needed them ?

Also I tried to add OpenSans16, I really like the numbers in this one. Let me know what you think ! 1731686219319

rapha-tech avatar Nov 15 '24 15:11 rapha-tech

Those all look good indeed!

Since I'll have to test on the various devices I have for any font change and change of size can introduces subtle bugs, I'd prefer it be another PR. You can get started right away on your font PR, I don't think it depends on this one to be merged (though it will merged as soon as I'm at my desk today anyway)

As for ÉÒÙÀ I'll leave it up to you if you want to include them. I agree that we don't have many places where we'd need them.

Maybe I can update the rendering code to fallback to the corresponding bare letter when a diacritics glyph is missing, so that translations can at least use proper spelling even if not shown.

ducalex avatar Nov 16 '24 18:11 ducalex