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

Suppressed a z0 custos separated from a line break (z) by only bars/clefs. #1198

Merged
merged 3 commits into from
Aug 9, 2016

Conversation

henryso
Copy link
Contributor

@henryso henryso commented Aug 6, 2016

Fixes #1190.

Some tests (that happen to employ this sequence of figures) change, and a test was added to exercise all the variants in the issue post.

Please review and merge if satisfactory.

@@ -5,6 +5,7 @@ As of v3.0.0 this project adheres to [Semantic Versioning](http://semver.org/).
[Unreleased][unreleased]
## Changed
- Notes are now left-aligned as if all clefs had the same width as the largest clef in the score. You can get previous behavior back with `\grebolshiftcleftype{current}`, or temporary force alignment until the end of a score with `\grelocalbolshiftcleftype`. See Documentation of these functions and [#1189](https://github.com/gregorio-project/gregorio/issues/1189).
- If only bars and clefs are between an explicit custos `(z0)` and a line break `(z)`, the explicit custos will be suppressed (see [1190](https://github.com/gregorio-project/gregorio/issues/1190)).
Copy link
Contributor

@eroux eroux Aug 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think the most important part is that now things like (c4z) will work properly.

Edit: sorry, I didn't realize you din't implement it yet!

@eroux
Copy link
Contributor

eroux commented Aug 6, 2016

Well, the changes look OK to me, but I don't think they really make sense without the changes that make (c4z) work properly (meaning: just changing clef, not prinint it)

@henryso
Copy link
Contributor Author

henryso commented Aug 6, 2016

Ok, I'm confused. How is (c4z) supposed to change?

@eroux
Copy link
Contributor

eroux commented Aug 6, 2016

Hmmm, I realize I wasn't clear in my initial comment, for instance in #1190 (comment) what I call a "clef change" is not really typing the clef, it's just instructing GregorioTeX to change the clef at the beginning of lines... Basically, the idea is that when a z is present in the area of a clef change, we don't make a discretionary with the different case (line breaking here or not), we directly typeset the relevant case. For instance the output of (z0::c4z) in GregorioTeX would be the same as the one (::z) plus the instruction to change the clef at the beginning of lines. Do you see what I mean?

@henryso
Copy link
Contributor Author

henryso commented Aug 6, 2016

Ok, if I understand correctly, a clef immediately before a line break in GABC should be typeset as an instruction to change the "line clef" starting with the line after the line break. Is that correct?

@henryso
Copy link
Contributor Author

henryso commented Aug 7, 2016

This gets tricky positioning the custos in the localrightbox before such a clef change. Any ideas would be appreciated.

@eroux
Copy link
Contributor

eroux commented Aug 7, 2016

It's correct yes, and the discretionary mechanism should not be used. For the custos, I think it's computed in the exact same way as if it was a z0, what would be the difference? If what you mean is that there is no macro to tell GregorioTeX to use an end of line custos without typesetting it directly I can add one

@henryso
Copy link
Contributor Author

henryso commented Aug 7, 2016

I need help here. I made the changes, and the executable emits \GreNextCustos and \GreSetLinesClef instead of a "real" clef when the clef is at the end of a line. I think it's emitting the correct thing, but the localleftbox does not seem to be updated. The localrightbox is correctly placing the custos for the clef change. Any ideas what I'm doing wrong?

@eroux
Copy link
Contributor

eroux commented Aug 7, 2016

Thanks a lot, I'll take a look tomorrow

@eroux
Copy link
Contributor

eroux commented Aug 8, 2016

Well, these days seem very busy, I'm not sure I'll be able to test, I'll tell you when I can (maybe not before next week). @rpspringuel maybe you would have time to take a look and debug the TeX side?

@eroux
Copy link
Contributor

eroux commented Aug 8, 2016

Trying the pull request I'm getting:

gabc/gabc-score-determination-l.c: In function ‘yy_get_next_buffer’:
gabc/gabc-score-determination.l:73:23: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
         for (n = 0; n < max_size \
                       ^
gabc/gabc-score-determination-l.c:1941:3: note: in expansion of macro ‘YY_INPUT’
   YY_INPUT( (&YY_CURRENT_BUFFER_LVALUE->yy_ch_buf[number_to_move]),
   ^~~~~~~~
gabc/gabc-score-determination-l.c: In function ‘gabc_score_determination__scan_bytes’:
gabc/gabc-score-determination-l.c:2453:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for ( i = 0; i < _yybytes_len; ++i )
                 ^
mv -f gabc/.deps/gregorio-gabc-score-determination-l.Tpo gabc/.deps/gregorio-gabc-score-determination-l.Po
gcc -DHAVE_CONFIG_H -DBRANCH_VERSION='"henryso-fix-1190-9bb3fd0-3898"' -I.  -I../src -I../src/gabc  -I../src/dump  -I../src/gregoriotex -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2   -g -O2 -std=gnu89 -pedantic -fstack-protector-strong -fPIE -Wformat=2 -Werror=format-security -Wstrict-prototypes -Wdeclaration-after-statement -Wall -Wextra -MT gabc/gregorio-gabc-notes-determination-l.o -MD -MP -MF gabc/.deps/gregorio-gabc-notes-determination-l.Tpo -c -o gabc/gregorio-gabc-notes-determination-l.o `test -f 'gabc/gabc-notes-determination-l.c' || echo './'`gabc/gabc-notes-determination-l.c
gabc/gabc-notes-determination-l.c: In function ‘yy_get_next_buffer’:
gabc/gabc-notes-determination-l.c:11599:18: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   for ( n = 0; n < max_size && \
                  ^
gabc/gabc-notes-determination-l.c:13259:3: note: in expansion of macro ‘YY_INPUT’
   YY_INPUT( (&YY_CURRENT_BUFFER_LVALUE->yy_ch_buf[number_to_move]),
   ^~~~~~~~
gabc/gabc-notes-determination-l.c: In function ‘gabc_notes_determination__scan_bytes’:
gabc/gabc-notes-determination-l.c:13763:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for ( i = 0; i < _yybytes_len; ++i )

(with gcc6 on debian/sid)

@eroux
Copy link
Contributor

eroux commented Aug 8, 2016

Ok, nailed it: \gre@save@clef also needs to be called... I don't have time anymore tonight, but I think the best would be to call it from \GreSetLinesClef directly, I'm not really sure why it's not done that way....

[with thanks to @eroux]
Part of the implementation for gregorio-project#1190.
@henryso
Copy link
Contributor Author

henryso commented Aug 9, 2016

Thanks. I made the change, reviewed the tests and accepted those changes as well.

This is now ready for review and merge if satisfactory.

@eroux
Copy link
Contributor

eroux commented Aug 9, 2016

Well, everything looks ok to me, maybe we could just add a test with z- and Z-?

@henryso
Copy link
Contributor Author

henryso commented Aug 9, 2016

Done. Please review.

@eroux eroux merged commit f9b4ad0 into gregorio-project:develop Aug 9, 2016
@henryso henryso deleted the fix-1190 branch August 9, 2016 13:49
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.

2 participants