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

Copy'n'paste approach for issue #1153. #1154

Merged
merged 2 commits into from
Jun 23, 2016

Conversation

jakubjelinek
Copy link
Contributor

I've actually tested only 4.1 backport of this change so far.
The commit also includes one pasto change -
\gre@space@count@additionaltopspacethreshold=2
...
\gre@space@count@additionaltopspacethreshold=0
where the latter IMHO should have been
\gre@space@count@additionaltopspacealtthreshold=0

I've also noticed the Copyright years weren't updated this year, shall that be done before 4.2 is released?

@jakubjelinek
Copy link
Contributor Author

Thinking about it more, I believe we must be also measuring the maximum height of the alt text for inter-staff spacing, with different baselines for alt text and nabc neumes I bet we also need to measure separately the maximum heights of the alt text vs. maximum height of nabc neumes and use the corresponding baseline for those. Or is that not the case?
As for the copy'n'paste, I think only the \gre@typesetnabcabovelines macro is large enough that it would be perhaps useful to use just one macro with an extra argument instead.

@henryso
Copy link
Contributor

henryso commented Jun 22, 2016

The calculation (not that I understand it completely) that you speak of is \gre@calculate@additionalspaces in gregoriotex-spaces.tex. I think it's being put into \gre@dimen@additionaltopspacealt.

@jakubjelinek
Copy link
Contributor Author

\gre@dimen@additionaltopspacealt I already replace with \gre@dimen@additionaltopspacenabc
for \GreNabcAboveLines. I've tried Sr. Maria's testcase copied 3 times and the spacing between the top of the nabc neumes and previous line of text is the same when \GreNabcAboveLines and \GreTextAboveLines is used for nabc, so I think with the cleanup change I've just pushed it should DTRT and not be too ugly anymore (but I'm never sure about \relax and % at the end of lines).

@eroux
Copy link
Contributor

eroux commented Jun 23, 2016

This looks ok to me

@henryso
Copy link
Contributor

henryso commented Jun 23, 2016

I'll merge this and review tests some time today.

@henryso henryso merged commit c1b861e into gregorio-project:release-4.2 Jun 23, 2016
henryso added a commit to henryso/gregorio-test that referenced this pull request Jun 23, 2016
henryso added a commit to gregorio-project/gregorio-test that referenced this pull request Jun 23, 2016
rpspringuel added a commit to rpspringuel/gregorio-test that referenced this pull request Sep 24, 2016
* release-4.2: (72 commits)
  Added a test for the suppression of \GreLastOfScore. Tests gregorio-project/gregorio#1205.
  Updated test harness to handle gregorio executable with version number. Tests gregorio-project/gregorio#1197.
  Updated tests to match the oriscus orientation at unison change. Tests gregorio-project/gregorio#1177.
  Small inconsistency
  Added a test to exercise per-line dimension changes. Tests gregorio-project/gregorio#1156.
  Accepted test results after merge.
  accept tests for fix-1144
  accept changes for #1169
  accept tests for 1146
  accept tests for #1146
  accept changes for fix-1138
  changes for fix-1137
  Accepted updated results after merging gregorio-project/gregorio#1154. Tests gregorio-project/gregorio#1153.
  Accepted updated results after merge of release-4.2 into fix-1155.
  accept changes for fix-1152
  add tests for 1155
  accept tests for fix-1155
  Accepted expectations after #1148. Tests gregorio-project/gregorio#1145.
  Added the test from gregorio-project/gregorio#1141.
  Added the test from gregorio-project/gregorio#1139.
  ...
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