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

[api-minor] Re-factor NullL10n and remove the hard-coded l10n strings (PR 17115 follow-up) #17146

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

Please note: These changes only affect the GENERIC build, since NullL10n is only a stub elsewhere (see PR #17135).

After the changes in PR #17115, which modernized and improved l10n-handling, the NullL10n-implementation is no longer a good fallback for the "proper" L10n-classes.
To improve this situation, especially for the standalone viewer-components, this patch makes the following changes:

  • Let the NullL10n-implementation extend an actual L10n-class, which is constant and lazily initialized, to ensure that it works exactly like the "proper" ones.

  • Automatically bundle the "en-US" l10n-strings in the build, via the pre-processor, such that we don't need to remember to manually update them.

  • Ensure that the standalone viewer-components register their DOM-elements for translation, similar to the default viewer, since this will allow future code improvements by using "data-l10n-id"/"data-l10n-args" in most (if not all) parts of the viewer.

  • Remove the NullL10n from the AnnotationLayer, to avoid affecting bundle size too much.
    For third-party users that access the AnnotationLayer, as exposed in the main PDF.js library, they'll now need to manually register it for translation. (However, the standalone viewer-components still works given the point above.)

@Snuffleupagus Snuffleupagus added viewer l10n Localization labels Oct 20, 2023
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/2d103322c62414d/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/2d103322c62414d/output.txt

Total script time: 1.44 mins

Published

…gs (PR 17115 follow-up)

*Please note:* These changes only affect the GENERIC build, since `NullL10n` is only a stub elsewhere (see PR 17135).

After the changes in PR 17115, which modernized and improved l10n-handling, the `NullL10n`-implementation is no longer a good fallback for the "proper" `L10n`-classes.
To improve this situation, especially for the *standalone* viewer-components, this patch makes the following changes:
 - Let the `NullL10n`-implementation extend an actual `L10n`-class, which is constant and lazily initialized, to ensure that it works *exactly* like the "proper" ones.

 - Automatically bundle the "en-US" l10n-strings in the build, via the pre-processor, such that we don't need to remember to manually update them.

 - Ensure that the *standalone* viewer-components register their DOM-elements for translation, similar to the default viewer, since this will allow future code improvements by using "data-l10n-id"/"data-l10n-args" in most (if not all) parts of the viewer.

 - Remove the `NullL10n` from the `AnnotationLayer`, to avoid affecting bundle size too much.
   For third-party users that access the `AnnotationLayer`, as exposed in the main PDF.js library, they'll now need to *manually* register it for translation. (However, the *standalone* viewer-components still works given the point above.)
@mozilla mozilla deleted a comment from moz-tools-bot Oct 20, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 20, 2023
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/7ef2b845d142b14/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/872ff168aff45d2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/7ef2b845d142b14/output.txt

Total script time: 24.80 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 17
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/7ef2b845d142b14/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/872ff168aff45d2/output.txt

Total script time: 36.46 mins

  • Font tests: FAILED
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  errors: 1
  different ref/snapshot: 26

Image differences available at: http://54.193.163.58:8877/872ff168aff45d2/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

This PR should also fix https://github.com/mozilla/pdf.js/security/code-scanning/404, simply by removing the affected code.

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@Snuffleupagus Snuffleupagus merged commit da186d1 into mozilla:master Oct 21, 2023
@Snuffleupagus Snuffleupagus deleted the NullL10n-refactor branch October 21, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l10n Localization viewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants