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 : i18n #94

Merged
merged 12 commits into from
Jun 14, 2024
Merged

fix : i18n #94

merged 12 commits into from
Jun 14, 2024

Conversation

kabir-afk
Copy link
Collaborator

@kabir-afk kabir-afk commented Jun 14, 2024

Changes Made

June 13

  • refactor : storeParams now working properly - Used array destructuring to fetch the arguments properly. With this storeParam message now works properly and invokes the storeParam function inside sw.js and background-script.js .
    I think this addresses Popup>Settings with persistent memory on all browsers #87 . Although Selected language in settings resets when changing pages #56 still persists.
  • chore: passing message to execute i18n natively inside sw/bg.js - This is the highlight of this PR. I figured it was better to pass messages from popup/other content scripts to execute banana.i18n directly inside sw.js/background-script.js and then send the response acccordingly.
    Pros and cons of what we were doing earlier vs now :
    • EARLIER [EDIT: Chrome native's browser.i18n API]

      • Pros :
        • It worked as expected and wasn't too asynchronous in nature, relied on local storage in the name of asynchronous nature.
      • Cons :
        • Verbosity changed depending on the context of usage : For example - SignItVideosGallery and SignItCoreContent. In both the files different implementations of the same functions exist.
        • Repetitve in nature : If some other content script or constructor function popped , we'd have to copy/paste it again in order to make it work.
        • Buggy UI : While changing UI language , Popup had to be toggled off/on in order to see the changes and similar had to be done for the modal. This might have been due to SignItCoreContent constructor not initializing timely , rather than a local storage issue. Although I am not sure why this occured as upon being passed with chosen arguements (user's preferred language ), it was logged correctly but rendered previous language.
    • NOW [EDIT: Wikimedia banana.i18n sent up and processed in sw.js/background.js]

      • Pros :
        • Authenticity: leverages the original library , hence maintaining the authenticity.
        • Message passing: only needs a message to execute.
        • Little to no verbosity: Unlike our own i18n , this works the same in all the contexts and we dont have to redefine it as per our preference every time.
      • Cons :
        • Requires an async environment: As we are passing messages , we are returned with a promise which needs to be resolved whenever the function is called. This can become problematic when working with constructors. This is dealt with in fix : implemented the same message passing functionality as in popup by using an async prototype function named init, although not the best workaround.
        • Irregualar html parsing: While the library banana.i18n addresses its own , if not added as html inside the script, will render the string as is.

With this #84 can be closed now.

June 14

@kabir-afk kabir-afk changed the title ongoing-fix : i18n fix : i18n Jun 14, 2024
@kabir-afk
Copy link
Collaborator Author

NOTE: Just in case you get HTTP Status Code 502 : Method not allowed. comment out the
signLanguages = await getSignLanguagesWithVideos(); and records = await getAllRecords( signLanguage ); line in both sw.js and background-script.js. I haven't thoroughly tested the changes in FF, so please let me know if there are some discrepancies.

@hugolpz hugolpz self-assigned this Jun 14, 2024
@hugolpz hugolpz added this to the GSoC 2024 milestone Jun 14, 2024
@hugolpz
Copy link
Member

hugolpz commented Jun 14, 2024

This is the highlight of this PR. I figured it was better to pass messages from popup/other content scripts to execute banana.i18n directly inside sw.js/background-script.js and then send the response acccordingly.

Thank @kabir-afk for figuring that out !

// let hlwa = browser.i18n.getMessage("si-panel-videos-title");
// console.log("hlwa = ",hlwa);
var SignItCoreContent = function () {
console.log("SignItCoreContent.js");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we logging on console ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is legacy, for when i inspected, to know to which file the following logs were related to. Ca be removed if needed

@hugolpz hugolpz merged commit efd4cc6 into lingua-libre:master Jun 14, 2024
@kabir-afk kabir-afk deleted the fix/i18n branch June 17, 2024 09:17
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