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

[skip ci] Create i18n API #446

Merged
merged 43 commits into from
Apr 5, 2023
Merged

[skip ci] Create i18n API #446

merged 43 commits into from
Apr 5, 2023

Conversation

meadowsys
Copy link
Member

@meadowsys meadowsys commented Mar 27, 2023

This PR creates an i18n API in the core of the editor, accessible via atom.i18n.

Core translations are loaded from i18n dir in the root of the project, and package translations (including core packages) are loaded in the i18n dir in the root of the package, similar to how themes/grammars work (or so confused-techie says). They are automatically namespaced by package name / core for core strings (eg. core.welcome, or myPackage.welcome).

Unsure if we should extract out all the strings in this PR, or if we should merge this PR with only the API and create a seperate PR (or PRs) to extract out strings. Decided on discord, to let this PR be merged as it currently is (of course going through the review process) and create a new PR to extract out the rest of the strings.

Crowdin integration has yet to be set up too, but I'm pretty sure this would have to be merged to master first, before that can be done. Would also need someone with admin perms in the org to set up the integration, I think.

API details
  • atom.i18n.t(key, opts): basic translation function. Key is eg. core.welcome, and opts is an object of variables to interpolate in the string.
  • atom.i18n.getT(namespace): gets a t function, similar use as atom.i18n.t, for keys in a specific namespace. Ex:
    let t = atom.i18n.getT("core");
    let welcome = t("welcome"); // core.welcome
  • atom.i18n.registerStrings(packageId, strings): Programmatically register strings under the package namespace.
  • In menu items, in addition to a regular label key in a menu item, you can now have a localisedLabel key. This can either point to a key, or can point to an object, with properties key and opts for values to interpolate. (note: this works by replacing label by passing the data in localisedLabel through the translation API. You can have both, but label will be overwritten)
  • The second parameter in a package's activate function is now an object (to let it be extensible with more things in the future, Futureproofing™) with a t property that is a t function scoped to the package.

There are a few TODO comments still in the file for "unessential", but likely important features (eg. for performance), like caching the parsed string ASTs (also persistently to disk) and saving IntlMessageFormat instances.

@meadowsys meadowsys changed the title Create i18n API [skip ci] Create i18n API Mar 27, 2023
@meadowsys
Copy link
Member Author

there are a few things like menus and the display text of configuration settings that likely need a good amount of reworking to support translating

@meadowsys meadowsys mentioned this pull request Mar 29, 2023
In addition to regular `label` key in a menu item, you can now have a `localisedLabel` key. This can either point to a key, or can point to an object, with properties `key` and `opts` for values to interpolate.
sorry this was bothering me lol
loading is done at startup if present, saving not yet implemented
@meadowsys
Copy link
Member Author

The implementation of the API should be done now, just need to get all strings extracted now

Move them all to the dynamic place, so that its consistently in one place, and remove the last 2 settings that aren't currently in use
Provided as second argument, so, `activate(_, { t })`
@meadowsys meadowsys marked this pull request as ready for review April 2, 2023 02:25
@meadowsys
Copy link
Member Author

As decided on discord, lets just get this one merged, then extract the rest of the strings in another PR

@meadowsys meadowsys marked this pull request as draft April 2, 2023 03:00
@meadowsys meadowsys marked this pull request as ready for review April 2, 2023 04:05
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Alright, so I've taken a pretty close look at the code in a vacuum. (As in not fully testing everything, or properly looking at it collectively)

But right off the bat, this looks super super solid! Seriously impressive work, just did add a few comments or questions throughout, mostly related to trying to make it a bit more robust, or easier to use.

I did give it some minimal testing as in insuring that it does run locally and all strings seem to be where they should be, and that the API is available as the public atom.i18n method.

But yeah otherwise I think there does need to be some benchmarking here, or at least making sure what's here isn't to slow. Since launching locally did take long enough to get a warning the application was unresponsive, but did only occur once, (so could be Windows weirdness, or could be that after everything was cached the issue went away).

So I think in a perfect world we could try to make this less synchronous, but not sure that's possible within the scope of this PR.

But hope I didn't leave to many comments, and if you think any of my suggestions can be addressed in this PR that's awesome, otherwise I'll go ahead and try to review this more as a whole, and see if I can't do some light testing on speeds

src/i18n.js Show resolved Hide resolved
src/i18n.js Show resolved Hide resolved
src/i18n.js Outdated Show resolved Hide resolved
src/i18n.js Outdated Show resolved Hide resolved
src/i18n.js Show resolved Hide resolved
src/i18n-helpers.js Show resolved Hide resolved
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Also ended up fleshing out one of my suggestions a bit more so you can see what I mean.

src/i18n.js Show resolved Hide resolved
@meadowsys
Copy link
Member Author

meadowsys commented Apr 3, 2023

I think there does need to be some benchmarking here

I did check the memory in the dev tools (took a heap snapshot), and if I'm reading it correctly (looking at "Retained Size"), currently I18n is holding onto ~153KB of stuff (which would include I18nCacheHelper, ~5KB of stuff, (edit: as well as a few other pieces of the global API, like config, notificationManager (which actually isn't needed anymore lol), packages)). I'm not sure how this would balloon, but from these numbers I don't think there would be any concern here?

we could try to make this less synchronous

Yeah, in an ideal world :p I think t should stay sync, otherwise it would have to convert a lot of sync code (functions) into async. Not sure if it will take a performance hit because functions will have to be converted to async? Also wouldn't be sure how to handle the situation where t is called and its still asyncronously loading strings or something (since you can't make sync code wait on async code)

@confused-Techie
Copy link
Member

@meadowsys The benchmarking doesn't sound to bad then. Not much memory usage, although am curious about return times.

But yeah otherwise, the less sync suggestion, like I said I don't think it could really be addressed in this PR. As that's something present throughout the entire codebase. So not much that we can really do there tbh. But this sounds awesome then!

meadowsys and others added 2 commits April 2, 2023 23:20
Co-authored-by: confused-Techie <dev@lhbasics.com>
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

While I feel like theres many important optimizations to make, and finalizations, this is at a place I'm comfortable starting the deployment of it.

The code is solid, and methodology is solid.

I do think once we have some strings it'll be very important to test how long it takes to get these strings, and what kind of storage usage we are talking about with our caches.

But overall I say lets get this to a place where we can investigate those both properly, thanks a ton for the awesome work here @meadowsys

@meadowsys
Copy link
Member Author

thank you for the help too! and yeah, definitely will want to take a look at resource consumption down the line.

@meadowsys meadowsys merged commit 7590a6b into master Apr 5, 2023
@meadowsys meadowsys deleted the i18n branch April 5, 2023 04:58
@meadowsys meadowsys restored the i18n branch April 6, 2023 20:05
@meadowsys meadowsys mentioned this pull request Sep 12, 2023
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.

2 participants