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

Move symbolic font glyphs to private use area if they don't have unicode mappings (issue 2594, bug 789074, bug 865644) #7705

Merged
merged 2 commits into from
Dec 10, 2016

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Oct 9, 2016

This is a tentative patch, which builds upon PR #7482 (that commit is included here with the correct author), that attempts to address issue #2594, bug 789074, bug 865644.

According to #2594 (comment) and #2594 (comment) it seems like this kind of simple hack might actually work, and I'm opening the PR to be able to run tests on (especially) the Linux bot.

@timvandermeij
Copy link
Contributor

I checked most of the test failures (not all of them), but those looked like improvements to me. It has been reported that this patch fixes the original issue.

/cc @yurydelendik or @brendandahl for review

@brendandahl
Copy link
Contributor

It looks like we're moving a lot of glyphs to the private use area, even ones that probably shouldn't be.
Some random pdf's I looked at:
issue5501.pdf
before patch

fonts.js:920 originalCharCode=65 (A)
fonts.js:931 unicode=         65 (A)
fonts.js:920 originalCharCode=69 (E)
fonts.js:931 unicode=         69 (E)

after patch

fonts.js:920 originalCharCode=65 (A)
fonts.js:932 unicode=         65 (A)
fonts.js:958 remapping to=    57380 ()
fonts.js:920 originalCharCode=69 (E)
fonts.js:932 unicode=         69 (E)
fonts.js:958 remapping to=    57381 ()

artofwar.pdf

originalCharCode=65 (A)
fonts.js:933 unicode=         65 (A)
fonts.js:921 originalCharCode=66 (B)
fonts.js:933 unicode=         66 (B)

after

originalCharCode=65 (A)
fonts.js:933 unicode=         65 (A)
fonts.js:959 remapping to=    57359 ()
fonts.js:921 originalCharCode=66 (B)
fonts.js:933 unicode=         66 (B)
fonts.js:959 remapping to=    57360 ()

Also, see my comment in #2594 (comment)

@Snuffleupagus
Copy link
Collaborator Author

/botio test

2 similar comments
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@yurydelendik
Copy link
Contributor

my 2¢: we don't have complete charCode->glyphNames mapping (based on encoding and differences). I'm thinking we shall address this first and after that adjustMapping will use glyphNames/unicode to determine if char code is at right location and will build FreeType2 autofit-friendly toFontChar?

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Oct 27, 2016

The latest version of this PR is in all likelihood my last attempt at fixing #2594, since it's an issue that I'm personally not affected by.

Here, besides the original patch by @brendandahl which already fixes the majority of the problem, I'm also amending src/core/glyphlist.js with the (common) names used in AMSmath Type1 (La)TeX fonts.
This is based on lists of TeX glyph names, and reasonable Unicode values, that I found while searching the web. Hopefully this should be enough to avoid mapping these math symbols to regular characters, thus preventing FreeType from using the wrong glyphs when computing hint values, and should also improve text-selection at the same time.

If this is a reasonable approach, we probably need a couple of Linux users (currently affected by the issue) to test this to ensure that it's in fact a sufficient solution.

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/c75a6e95f23893f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/b5bba0611348d95/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/b5bba0611348d95/output.txt

Total script time: 25.65 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/c75a6e95f23893f/output.txt

Total script time: 39.57 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/768d76e3834ac61/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/768d76e3834ac61/output.txt

Total script time: 2.99 mins

Published

briangough pushed a commit to overleaf/web that referenced this pull request Nov 16, 2016
@Snuffleupagus Snuffleupagus changed the title Move symbolic font glyphs to private use area if they don't have unicode mappings (issue 2594) Move symbolic font glyphs to private use area if they don't have unicode mappings (issue 2594, bug 865644) Nov 22, 2016
@Snuffleupagus Snuffleupagus changed the title Move symbolic font glyphs to private use area if they don't have unicode mappings (issue 2594, bug 865644) Move symbolic font glyphs to private use area if they don't have unicode mappings (issue 2594, bug 789074, bug 865644) Nov 22, 2016
@brendandahl
Copy link
Contributor

r+ pending confirmation of fix

@timvandermeij
Copy link
Contributor

The fix has been confirmed, so I'll take care of landing this.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/1ccea58dc194a13/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/1ccea58dc194a13/output.txt

Total script time: 2.14 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/cf88fd2fe81c120/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/c9019a03bb8c0da/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://107.22.172.223:8877/c9019a03bb8c0da/output.txt

Total script time: 26.13 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/cf88fd2fe81c120/output.txt

Total script time: 27.46 mins

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

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

@timvandermeij
Copy link
Contributor

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/d57a8875608d51b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/efe11a00014626c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/efe11a00014626c/output.txt

Total script time: 25.85 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/d57a8875608d51b/output.txt

Total script time: 26.58 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@timvandermeij timvandermeij merged commit 00a006e into mozilla:master Dec 10, 2016
@timvandermeij
Copy link
Contributor

Thank you for the patch!

@Snuffleupagus Snuffleupagus deleted the issue-2594 branch December 10, 2016 21:28
movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Move symbolic font glyphs to private use area if they don't have unicode mappings (issue 2594, bug 789074, bug 865644)
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.

5 participants