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

Import l10n files from mozilla-aurora [WIP] #3762

Closed
wants to merge 1 commit into from

Conversation

timvandermeij
Copy link
Contributor

This PR fixes #3242. The first commit adds a Makefile utility called importl10n (call: node make importl10n) that fetches the latest l10n files from mozilla-aurora. This implies that translations should now only be done at the central Mozilla repository and not here anymore. This avoids users translating strings here that are already translated by the Firefox team and makes sure that translations are always synchronized between Firefox and PDF.js.

The second commit is quite a huge one actually. It actually adds all new translations available on mozilla-aurora to our repository, thereby synchronizing both translations. It overwrites our files with the ones from mozilla-aurora, but I have kept our metadata.inc files because the Firefox team does not translate them. For all newly imported languages, the import command just copies the English metadata.inc file and changes the language code. This is done such that the extensions for all languages can be properly generated (I think the extension generator requires metadata.inc to be available for each available language, though I could be wrong here).

The third commit fixes a minor linting issue I discovered after submitting this PR. I don't know how to squash it with the first commit, but retain the second one, so I've not done that. If it's really necessary, I'll need to figure out a way to do this.

In the end, this PR gives PDF.js a great amount of new and improved translations and the Makefile command makes sure that synchronizing the translations in the future will be as simple as running the node make importl10n command and committing/pushing the result.

If you have any feedback, please let me know :-)

  • Investigate polling Nightly channel (attemps to do this failed before and it is not a core feature for this PR).
  • Specify locales to download in a separate file.
  • Split into two PRs: one for the Makefile utility and one for the l10n updates.
  • Investigate langcode errors with node make firefox.
  • Check if addon size can be reduced.
  • Remove placeholder metadata.inc file creation.

@Snuffleupagus
Copy link
Collaborator

@timvandermeij As I said before when we talked about this, I think this is really nice!
A few questions/ideas:

  • What happened with my suggestion (in private chat) to import the Nightly translations if they are available, and otherwise fallback to using the Aurora translations?
    Since it takes 6 weeks from the time that changes lands on mozcentral (Nightly) until they are available in Aurora, using Aurora might mean that it takes a lot longer than necessary before new features are translated in PDF.js.
  • Also, would it make sense to specify the list of locales in a separate file, that is read by make.js?
    This way it would be easier to add/remove locales, without having to edit the makefile.

@Snuffleupagus
Copy link
Collaborator

Regarding the squash issue: it might be easier to submit the updated makefile in one PR, and then once that lands submit another PR with the actual l10n updates. Just a thought!

@timvandermeij
Copy link
Contributor Author

@Snuffleupagus Oh, I forgot to mention. I tried to implement your suggestion, but after several hours of struggling with Node.js, it failed to work properly. Because Nightly doesn't have all translations, we would have to check at least two different places. I tried to implement it by first checking the Nightly channel and if that failed, to try Aurora, etc., but because Node.js code works in parallel and not sequential, this was taking too much time and not all files would be downloaded by Node.js. No matter what I did, it would always do one request and then stop... :-(

I like the idea of your second comment and I will definitely try to implement that. Maybe submitting two PRs is even better, indeed.

@Snuffleupagus
Copy link
Collaborator

@timvandermeij I believe that I said something like this previously: if you manage to get the Nightly/Aurora combination working that would be great, but if not don't waste too much time on it. Your solution is still an order of magnitude better than the current translation situation!

@Snuffleupagus
Copy link
Collaborator

I compared the size of the Firefox addon (created by node make firefox) with and without this patch applied.
With this patch, the size of the addon increased by 33 percent, which isn't as bad as I feared.

@timvandermeij There seems to be issues with some of the locales. Running node make firefox, I get the following output (I've truncated the log to focus only on the interesting part):

### Building localization files
Skipping invalid locale: ach
Skipping invalid locale: ast
Skipping invalid locale: csb
Skipping invalid locale: ja-JP-mac
Skipping invalid locale: lij
Skipping invalid locale: mai
Skipping invalid locale: nso
Skipping invalid locale: sah
Skipping invalid locale: son

@Snuffleupagus
Copy link
Collaborator

(I think the extension generator requires metadata.inc to be available for each available language, though I could be wrong here).

It works just fine even if that file isn't available, since in that case the default values in this file will be used instead: https://github.com/mozilla/pdf.js/blob/master/extensions/firefox/install.rdf.

// Defines all languages that have a translation at mozilla-aurora.
// This is used in make.js for the importl10n command.
function getlangcodes() {
return [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: one space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in new commit.

@timvandermeij
Copy link
Contributor Author

Superseded by #3766.

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.

Import language files from mozilla-central
2 participants