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

Vertical episemus on low notes ignored in vertical spacing #60

Closed
rpspringuel opened this issue Feb 27, 2015 · 8 comments
Closed

Vertical episemus on low notes ignored in vertical spacing #60

rpspringuel opened this issue Feb 27, 2015 · 8 comments

Comments

@rpspringuel
Copy link
Contributor

When a score has a low note of a' or b' the presence of the episemus is ignored when determining the spacing between the notes and the text. This leads to the text overlapping with the episemus.

The problem appears to be in the first argument choices that Gregorio makes for \grefirstlinebottomspace. They should be:
0. note on first line or higher

  1. note below first line (c)
  2. note on 0th line (b) or note below first line with vertical episemus (c')
  3. note below 0th line (a) or note on 0th line with vertical episemus (b')
  4. note below 0th line with vertical episemus (a')

Cases appear to currently be:
0. note on first line or higher

  1. note below first line (c)
  2. note on 0th line (b) or note below first line with vertical episemus (c')
  3. note below 0th line (a)

For consistency, case 1 should also be selected for note on first line with vertical episemus (d'), but my testing indicates that there is no spacing issue there right now.

@eroux eroux changed the title Vertical episemus on low notes ignored Vertical episemus on low notes ignored in vertical spacing Mar 1, 2015
@rpspringuel rpspringuel modified the milestone: 3.0 Mar 16, 2015
@eschwab
Copy link
Contributor

eschwab commented Mar 30, 2015

Is this still on the docket for 3.0?

@eroux
Copy link
Contributor

eroux commented Mar 30, 2015

Good point, as it might introduce an incompatible change, maybe it would be good to do it... I can do it (tomorrow), if someone wants to do it before, please do!

@henryso
Copy link
Contributor

henryso commented Mar 30, 2015

How incompatible a change? Does the user have to modify their gabc or tex files? If it's just a bug-fix, even if it changes the output (thus fixing the bug), then this can even be considered a patch release (i.e., 3.0.1).

Also, if I understand the bug description, it's a problem with the C code, right? That the C code chooses the incorrect value for the parameter?

Is this a long-standing bug? If it wasn't introduced recently (i.e., 2.4.2, 2.4.3, 3.0.0), then I'd vote to wait for the next release instead of rushing a fix.

@rpspringuel
Copy link
Contributor Author

This should be a bugfix change. It will change scores visually, but there is no change in the underlying gabc or in a user's TeX. It is a long-standing bug. I discovered it while making my changes to the spacing code, but didn't fix it at the time.

Fixing it requires a change in the C code (to pick the correct value for the parameter) and making some modifications to \grefirstlinebottomspace in gregoriotex-spaces.tex. The latter needs a case 4 similar to what \gre@calculate@additionalspaces above it has. I don't know what changes need to be made for the former.

@eroux
Copy link
Contributor

eroux commented Mar 31, 2015

Oh ok, I thought incompatible changes would have to occur in the generated tex files, but if it's not the case, we can delay this feature.

@eroux eroux removed this from the 3.0 milestone Mar 31, 2015
@henryso
Copy link
Contributor

henryso commented Mar 31, 2015

As far as I can tell, the C code pas passing 4 for the first argument. I think the problem is that \grefirstlinebottomspace is simply not handling a case 4.

@rpspringuel
Copy link
Contributor Author

Did anyone check to see that b' is getting mapped to case 3? Based on what I wrote when I found the bug, that wasn't happening (in addition to case 4 being missing).

@henryso
Copy link
Contributor

henryso commented Mar 31, 2015

I've confirmed that b' is mapping to case 3 in release-3.0 gregorio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants