Resolve Issues in packaged Pulsar Localization #1337
Conversation
| // We will copy the methodology that `./src/main-process/atom-window.js` uses | ||
| // to determine our 'resourcePath' | ||
| console.log("I18n: preload started"); | ||
| this.localeFallbackList = [ "en" ]; // Hardcode our default fallback value |
There was a problem hiding this comment.
Since we have access to atom.config, could we simply preload the locale that the user has configured, if any?
That implies that switching locales would require a window reload or Pulsar relaunch, but I don't think that's an awful idea anyway.
There was a problem hiding this comment.
(And if that's jumping the gun, that's fine — also OK with just preloading a hard-coded 'en' at this early stage.)
There was a problem hiding this comment.
Yeah a full relaunch was my original intention anyway when switching locales. I think this iteration of the code was before I realized we would have access to the config API.
I'll try that locally when I can and ensure everything works. If it does I'll switch to using that
There was a problem hiding this comment.
So actually double checking this, unfortunately we still need to manually set a locale during the preload stage. while we do have the config object local to I18n, it hasn't been initialized yet, so won't have any user preferences. It's only initialized in a call of atom-environment.initialize() and that's when it loads the user config, which we can then pick up in our I18n.initialize() call. But during preload we will still need to load all locales and prune them afterwards unfortunately.
There was a problem hiding this comment.
I don't know that I've got the bandwidth to give this proper review — meaning checking it out and ensuring nothing explodes — but I'm happy to take your word for that once you confirm that a downloaded binary works as you intend.
Meanwhile, I left one comment. I think it's a good idea to load as little as possible by default (so that i18n has no impact on users that aren't using it). For similar reasons, I'm skeptical about ever preloading an arbitrary number of locales just in case the user might switch to them.
src/i18n.js
Outdated
| // During preload we load all available locales, and MUST prune them later | ||
| this.addStrings(CSON.readFileSync(localePath) || {}, locale); | ||
| } | ||
| console.log("I18n: preload complete"); |
There was a problem hiding this comment.
I'm also slightly curious about how long it takes to load locales; it doesn't need to be part of this PR, but eventually some timecop benchmarks should live here.
There was a problem hiding this comment.
And maybe the console.logs can be commented out before this lands?
There was a problem hiding this comment.
I was actually debating about integrating this into timecop or if a new package is better. So glad we are on the same page there, because really time to boot, and increased memory usage are two primary concerns for implementing this new API. So I may work on something else that gives us timecop capabilities
|
Thanks for the approval, and don't worry I totally agree that we don't want to have more ever loaded than needed. Even in the event that we have to load everything just because we can't check the config, I'd want to longterm implement a way to prune unneeded strings from memory. But yeah we will see |
|
@savetheclocktower I've gone ahead and removed the extra If that all sounds good to you, then I'll go ahead and merge this and start working again on package localization, as well as looking at ways to integrate with timecop. |
|
Yup, let's do it. |
The fixes from this PR are pulled directly from #1210 but moved into this PR to ensure it's an easier review and can get just the fixes in, without the features.
After merging our new localization API to main, I discovered that in built versions of Pulsar (not dev versions) that bundled packages didn't have localized menus, nor did Pulsar's own menus have translations available. As well as submenus were not translated.
This PR does two things, the easy thing was recursively translating submenus in the menu bar.
The hard thing, was to ensure that Pulsar's bundled packages were translated, as well as Pulsar's own menus and other 'first-up' strings.
For bundled packages we had to add the locales data to their metadata during the build process, so we could access that before the ASAR was readable. This is the same exact way we already read keymaps and menu data for packages, so it made sense to expand that to locales. We also had to slightly move when locales are loaded during the preload process, to ensure it happened before menu/context menus are read.
As for Pulsar, we had to make sure the I18n API was one of the first ones up out of everything. Since some strings for Pulsar are read during the constructor phase, which is when the rest of "the world" comes up. So that meant we had to setup I18n first and move it's spot during the
atom-environmentconstructor.With these few changes, it ensures that just about everything in Pulsar should be able to access the I18n API, and translate at all times. The only outliers will be any items that already don't have access to the
atomglobal, or the few pieces of code that execute outside of that, such as handlers for setting up the window or world.