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] Change the format of the fontName-property, in defaultAppearanceData, on Annotation-instances (PR 12831 follow-up) #13169

Conversation

Snuffleupagus
Copy link
Collaborator

Currently the fontName-property contains an actual /Name-instance, which is a problem given that its fallback value is an empty string; see

fontName: Name.get(""),

The reason that this is a problem can be seen in
Name.get = function Name_get(name) {
const nameValue = nameCache[name];
// eslint-disable-next-line no-restricted-syntax
return nameValue ? nameValue : (nameCache[name] = new Name(name));
};
, since an empty string short-circuits the cache. Essentially, in PDF documents, a /Name-instance cannot be empty and the way that the DefaultAppearanceEvaluator does things is unfortunately not entirely correct.

Hence the fontName-property is changed to instead contain a string, rather than a /Name-instance, which simplifies the code overall.

Please note: I'm tagging this patch with "[api-minor]", since PR #12831 is included in the current pre-release (although we're not using the fontName-property in the display-layer).

…AppearanceData`, on Annotation-instances (PR 12831 follow-up)

Currently the `fontName`-property contains an actual /Name-instance, which is a problem given that its fallback value is an empty string; see https://github.com/mozilla/pdf.js/blob/ca7f546828603d15ac0975f6131669321bfccceb/src/core/default_appearance.js#L35
The reason that this is a problem can be seen in https://github.com/mozilla/pdf.js/blob/ca7f546828603d15ac0975f6131669321bfccceb/src/core/primitives.js#L30-L34, since an empty string short-circuits the cache. Essentially, in PDF documents, a /Name-instance cannot be empty and the way that the `DefaultAppearanceEvaluator` does things is unfortunately not entirely correct.

Hence the `fontName`-property is changed to instead contain a string, rather than a /Name-instance, which simplifies the code overall.

*Please note:* I'm tagging this patch with "[api-minor]", since PR 12831 is included in the current pre-release (although we're not using the `fontName`-property in the display-layer).
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/ed3e8249e78d5d3/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/59c2f3a8c89eac8/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/ed3e8249e78d5d3/output.txt

Total script time: 24.96 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/ed3e8249e78d5d3/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/59c2f3a8c89eac8/output.txt

Total script time: 29.75 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://3.101.106.178:8877/59c2f3a8c89eac8/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/03bf87aef8d645a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/9ccf795ba7b10c1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/03bf87aef8d645a/output.txt

Total script time: 3.99 mins

  • Integration Tests: FAILED

@pdfjsbot
Copy link

pdfjsbot commented Apr 1, 2021

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/9ccf795ba7b10c1/output.txt

Total script time: 5.07 mins

  • Integration Tests: Passed

@timvandermeij timvandermeij merged commit 5cf116a into mozilla:master Apr 2, 2021
@timvandermeij
Copy link
Contributor

Thanks!

@Snuffleupagus Snuffleupagus deleted the DefaultAppearanceEvaluator-fontName branch April 2, 2021 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants