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

Reach the translation glyph node by traverse_id() #1201

Merged
merged 1 commit into from
Aug 22, 2016

Conversation

anthonyfok
Copy link
Contributor

LuaTeX-ja prepends custom whatsit nodes to store e.g. text direction,
breaking hard-coded paths like startnode.head.next.head.next.head
in the center_translation() function.

This patch uses node.travese_id() to find our desired nodes instead.

Fixes #1180

LuaTeX-ja prepends custom whatsit nodes to store e.g. text direction,
breaking hard-coded paths like startnode.head.next.head.next.head
in the center_translation() function.

This patch uses node.travese_id() to find our desired nodes instead.

Fixes gregorio-project#1180
@eroux
Copy link
Contributor

eroux commented Aug 8, 2016

Looks good to me apart from two things:

  • the pull request should be against develop
  • did you run the tests on the gregorio-test repository?

@eroux
Copy link
Contributor

eroux commented Aug 8, 2016

Oh, also an entry should be added in the CHANGELOG.md file

@rpspringuel
Copy link
Contributor

Actually, I think this is a valid candidate for a bug-fix to 4.1. It fixes a broken feature and only affects the files which are uploaded to CTAN, meaning we can push the update to TeXLive 2016 users who update via tlmgr.

It still does need a CHANGELOG.md entry.

@rpspringuel rpspringuel mentioned this pull request Aug 18, 2016
@rpspringuel rpspringuel merged commit 01646ba into gregorio-project:master Aug 22, 2016
@anthonyfok anthonyfok mentioned this pull request Aug 22, 2016
@anthonyfok
Copy link
Contributor Author

@eroux Thank you for your review. I am sorry that I had been very busy and still haven't found the time too follow up, especially in providing test cases in the gregorio-tests repository.

@rpspringuel Thank you so much for merging this pull request. Unfortunately, I encountered a bug Saturday afternoon when I tried to change the Chinese font used in the translation, using \setmainjfont from luatexja-fontspec, lualatex crashed with this error:

! error:  (node lib): lua <node> expected, not an object with type nil

It crashed here:

  local trans_width = node.dimensions(glyphnode)

because glyphnode becomes nil with certain fonts, for reasons that still befuddles me. (I know too little about TeX.) Anyhow, my patch has wrongly assumed that the translation text would be enclosed in the glyph node directly within the hbox, as in vlist { hlist { glyph }}. It turns out that, with some CJK fonts, the glyph node got wrapped into one deeper level, as in vlist { hlist { hlist { glyph } } }.

"Good fonts" that work: "Adobe Ming Std", "Adobe Song Std", "AR PL UKai TW", "AR PL UMing TW", where a dump_nodes() would show this:

Module gregoriotex Info: node hlist [2] {1} on input line 63
Module gregoriotex Info: ..node whatsit [8] {1} on input line 63
Module gregoriotex Info: ..node rule [3] {1} on input line 63
Module gregoriotex Info: ..node glyph [256] {1} on input line 63
Module gregoriotex Info: ..node glue [13] {1} on input line 63
Module gregoriotex Info: ..node rule [3] {1} on input line 63
Module gregoriotex Info: ..node glyph [256] {1} on input line 63
Module gregoriotex Info: ..penalty=10000 {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: ..node hlist [0] {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: --end dump-- on input line 63

"Bad fonts" that would crash: STZhongSong, Fandol, cwTeXKai, HanWangMingBold etc.

Module gregoriotex Info: node hlist [2] {1} on input line 63
Module gregoriotex Info: ..node whatsit [8] {1} on input line 63
Module gregoriotex Info: ..node hlist [0] {1} on input line 63
Module gregoriotex Info: ..node glue [13] {1} on input line 63
Module gregoriotex Info: ..node hlist [0] {1} on input line 63
Module gregoriotex Info: ..penalty=10000 {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: ..node hlist [0] {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: ..node glue [0] {1} on input line 63
Module gregoriotex Info: --end dump-- on input line 63

So, my quick fix is to check for nil right after the local glyphnode = get_first_node_by_id(glyph, hlistnode.head) line:

  if glyphnode == nil then
      glyphnode = get_first_node_by_id(hlist, hlistnode.head)
  end

Though probably it shouldn't be called a glyphnode any more. This, fortunately, fixes the crash. For example, cwTeXKai and Fandol works perfectly after this patch.

However, with STZhongSong and HanWangMingBold, there are the occasional translation text that are not quite centered but somehow shifted to the right. But that is a relatively minor issue that shouldn't hold up 4.1.5.

So, @rpspringuel, would you like me to make a new pull request adding the check for nil? Or would you like to do this yourself (and probably with a better fix)? Please let me know. Many thanks!

(I will be at the computer this morning standing by. I will also try to upload my test files somewhere if you would like to take a look at them.)

Thanks again!

@anthonyfok
Copy link
Contributor Author

Oops, I see that I am too late (4.1.5 is out, yay!) No worries. Maybe for 4.1.6 then. I will make another pull request when I make progress (i.e. when the alignment is correct for all fonts.)

Thank you all!

@rpspringuel
Copy link
Contributor

Please do. Just be sure to submit a new PR against the ctan branch, not master.

@anthonyfok
Copy link
Contributor Author

@rpspringuel Thank you for the note about the ctan branch, otherwise I would have incorrectly submitted the PR against master again. 😉

And thank you to you all, all Gregorio authors, for making typesetting Gregorian chant possible on the computer! Here is Salve Regina with Chinese translation (not the official Chinese translation; it has been modified to try to fit the Latin text order while kind of making sense in Chinese, but hopefully can help our choir and parishioners learn it):

Salve_Regina_中文譯詞.pdf

salve_regina_projector-p1
salve_regina_projector-p2
salve_regina_projector-p3

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.

3 participants