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

Migration from webL10n to i18next #4459

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

ac-mmi
Copy link
Contributor

@ac-mmi ac-mmi commented Feb 24, 2025

@walterbender I have successfully migrated the internationalization system in Music Blocks from webL10n to i18next. I have tested the implementation, and it is working as expected. The only remaining task is to update the test suites, which are currently tailored for webL10n, to support the new i18next standards.

I made the following changes:

  • In loader.js: Added i18next initialization with i18nextHttpBackend to load translations from JSON files stored in the locales folder. This change allows the application to switch languages dynamically.
  • In Multiple JavaScript Files: Replaced instances of webL10n functionality with i18next's function for translations. This change was made across several files(activity.js,block.js,blocks.js,ActionBlocks.js,BooleanBlocks.js and more...)
  • Created locales Folder: Added a new folder named locales containing converted PO files into JSON format, supporting i18next's structure for managing translations.

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
SaveInterface.test.js
VolumeActions.test.js
musicutils.test.js
ToneActions.test.js
PitchActions.test.js
MeterActions.test.js
rubrics.test.js
turtle-singer.test.js
DrumActions.test.js
languagebox.test.js
DictActions.test.js
themebox.test.js
turtledefs.test.js
macros.test.js

@walterbender
Copy link
Member

I am not sure why you changed the _() function to t(). It is quite an invasive and seemingly unnecessary change.
Is there a standard way to convert the PO files to their JSON representation?
It seems (at least) the es file has some encoding issues.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 24, 2025

@walterbender I switched from _() to t() because it's the standard function used by i18next. If preferred, I can alias _() to t() to minimize changes, make a wrapper function.
For the PO to JSON conversion, I used a custom Python script, ensuring comments were ignored and keys matched the original msgids. If there’s a preferred method, I’m open to adjustments.
I’ll check the encoding issues in the es file.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 24, 2025

@walterbender If you were referring to encoding issues in es.po (if I assumed that correctly), I ran a thorough check on the PO files, including es.po, and didn't find any encoding issues using a custom script that detects unescaped quotes and encoding errors. The only issues found were in other files, like ayc.po, quz.po, and te.po, where unescaped double quotes were present.

@walterbender
Copy link
Member

I think keeping the _() wrapper is a good idea.
I am fine with a simple Python script for conversion of the PO files, but it would be good to test some of the more language-specific PO file features which we deliberately avoided with WebL10n, such as handling plurals and word order.
Please do look into the encoding issue. I only noticed it with ES but it could be elsewhere as well.

@walterbender
Copy link
Member

look t your es.json file and you'll see that it is no longer utf-8. It is some weird encoding.

"Are you sure you want to clear the workspace?": "¿Estás segura de que quieres borrar el espacio de trabajo?",

Should be:

"Are you sure you want to clear the workspace?": "¿Estás segura de que quieres borrar el espacio de trabajo?"",

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
VolumeActions.test.js
SaveInterface.test.js
musicutils.test.js
ToneActions.test.js
PitchActions.test.js
MeterActions.test.js
rubrics.test.js
turtle-singer.test.js
DrumActions.test.js
languagebox.test.js
DictActions.test.js
themebox.test.js
turtledefs.test.js
macros.test.js

@walterbender
Copy link
Member

Not sure what you are doing to fix the encodings, but it is not correct. There should not be any strings with "Â" in them. It is not a glyph used in Spanish.

Copy link

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

synthutils.test.js
VolumeActions.test.js
SaveInterface.test.js
musicutils.test.js
ToneActions.test.js
PitchActions.test.js
MeterActions.test.js
rubrics.test.js
DrumActions.test.js
turtle-singer.test.js
languagebox.test.js
DictActions.test.js
themebox.test.js
turtledefs.test.js
macros.test.js

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 24, 2025

@walterbender Could you please let me know the encoding format used in the PO files? This will help me improve the Python script for conversion and avoid similar issues in the future.

@walterbender
Copy link
Member

Everything should be utf-8

@omsuneri
Copy link
Member

@ac-mmi I saw many tests failures please try to resolve those along with the PR only.

@omsuneri
Copy link
Member

omsuneri commented Feb 24, 2025

@walterbender do we really need this change currently as I don't feel so
as this is part of one of the projects we have in GSOC let's wait till the best ideas we get as it is very early to decide the changes at this moment also @ac-mmi I feel we need a discussion before moving to i18n right now..

@omsuneri
Copy link
Member

@walterbender do we really need this change currently as I don't feel so currently
as this is part of one of the projects we have in GSOC let's wait till the best ideas we get as it is very early to decide the changes at this moment also @ac-mmi I feel we need a discussion before moving to i18n right now..

@walterbender
Copy link
Member

It needs work.

Copy link

✅ All Jest tests passed! This PR is ready to merge.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 24, 2025

@walterbender i have made changes in the _() functionality catering to i18next functions and also passing the test suits of the repo

@walterbender
Copy link
Member

ES is still broken. Now all of the non-ASCII characters are just missing.
We need to discuss a number of other details, including how to handle the Kana/Kanji situation. And language detection in general.

Copy link

✅ All Jest tests passed! This PR is ready to merge.

….js so that renderLanguageSelectIcon() works correctly
Copy link

✅ All Jest tests passed! This PR is ready to merge.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 25, 2025

@walterbender I see that the Kana/Kanji situation is currently handled in the LanguageBox class by splitting them into separate options (ja for Kanji and kana for Kana). Do you more integrated approach, like combining them under a single "Japanese" option with a sub-preference?
Also, for language detection, would you consider automatic detection from the browser settings, or prefer a language selection screen at the start? I feel either approach could enhance the user experience.

@walterbender
Copy link
Member

The default should be from the browser, but the user should be able to override it with the language selector. As for JA, it is a bit complicated. I think we prob. need to keep separate PO files, but if there are better approaches, I am open to suggestions.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 26, 2025

@walterbender The current implementation in activity.js already meets your requirement. It defaults to the browser's language using navigator.language if no language is set, and allows the user to override this choice through the language selector.
it is just the kana/kanji problem thats left right.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Feb 26, 2025

@walterbender For handling the Kana/Kanji situation, I suggest using a single jp file using nested keys avoiding redundancy and keeps everything consistent and easier to maintain. For eg. In a single jp file we can define words like this

 {
    "music": {
        "kanji": "音楽",
        "kana": "おんがく"
    },
    "play": {
        "kanji": "再生",
        "kana": "さいせい"
    }
}

In the UI, we can check the user's Kana/Kanji preference and based on that, it would select the appropriate sub-key (kanji or kana) when updating the text content.

@walterbender
Copy link
Member

What is the standard practice with PO files for "nesting"? I don't think it is prudent to invent a novel solution to a common problem.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Mar 1, 2025

@walterbender in .po file using contextual keys(msgctxt) like given below:

msgctxt "kanji"
msgid "music"
msgstr "音楽"

msgctxt "kana"
msgid "music"
msgstr "おんがく"

Copy link

github-actions bot commented Mar 4, 2025

✅ All Jest tests passed! This PR is ready to merge.

Copy link

github-actions bot commented Mar 7, 2025

✅ All Jest tests passed! This PR is ready to merge.

1 similar comment
Copy link

github-actions bot commented Mar 7, 2025

✅ All Jest tests passed! This PR is ready to merge.

Copy link

github-actions bot commented Mar 7, 2025

✅ All Jest tests passed! This PR is ready to merge.

Copy link

github-actions bot commented Mar 7, 2025

✅ All Jest tests passed! This PR is ready to merge.

1 similar comment
Copy link

github-actions bot commented Mar 7, 2025

✅ All Jest tests passed! This PR is ready to merge.

Copy link

github-actions bot commented Mar 7, 2025

✅ All Jest tests passed! This PR is ready to merge.

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Mar 7, 2025

@walterbender I’ve successfully merged the Kana and Kanji translations into a single JSON file and implemented the necessary logics to handle the scenario correctly. The system now distinguishes between them without requiring separate files.
Let me know if any tweaks are needed.

@walterbender
Copy link
Member

Seems there a many unrelated changes to activity.js
Do we need the "old" JSON files?

I like the way that you've structured the ja JSON file. What changes need to occur in the PO files.

What changes did you make to es.po?

@pikurasa do we want any untranslated KANA to use the KANJI or EN?

@ac-mmi can you please describe the new workflow for translation?

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Mar 8, 2025

@walterbender New Workflow for translation:

1. i18next Language Initialization & Loading
The loader.js file initializes i18next, loads translations from locales/{{lng}}.json, and updates the UI dynamically. The keySeparator: '=)' is used to handle structured keys correctly.

2. Handling Language Preferences in activity.js
If a user has previously selected a language, it is stored in this.storage.languagePreference. For Japanese, if ja is selected, it appends the Kana preference (ja-kana or ja-kanji). This ensures the correct script is used based on user preferences.

3. Language Selection Logic in languagebox.js
When a user selects a language from the dropdown, the preference is stored. For Japanese, ja-kana or ja-kanji is explicitly set. A message prompts the user to refresh the browser to apply changes.

4. Translation Key Lookup with Fallback (customTranslate)
If a key does not exist in the selected language, it falls back to the default English translation. This ensures that missing translations do not break the UI. This fallback mechanism is implemented in the i18next.init function inside loader.js.

5. Updates to _() Function in util/util.js
Changes were made to the _() function to improve translation handling. It now supports Kana/Kanji situations by attaching the correct subarray key (=)kana or =)kanji) when necessary. Additionally, it ensures better fallback mechanisms and text sanitization for improved translation accuracy.

Previously, Kana and Kanji translations were stored in separate JSON files. Now, they have been merged into a single JSON file under locales/ja.json with key separation, which I believe will make maintenance easier.

@walterbender
Copy link
Member

What changes, if any, need to be made to the PS files? Presumable we don't need localization.ini any more?

@ac-mmi
Copy link
Contributor Author

ac-mmi commented Mar 8, 2025

@walterbender No changes are needed in the .po files since I created a Python script to handle the conversion automatically.

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.

3 participants