Re: Bugfix for issue 1630 (issue4490045)
LGTM Carl http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/28 16:13:43, benko.pal wrote: aargh, that's not too readable. what I actually suggest is replacing lines 204-207 of http://codereview.appspot.com/4490045/diff/12001/lily/completion-note-heads-engraver.cc File lily/completion-note-heads-engraver.cc (right): 204 if ((left_to_do_ - note_dur.get_length ()) Rational (0)) 205 event-set_property(autosplit-end, ly_bool2scm (true)); 206 else 207 event-set_property(autosplit-end, ly_bool2scm (false)); by nbsp; event-set_property (autosplit-end, nbsp; nbsp; ly_bool2scm (left_to_do_ - note_dur.get_length () 0)); Pal That was the original code. It was pointed out (see Neil's comment above) that the only check on this is whether or not it is greater than zero, so a boolean works. Hence, the code was changed to use a boolean. http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Bugfix for issue 1630 (issue4490045)
On 2011/05/29 08:53:30, benko.pal wrote: I must miss something, to me it's still a boolean. and (still to me) it's not an inline conditional, but an assignment of a boolean expression to a boolean variable. No, it is I that missed something. I'm sorry for the noise. Thanks, Carl http://codereview.appspot.com/4490045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
The code looks fine in general, but I question two of the properties that have been added for MultiMeasureRest. http://codereview.appspot.com/4536068/diff/19001/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): http://codereview.appspot.com/4536068/diff/19001/lily/multi-measure-rest.cc#newcode329 lily/multi-measure-rest.cc:329: longest-church-rest I'm not sure I understand how longest-church-rest interacts with \ usable-duration-logs. Why can't longest-church-rest just be the smallest value in usable-duration-logs? Why do we need a separate property for this? Also, why do we need a grob property for measure-duration-log? The length of a measure is a context property of the Timing context; I don't see a reason to have the possibility of having a different measure duration in the time signature and in the multi-measure rest grob. http://codereview.appspot.com/4536068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows LilyPond to ignore certain note-heads in a stem. (issue4547058)
Do we need to change the definition of stem-attachment in scm/define-grobs.scm to be ,boolean-or-number-pair? (and maybe define a new ,boolean-or-number-pair predicate)? Do we need to make any changes to ly/note-head-scheme.cc? Do we need to check for #f in ly/note-collision.cc? I just did a git grep stem-attachment and saw these places where stem-attachment was used. Thanks, Carl http://codereview.appspot.com/4547058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Loose-lines honor padding between systems (issue4553060)
IIRC, part of the motivation for the new spacing algorithm was the desire to put staves in fixed positions on the page, regardless of what else was around. Does this patch eliminate this possibility? If so, is it possible to disable it? I guess by setting padding to 0? http://codereview.appspot.com/4553060/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Loose-lines honor padding between systems (issue4553060)
On 2011/05/25 18:08:59, Keith wrote: On 2011/05/25 13:43:55, Carl wrote: IIRC, part of the motivation for the new spacing algorithm was the desire to put staves in fixed positions on the page, regardless of what else was around. Does this patch eliminate this possibility? No. Great! LGTM. http://codereview.appspot.com/4553060/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix determine-frets so that it preserves note order (issue4518045)
On 2011/05/26 03:26:32, Keith wrote: `make check` was crashing on an unbound variable 'note'. Given line 345 above the fix was so obvious that I just pushed it. Thanks for the fix. I had found that fix as well, and was getting ready to push it. But there's still another problem. Right now 'ignore has the same effect as 'recalculate, and it shouldn't. So this patch fixes compiling, but not the regtest. Thanks, Carl http://codereview.appspot.com/4518045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New alist to replace special characters. (issue4553056)
http://codereview.appspot.com/4553056/diff/1/lily/text-interface.cc File lily/text-interface.cc (right): http://codereview.appspot.com/4553056/diff/1/lily/text-interface.cc#newcode40 lily/text-interface.cc:40: int max_length = scm_to_int (ly_chain_assoc_get (ly_symbol2scm (replacement-string-max-length), Why is string-max-length needed? http://codereview.appspot.com/4553056/diff/1/ly/special-characters.ly File ly/special-characters.ly (right): http://codereview.appspot.com/4553056/diff/1/ly/special-characters.ly#newcode1 ly/special-characters.ly:1: #(define special-characters-alist I think this should be a .scm file, rather than a .ly file. I'm concerned about the default position of moving from UTF-* to ascii for special characters. I think that's moving in the wrong direction. I do think the ligature replacement is very good. But I could easily be persuaded that this is the right thing to do. http://codereview.appspot.com/4553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New alist to replace special characters. (issue4553056)
On 2011/05/23 22:15:38, Bertrand Bordage wrote: Yes. Knowing this, I suggest we keep whitespaces, punctuation, quotes and word dividers (with some small changes). There's still something that bothers me : isn't there some special characters that you can't do with you keyboard ? Even on linux I can't type some symbols like ſ or • without copying/pasting. So, to your question What is the advantage to typing \\aa instead of å? I answer I don't have any key for this. See the Wikipedia entry on Unicode input for ways to get characters that aren't on your keyboard: http://en.wikipedia.org/wiki/Unicode_input HTH, Carl http://codereview.appspot.com/4553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: New alist to replace special characters. (issue4553056)
On 2011/05/23 23:11:33, Bertrand Bordage wrote: Making this easier should be the OS's job. Yes, I agree. That's why I'm not in favor of making it part of LilyPond for us to maintain. Having the facility to do the general substitution as part of LilyPond is fine, IMO. Having the list of all the Unicode characters is not a good idea. Having that list as an LSR snippet would be OK. That way it's not part of the contract we have with users to maintain it. Thanks, Carl http://codereview.appspot.com/4553056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows LilyPond to ignore certain note-heads in a stem. (issue4547058)
What if instead of setting a boolean stem-ignore, you just set stem-attachment = ##f in order to get this behavior? This would be consistent with setting stencil = ##f in order to eliminate the stencil. If you want to keep a separate boolean, I think I'd prefer the name no-stem to stem-ignore. The object of the property is to get a notehead without a stem, and no-stem seems to communicate that better than stem-ignore, IMO. Carl http://codereview.appspot.com/4547058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Adds longas, maximas and non-standard tweaks to MultiMeasureRest (issue4536068)
This looks generally good to me.
I'm concerned about the name duration-log-list. I've commented more
on it below.
Thanks,
Carl
http://codereview.appspot.com/4536068/diff/1/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):
http://codereview.appspot.com/4536068/diff/1/scm/define-grob-properties.scm#newcode232
scm/define-grob-properties.scm:232: (duration-log-list ,list? List of
@code{duration-log}.)
This name is nice and generic, which is good. Bit it has no information
content as far as I can see. Can we make it more explicit by changing
either the name (to something like usable-duration-logs) or the
description (to something like List of duration-logs that can be used
in typesetting the grob)?
As I read through things I couldn't understand what duration-log-list
was for until I read the code (and implied it from the regression test).
http://codereview.appspot.com/4536068/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes the assert problem coming from ledger-line-spanner.cc. (issue4535055)
LGTM. Carl http://codereview.appspot.com/4535055/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Start towards fixing the tie / time signature collision problem. (issue4528061)
I didn't follow everything you've done, because I didn't have time to look through it all in detail. You should be aware, however, that the points for the tie are different from the extents. The left end of a tie will be at t = -delta, rather than at t=0. This is because of the width of the curve; the bezier curve goes through the center of the ink; there's a non-zero width to the bezier. I'm not sure if this is responsible for your difficulties, but I think it may be. HTH, Carl http://codereview.appspot.com/4528061/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Better width functions for the current arpeggio print functions. (issue4517051)
Seems reasonable to me. I couldn't think of any way to generalize the internal call. Carl http://codereview.appspot.com/4517051/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fattens the 256 first braces. (issue4518052)
LGTM, with a small nitpick. I like the new braces better. Carl http://codereview.appspot.com/4518052/diff/1/mf/feta-braces.mf File mf/feta-braces.mf (right): http://codereview.appspot.com/4518052/diff/1/mf/feta-braces.mf#newcode148 mf/feta-braces.mf:148: fatten_factor := 1.5; I think that good practice would have you do a save when you define a new variable, so you won't override it if it's defined elsewhere in the file. Using save creates a local variable. http://codereview.appspot.com/4518052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix determine-frets so that it preserves note order (issue4518045)
Reviewers: MikeSol, Message: In response to Mike's request, I've fixed the problem in determine-frets that sometimes changed the order of the notes in the chord. This keeps the glissando between the same notes in the TabStaff as in the Staff. I've made enough changes that I'd like to get a review of the changes. Thanks, Carl Description: Fix determine-frets so that it preserves note order Please review this at http://codereview.appspot.com/4518045/ Affected files: M input/regression/tablature-harmonic.ly M scm/translation-functions.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: NR rewrite of 3.2 Titles and Headers (issue4124056)
Looks mostly good to me.
Carl
http://codereview.appspot.com/4124056/diff/32001/Documentation/notation/input.itely
File Documentation/notation/input.itely (right):
http://codereview.appspot.com/4124056/diff/32001/Documentation/notation/input.itely#newcode667
Documentation/notation/input.itely:667: Text fields left unset in a
@code{\header} block are replaced with
This paragraph has three different ideas -- null markups, piece and
opus, and forcing titles to start on a new page.
They should probably be separated into three different paragraphs.
http://codereview.appspot.com/4124056/diff/32001/Documentation/notation/input.itely#newcode937
Documentation/notation/input.itely:937:
Need @seealso for the @ref in this node -- Title blocks explained,
Default layout of book and score title blocks,.
http://codereview.appspot.com/4124056/diff/32001/Documentation/notation/input.itely#newcode987
Documentation/notation/input.itely:987: @end lilypond
Need @seealso here, with all of the @ref in this node (Default layout of
book and title blocks).
http://codereview.appspot.com/4124056/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix calculation of vertical offset when 'staff-padding is set (issue4489042)
LGTM. I tried fixing this but couldn't track down all the interfaces to figure it out. I saw that we were going off staff position rather than parent position, but didn't know where to fix it. Thanks! http://codereview.appspot.com/4489042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Gets rid of chordGlissando. (issue4444066)
LGTM. It wasn't necessary to remove \chordGlissando from the text of the non-english docs, and it may even have been best not to do so. We need to keep compiling working, so if these docs had a snippet containing \chordGlissando, we would have needed to change that. But since the snippet was included as a file, it was sufficient to change the file. Please go ahead and push. Thanks, Carl http://codereview.appspot.com/066/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix make doc error: Character set messup in pdf-scheme.cc (issue4449061)
LGTM, with Neil's comments implemented. http://codereview.appspot.com/4449061/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Articulate patch for barcheck warnings. (issue4435069)
LGTM, although I'm not an expert on articulate. http://codereview.appspot.com/4435069/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: rest-collision.cc: adjust all rests in column. Issues 1618 and 1547 (issue4442083)
LGTM. A couple of non-essential comments. http://codereview.appspot.com/4442083/diff/12001/input/regression/rest-polyphonic-2.ly File input/regression/rest-polyphonic-2.ly (right): http://codereview.appspot.com/4442083/diff/12001/input/regression/rest-polyphonic-2.ly#newcode5 input/regression/rest-polyphonic-2.ly:5: result in collision, but is supressed if the rest has a pitch. The texidoc should have a statement that describes what the output should look like. http://codereview.appspot.com/4442083/diff/12001/lily/rest-collision.cc File lily/rest-collision.cc (right): http://codereview.appspot.com/4442083/diff/12001/lily/rest-collision.cc#newcode281 lily/rest-collision.cc:281: Move around ordinary rests (not multi-measure-rests) to avoid Perhaps include pitched rests along with mult-measure-rests in the description of rests not affected by this interface. http://codereview.appspot.com/4442083/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Avoid repeats of 'staff-affinity' warning; change text. (issue4278058)
LGTM. http://codereview.appspot.com/4278058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issues 1639 and 1640. (issue4457042)
LGTM. http://codereview.appspot.com/4457042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issues 1639 and 1640. (issue4457042)
Regtest for 1640? http://codereview.appspot.com/4457042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Another fix candidate for issue 1613. (issue4426072)
Looks good to me -- just a comment on a variable name. http://codereview.appspot.com/4426072/diff/1001/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/4426072/diff/1001/lily/beam.cc#newcode1272 lily/beam.cc:1272: Interval vorboten; We shouldn't use german words for variable names, I think. English is the standard code language for lilypond. And if we do, we should spell it properly -- I'm pretty sure the spelling is verboten. Perhaps disallowed? http://codereview.appspot.com/4426072/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows glissandi between chords (issue4442082)
Excellent work! A couple of comments below. http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties.scm File scm/define-context-properties.scm (right): http://codereview.appspot.com/4442082/diff/3001/scm/define-context-properties.scm#newcode260 scm/define-context-properties.scm:260: (glissandoMap ,list? A map in the form of '((source1 . target1) I think we ought to enable the current behavior (one glissando line per chord, instead of one per note) to be kept (so we won't break existing scores). Perhaps a value of #f should be used to indicate keeping the old behavior. http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-interfaces.scm File scm/define-grob-interfaces.scm (right): http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-interfaces.scm#newcode103 scm/define-grob-interfaces.scm:103: '(glissando-index)) As I mention elsewhere, I don't think this is a user property, so needn't be included in the interface. http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4442082/diff/3001/scm/define-grob-properties.scm#newcode412 scm/define-grob-properties.scm:412: (glissando-index ,integer? The index of a glissando in its note It seems to me like this should be an internal grob property, rather than a user grob property, and that it need not be part of an interface. I can't imagine how it ought to be tweaked by the user. http://codereview.appspot.com/4442082/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add predefined mandolin fretboards to lilypond. (issue4370054)
LGTM. If I had done it I would have defined chordShapes for the common shapes and moved them up and down the fretboard. But that's not necessary; I'd totally support this addition. Sorry I'm so slow on this -- I reviewed it a while ago and forgot to comment. Carl http://codereview.appspot.com/4370054/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
MIDI: partcombine regtest needs skips (issue 1609) (issue4433044)
The patch seems appropriate. However, I think there should be more patches, IIUC. There are comments in this file referring to being automatically generated. While this may have been the original genesis of the file, since it's in input/regression it's not automatically generated from out/*. I think these comments should go away. But maybe I'm mistaken. Thanks, Carl http://codereview.appspot.com/4433044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: pdf-metadata: Use UTF-16BE for metadata if required (fix #1502) (issue4398046)
On 2011/04/14 22:52:04, reinhold_kainhofer.com wrote: Yes, of course! As Graham wants to push Bertrand's patch on Saturday morning, I'll update my patch accordingly. As Bertrand's patch was already out, I didn't even try to include its functionality (but added the TODO comment as a reminder). Bertrand's patch has now been pushed. If you can finish your patch, then we can get it pushed and applied to stable/2.14 Thanks, Carl http://codereview.appspot.com/4398046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: pdf-metadata: Use UTF-16BE for metadata if required (fix #1502) (issue4398046)
LGTM Carl http://codereview.appspot.com/4398046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
pdf-metadata: Use UTF-16BE for metadata if required (fix #1502) (issue4398046)
THe structure of the patch looks good, but I think it conflicts with Bertrand's patch for 1605. Perhaps the two could be combined. Thanks, Carl http://codereview.appspot.com/4398046/diff/1/scm/framework-ps.scm File scm/framework-ps.scm (right): http://codereview.appspot.com/4398046/diff/1/scm/framework-ps.scm#newcode405 scm/framework-ps.scm:405: (define (metadata-encode val) This needs to be coordinated with Bertrand's patch for issue 1605: http://codereview.appspot.com/4377054/ You use metadata-encode (but don't escape the parentheses and backslashes. He just escapes the parentheses and backslashes. Combining the two would be goo, I think. http://codereview.appspot.com/4398046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix issue 1605 (issue4377054)
I'd recommend that the new function be placed in scm/output-ps.scm Thanks, Carl http://codereview.appspot.com/4377054/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix issue 1605 (issue4377054)
On 2011/04/12 13:33:05, Bertrand Bordage wrote: How ? output-ps is dependant of framework-ps... When I move it to output-ps, it doesn't work. Never mind. I don't know what I was thinking when I made that comment. Sorry, Carl http://codereview.appspot.com/4377054/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows users to prevent rests from automatically shifting. (issue4385053)
Wow -- what a quick response! Thanks! I have a couple of comments. Carl http://codereview.appspot.com/4385053/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4385053/diff/1/scm/define-grob-properties.scm#newcode61 scm/define-grob-properties.scm:61: (automatic-shift ,boolean? Should a rest be automatically shifted?) Since this applies to both MMRest and Percent Repeat, should it say rest? Also, I think it should have a statement that defines the kind of shift we're talking about, if possible. http://codereview.appspot.com/4385053/diff/1/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4385053/diff/1/scm/define-grobs.scm#newcode1545 scm/define-grobs.scm:1545: (automatic-shift . #t) You've added this property to the PercentRepeat grob, but I don't see where the engraver has been changed. http://codereview.appspot.com/4385053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows users to prevent rests from automatically shifting. (issue4385053)
Instead of adding a property, is there a way to just make the default value of the property be staff_space? Thanks, Carl http://codereview.appspot.com/4385053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows users to prevent rests from automatically shifting. (issue4385053)
Instead of adding a property, is there a way to just make the default value of the property be staff_space? Thanks, Carl http://codereview.appspot.com/4385053/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Add predefined mandolin fretboards to lilypond. (issue4384055)
Reviewers: ,
Message:
Marc Hohl has prepared a patch for mandolin predefined fretboards.
The patch looks good to me.
Please review.
Thanks,
Carl
http://codereview.appspot.com/4384055/diff/1/Documentation/notation/fretted-strings.itely
File Documentation/notation/fretted-strings.itely (right):
http://codereview.appspot.com/4384055/diff/1/Documentation/notation/fretted-strings.itely#newcode1905
Documentation/notation/fretted-strings.itely:1905:
@file{scm/string-tunings-init.scm} contains predefined banjo tunings.
Thanks for the catch!
Description:
Add predefined mandolin fretboards to lilypond.
Please review this at http://codereview.appspot.com/4384055/
Affected files:
A Documentation/included/display-predefined-mandolin-fretboards.ly
M Documentation/notation/fretted-strings.itely
M Documentation/notation/notation-appendices.itely
A ly/predefined-mandolin-fretboards.ly
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add some polyphonically directed grobs (issue4387046)
LGTM. Carl http://codereview.appspot.com/4387046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Autobeam nitpicks (issue4385050)
LGTM Carl http://codereview.appspot.com/4385050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Autobeam nitpicks (issue4385050)
On 2011/04/10 02:47:42, Carl wrote: LGTM Carl If you have push access, go ahead and push. If not, send it to me and I'll push it. No further review is necessary. Thanks, Carl http://codereview.appspot.com/4385050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix 1569: Bad behavior of NoteNames context (issue4312043)
Reviewers: ,
Message:
Here's a proposed patch for Issue 1569.
Please review.
Thanks,
Carl
Description:
Fix 1569: Bad behavior of NoteNames context
Changed default value of 'staff-affinity to #UP
Changed default spacing parameters to be the same as for Lyrics
context
Added some documentation about spacing of nonstaff lines to help
users better understand nonstaff line spacing.
Added regression test file
Please review this at http://codereview.appspot.com/4312043/
Affected files:
M Documentation/notation/spacing.itely
M input/regression/note-names.ly
M ly/engraver-init.ly
Index: Documentation/notation/spacing.itely
diff --git a/Documentation/notation/spacing.itely
b/Documentation/notation/spacing.itely
index
8c2edce2d5c0b507271dc2223b69496978eb04ae..00901e7e2ec4fac000c92a7e1e96351d1bd0baee
100644
--- a/Documentation/notation/spacing.itely
+++ b/Documentation/notation/spacing.itely
@@ -1967,9 +1967,9 @@ set to @code{UP} should not immediately follow one
that is set to
@code{DOWN}; those at the bottom should use @code{UP}. Setting
@code{staff-affinity} for a staff causes it to be treated as a
non-staff line. Setting @code{staff-affinity} to @code{#f} causes
-a non-staff line to be treated as a staff.
-
-@c TODO: verify last clause below (even if other...)
+a non-staff line to be treated as a staff. Setting
+@code{staff-affinity} to @code{UP}, @code{CENTER}, or @code{DOWN}
+causes a staff to be spaced as a non-staff line.
@item nonstaff-relatedstaff-spacing
The distance between the current non-staff line and the nearest
@@ -1979,7 +1979,12 @@ either @code{UP} or @code{DOWN}. If
@code{staff-affinity} is
@code{CENTER}, then @code{nonstaff-relatedstaff-spacing} is used
for the nearest staves on @emph{both} sides, even if other
non-staff lines appear between the current one and either of the
-staves.
+staves. This means that the placement of a non-staff line depends
+on both the surrounding staves and the surrounding non-staff lines.
+Setting the @code{stretchability} of one of these types of spacing to
+a small value will make that spacing dominate. Setting the
+@code{stretchability} to a large value will make that spacing have
+little effect.
@item nonstaff-nonstaff-spacing
The distance between the current non-staff line and the next
Index: input/regression/note-names.ly
diff --git a/input/regression/note-names.ly b/input/regression/note-names.ly
index
2c756679a39b997fec252603242fb6f7112ecd0b..615b37a3c8fcc8aadeb1b548a696f4ac54be9085
100644
--- a/input/regression/note-names.ly
+++ b/input/regression/note-names.ly
@@ -1,27 +1,37 @@
-\version 2.13.38
+\version 2.13.56
\header {
- texidoc = Various languages are supported for note names input.
-Selecting another language within a music expression is possible,
-and doesn't break point-and-click abilities.
-
-}
+ texidoc =
+NoteNames context should be close to the related notes,
+and should not collide with the tempo markings.
+
+}
-%% Old syntax.
-\include english.ly
+\paper {
+ system-system-spacing
+#'basic-distance = #10 % increase this value for more space
+}
-\relative c'' {
- g4 bf d c
+notes = \relative c {
+ c'4 c c c
+}
- %% Manual override of the pitchnames variable
- %% and the parser note names:
- #(begin
-(set! pitchnames (ly:assoc-get 'nederlands language-pitch-names))
-(ly:parser-set-note-names parser pitchnames))
- bes4 a g fis
+mylyrics = \lyricmode {
+ \tempo Allegro
+ ly -- ric ly -- ric
+}
- %% The \language command acts in the same way:
- \language italiano
- sol4 fa mib re
+\score {
+
+\new Voice = voice {
+ \repeat unfold 13 \notes
+}
+\context NoteNames {
+ \repeat unfold 13 \notes
+}
+\new Lyrics \lyricsto voice {
+ \repeat unfold 13 \mylyrics
+}
+
}
Index: ly/engraver-init.ly
diff --git a/ly/engraver-init.ly b/ly/engraver-init.ly
index
0720893ffc725fb0157dcab101b54310dcb48612..aeb379d574260ef05d767db24b3fabd6866427a7
100644
--- a/ly/engraver-init.ly
+++ b/ly/engraver-init.ly
@@ -461,8 +461,18 @@ printing of a single line of lyrics.
\description A context for printing the names of notes.
\consists Axis_group_engraver
- % FIXME: not sure what the default should be here.
- \override VerticalAxisGroup #'staff-affinity = #DOWN
+ \override VerticalAxisGroup #'staff-affinity = #UP
+ \override VerticalAxisGroup #'nonstaff-nonstaff-spacing =
+#'((basic-distance . 0)
+ (minimum-distance . 2.8)
+ (padding . 0.2)
+ (stretchability . 0))
+ \override VerticalAxisGroup #'nonstaff-relatedstaff-spacing =
+#'((basic-distance . 5.5)
+ (padding . 0.5)
+ (stretchability . 1))
+ \override VerticalAxisGroup
+#'nonstaff-unrelatedstaff-spacing #'padding = 1.5
\consists Tie_engraver
\consists Note_name_engraver
___
lilypond-devel mailing list
lilypond-devel@gnu.org
Re: downstem 64th and 128th flag touchup (issue4273074)
LGTM. Carl http://codereview.appspot.com/4273074/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: unbeamed 32nd stem is shortened by 0.25 ss to fit beamed stems better. (issue4243071)
LGTM. Carl http://codereview.appspot.com/4243071/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: unbeamed 32nd stem is shortened by 0.25 ss to fit beamed stems better. (issue4243071)
Also, it's a simple enough change that I don't think any discussion is necessary. We should go ahead and push it. It's just a change of a single constant value. Carl http://codereview.appspot.com/4243071/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Makes the footnote separator span part of the page (issue4237059)
LGTM Carl http://codereview.appspot.com/4237059/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: add [pagesize=xyz] option. (issue4239048)
LGTM. Thanks, Carl http://codereview.appspot.com/4239048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fret diagram fixes (issue4176056)
LGTM. Thanks, Carl P.S. Can you propose your patch to git-cl to the git-cl maintainers? I think that your approach is the right one. But I don't want to have us in the position of needing a custom git-cl. http://codereview.appspot.com/4176056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: DOC: NR 1.5.2 Multiple voices - part combining (issue4188056)
On 2011/02/26 20:01:39, Colin Campbell wrote: I like that very much, James, thanks! A question for Reinhold, though: do I gather correctly that \partcombine is applied to a Staff, and turns the combining mechanism on, while \partcombineAutomatic is applied to a single Voice? That being so, does it turn off a previous \partcombineApart, e.g.? I would say that \partcombine is a function that applies to two music expressions. It creates Voices as necessary for the combined music. \partcombineAutomatic applies to the combined music at a given musical moment, and applies to both of the arguments to \partcombine at that time. The reason I would not say that \partcombine applies to a staff is that it doesn't set anything special for the Staff. It converts two separate music expressions into a set of music expressions necessary to support the appropriate combined Voices. Carl http://codereview.appspot.com/4188056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fret diagram fixes (issue4176056)
On 2011/02/17 16:17:29, nicolas.sceaux wrote: Hi, Here is a patch for fret diagrams, but as I have very little knowledge of them I may well be wrong on some points. First, it fixes sizing issues, when the size property is overridden: the xo signs became too big, and too far from the first fret. It seems that there was a unnecessary * size. Looks good to me. Thanks for checking this out. Then, it adds the possibility to use letters for fingers. On the book I'm reading, P is used for the thumb (pouce in French). A new feature is also to invert a dot color, on a per-dot basis. Also on the book I'm referring to, this is used to show where the fundamental note is on a chord. I like the fact that you've done this, and that you've done it only for the verbose style diagram. That way, you haven't added complexity to the terse diagram. Finally, the patch adds a way to customize the first fret label. There is also a modification of the first fret label position, but maybe this is a mistake. Is the label supposed to be vertically centered with the fret line? or the bottom of the label should be aligned with the fret line? In the former case, I should cancel the modification. From every reference I have, it is supposed to be vertically centered with the fret line. That was the intent in the original code. Thanks, Carl P.S. Can you adjust your mime-types entry for .scm files so that side-by-side diffs in Rietveld work? http://lists.gnu.org/archive/html/lilypond-devel/2011-01/msg00583.html http://codereview.appspot.com/4176056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 1278: Arrow notation for quarter-tones. (issue3789044)
This work looks good to me, but I'm not an expert in this area. I have one question, I think. Right now, the alteration consists of two integers, which have implied denominators of 1/2 and 1/4, if I understand correctly. Would it be more general to have the alteration consist of two rationals? Or could we have some way of defining a base-alteration pair that would be rationals? And then the implied denominators of 1/2 and 1/4 would no longer be implicit, but would be explicit in either the alteration or the base-alteration? I think (but am not sure) that such an implementation would meet the needs that Hans Aberg has defined for appropriate microtonal support. Of course, this entire suggestion could be garbage, because I am not an expert on this topic, as I mentioned above. Thanks, Carl http://codereview.appspot.com/3789044/diff/29001/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/3789044/diff/29001/lily/accidental-placement.cc#newcode239 lily/accidental-placement.cc:239: Alteration last_alteration (0); Why is this last_alteration (0), instead of last_alteration () or last_alteration (NATURAL)? http://codereview.appspot.com/3789044/diff/29001/lily/include/pitch.hh File lily/include/pitch.hh (right): http://codereview.appspot.com/3789044/diff/29001/lily/include/pitch.hh#newcode60 lily/include/pitch.hh:60: Pitch (int octave, int notename, int alt1, int alt2); I'm somewhat hesitant about this code that limits an alteration to two integers. Are we sure that this is general enough given the fact that we're moving to an Alteration datatype? http://codereview.appspot.com/3789044/diff/29001/lily/scale.cc File lily/scale.cc (right): http://codereview.appspot.com/3789044/diff/29001/lily/scale.cc#newcode36 lily/scale.cc:36: arguments are rational numbers giving the weigth in spelling: weigth - weight http://codereview.appspot.com/3789044/diff/29001/lily/scale.cc#newcode94 lily/scale.cc:94: scale. The number of pitches in this scale minus one The number of scale steps in an octave is the number of pitches in the global scale minus one. http://codereview.appspot.com/3789044/diff/29001/python/musicexp.py File python/musicexp.py (right): http://codereview.appspot.com/3789044/diff/29001/python/musicexp.py#newcode308 python/musicexp.py:308: return '(ly:make-pitch %d %d \'(%d . 0))' % (self.octave, Why is the 0 for the second alteration hard-coded? http://codereview.appspot.com/3789044/diff/29001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/3789044/diff/29001/scm/define-markup-commands.scm#newcode2418 scm/define-markup-commands.scm:2418: (interpret-markup layout props (markup #:musicglyph (assoc-get '(2 . 0) standard-alteration-glyph-name-alist Can this use SHARP, instead of (2 . 0)? http://codereview.appspot.com/3789044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Add dots to tocItemMarkup (issue4182056)
Looks very good. Just a couple more comments. Thanks, Carl http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#newcode994 scm/define-markup-commands.scm:994: (define (nest-patterns pattern pattern-width distance count patterns) It might be even better if this were defined as a markup command, for example pattern-markup. Then anybody else (like me) could use it for making a repeating pattern of markups (like a LyricExtender for shape notes). http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#newcode1032 scm/define-markup-commands.scm:1032: (x-offset (+ (* (- (- middle-width (* patterns-number period)) pattern-width) (/ (1+ dir) 2)) (abs (car pattern-x-extent) Wrap to proper formatting at about 80 characters. http://codereview.appspot.com/4182056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add dots to tocItemMarkup (issue4182056)
http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#newcode994 scm/define-markup-commands.scm:994: (define (nest-patterns pattern pattern-width distance count patterns) On 2011/02/16 14:26:45, Bertrand Bordage wrote: Ok. So, the input arguments will be count, pattern and distance (in this order) ? Or just count and pattern ? As a markup function, they should be count, pattern, distance. The order doesn't really matter. My preference would be pattern, count, distance. http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#newcode1021 scm/define-markup-commands.scm:1021: } On 2011/02/16 14:26:45, Bertrand Bordage wrote: Maybe a smaller example ? Shall we see more possibilities ? With a \override #'(line-width . 50) for example. I think what's here is sufficient. But I wouldn't be opposed to a change. http://codereview.appspot.com/4182056/diff/1/scm/define-markup-commands.scm#newcode1032 scm/define-markup-commands.scm:1032: (x-offset (+ (* (- (- middle-width (* patterns-number period)) pattern-width) (/ (1+ dir) 2)) (abs (car pattern-x-extent) On 2011/02/16 14:26:45, Bertrand Bordage wrote: (my Lord ;o) ) ^^ -- Yikes! http://codereview.appspot.com/4182056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add dots to tocItemMarkup (issue4182056)
Looks good. I had one comment. Feel free to add an entry to the changelog. It's done by editing the file Documentation/changes.tely Thanks, Carl http://codereview.appspot.com/4182056/diff/7001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/4182056/diff/7001/scm/define-markup-commands.scm#newcode3403 scm/define-markup-commands.scm:3403: (define-markup-command (pattern layout props pattern count space) Now that I see it written -- should there be a direction argument for pattern? Or at least should we document that it's a horizontal repeat? http://codereview.appspot.com/4182056/diff/7001/scm/define-markup-commands.scm#newcode3416 scm/define-markup-commands.scm:3416: (if (zero? i) Nicely done! http://codereview.appspot.com/4182056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc -- Clarify instructions on autobeam settings (issue4160048)
On 2011/02/15 07:15:48, Trevor Daniels wrote: http://codereview.appspot.com/4160048/diff/5001/Documentation/notation/rhythms.itely#newcode2211 Documentation/notation/rhythms.itely:2211: To avoid this problem, the time signature can be set in only one I still prefer should I don't want to say should because that can be interpreted as an all the time prescription. I've reworded it to say One way to avoid this problem is to set the time signature in only one staff. I've thought about when vs. whenever. I prefer when. I don't know if it's a UK vs. US thing, or just a Carl vs. Trevor thing. Thanks, Carl http://codereview.appspot.com/4160048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc -- Clarify instructions on autobeam settings (issue4160048)
Updated patch set posted for review. THanks, Carl http://codereview.appspot.com/4160048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add dots to tocItemMarkup (issue4172047)
I like the idea. I think to become part of the distribution it needs to support both postscript and svg. Thanks, Carl http://codereview.appspot.com/4172047/diff/1/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): http://codereview.appspot.com/4172047/diff/1/scm/define-markup-commands.scm#newcode1033 scm/define-markup-commands.scm:1033: #:with-dimensions (cons 0 middle-width) '(0 . 0) #:postscript (string-append 0.25 setlinewidth 1 setlinecap [0 1.5] 0 setdash (number-string offset) 0.12 moveto (number-string middle-width) 0 rlineto stroke) I don't think we should accept postscript markups as part of the distribution. They will break the SVG backend. We should ask for all markup functions that become part of the distribution to be supported in all backends, IMO. http://codereview.appspot.com/4172047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix #1490. (issue4186050)
LGTM Carl http://codereview.appspot.com/4186050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc -- Clarify instructions on autobeam settings (issue4160048)
On 2011/02/15 00:27:35, Felipe wrote:
http://codereview.appspot.com/4160048/diff/5001/Documentation/notation/rhythms.itely
File Documentation/notation/rhythms.itely (right):
http://codereview.appspot.com/4160048/diff/5001/Documentation/notation/rhythms.itely#newcode2207
Documentation/notation/rhythms.itely:2207: @code{Score} context. This
means
that a setting the time signature
Should it be This means that setting the time signature [...]?
Yes, thanks. Good catch!
http://codereview.appspot.com/4160048/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1229 Ensure space around prefatory matter (issue4187043)
LGTM. Thanks, Keith! Carl http://codereview.appspot.com/4187043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Doc -- Clarify instructions on autobeam settings (issue4160048)
Reviewers: ,
Message:
Mats identified some unexpected behavior with autobeam settings and time
signature setting.
http://thread.gmane.org/gmane.comp.gnu.lilypond.bugs/23117
This patch demonstrates the problem, describes the reason, and documents
two ways of avoiding the problem.
Please review this patch. I'm afraid I might have been a little too
much LM style in it, although I didn't talk through the code.
Thanks,
Carl
Description:
Doc -- Clarify instructions on autobeam settings
Multiple \time calls can revert custom autobeam settings. Clarify
in the documentation.
Please review this at http://codereview.appspot.com/4160048/
Affected files:
M Documentation/notation/rhythms.itely
Index: Documentation/notation/rhythms.itely
diff --git a/Documentation/notation/rhythms.itely
b/Documentation/notation/rhythms.itely
index
f0d518d99d1720d3de4b9ef9c82c12e95aaadae8..218f931a450ca72c62346f2b1fa0b90b52d0f03b
100644
--- a/Documentation/notation/rhythms.itely
+++ b/Documentation/notation/rhythms.itely
@@ -2085,12 +2085,92 @@ context to the default behavior.
\repeat unfold 6 { a8 }
@end lilypond
-These default automatic beaming settings for a time signature
+By default, the @code{Timing} translator is aliased to the
+@code{Score} context. This means that a setting the time signature
+in one staff will affect the beaming of the other staves as well.
+Thus, in the following example, the beaming will be at the standard
+beaming for 3/4 time, because the time signature setting in the
+second staff resets the beaming.
+
+@lilypond[quote,verbatim,relative=2]
+
+ \new Staff {
+\time 3/4
+\set Timing.baseMoment = #(ly:make-moment 1 8)
+\set Timing.beatStructure = #'(1 5)
+\repeat unfold 6 { a8 }
+ }
+ \new Staff {
+\time 3/4
+\repeat unfold 6 { a8 }
+ }
+
+@end lilypond
+
+In contrast, in this example, the beaming will follow the specified
+beaming rules, because the custom beaming in the second staff
+overrides the default beaming.
+
+@lilypond[quote,verbatim,relative=2]
+
+ \new Staff {
+\time 3/4
+\repeat unfold 6 { a8 }
+ }
+ \new Staff {
+\time 3/4
+\set Timing.baseMoment = #(ly:make-moment 1 8)
+\set Timing.beatStructure = #'(1 5)
+\repeat unfold 6 { a8 }
+ }
+
+@end lilypond
+
+To avoid this problem, the time signature can be set in only one
+staff.
+
+@lilypond[quote,verbatim,relative=2]
+
+ \new Staff {
+\time 3/4
+\set Timing.baseMoment = #(ly:make-moment 1 8)
+\set Timing.beatStructure = #'(1 5)
+\repeat unfold 6 { a8 }
+ }
+ \new Staff {
+\repeat unfold 6 { a8 }
+ }
+
+@end lilypond
+
+The default beam settings for the time signature can also be
+changed, so that when the time signature is set the desired
+behavior will be set. Changes in automatic beaming settings
+for a time signature are described in @ref{Time signature}.
+
+@lilypond[quote,verbatim,relative=2]
+
+ \new Staff {
+\overrideTimeSignatureSettings
+ #'(3 . 4) % timeSignatureFraction
+ #'(1 . 8) % baseMomentFraction
+ #'(1 5) % beatStructure
+ #'() % beamExceptions
+\time 3/4
+\repeat unfold 6 { a8 }
+ }
+ \new Staff {
+\time 3/4
+\repeat unfold 6 { a8 }
+ }
+
+@end lilypond
+
+
+The default automatic beaming settings for a time signature
are determined in @file{scm/time-signature-settings.scm}.
-The automatic beaming settings for a time signature can be changed
-as described in @ref{Time signature}.
-Most automatic beaming settings for a time signature contain an
+Many automatic beaming settings for a time signature contain an
entry for @code{beamExceptions}. For example, 4/4 time tries to
beam the measure in two if there are only eighth notes. The
@code{beamExceptions} rule can override the @code{beatStructure} setting
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1035 -- Add context property for negative frets (issue4056041)
On 2011/02/02 23:55:59, Neil Puttock wrote: LGTM. Just needs rebasing (I assume you don't want to delete tablature-dot-placement.ly) Done http://codereview.appspot.com/4056041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
On 2011/01/31 19:36:16, Keith wrote: On 2011/01/31 11:20:39, hanwenn wrote: * this hardcodes 0.1 in several places. Make a property, so we can override this Agreed in principle; the relevant property extra-spacing-height should absorb these magic numbers, but I am not willing to do so in this patch. The 0.1s were in the previous stable release; commit ee0488 tried to remove them, but had side effects. This patch reverts ee0488 (thus bringing back the ugly 0.1s) then adjusts a few existing properties to have the desired effects of ee0488. Since the 0.1s are all part of a Separation_item, what if we defined a new grob-property default-separation, and set the value to 0.1 for NonMusicalPaperColumn, NoteColumn, and PaperColumn, since these are the grobs having separation_item_interface? We could then get the value of default-separation from the paper column, and use that value throughout separation-item.cc I agree that it seems like a bit of a pain to do this when you're just wanting to revert a patch that now appears inappropriate. But if this property were added, no recompilation would be required in order to resolve the issues you've been fighting with here. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
This patch solves Neil's problem with clef spacing. LGTM. Carl http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix critical regressions to lyric spacing (issue4061043)
On 2011/01/25 13:57:32, Graham Percival wrote: Pushed. cb03a19174fd9245008176716742e1a2eb3a0b93 Thanks, Carl http://codereview.appspot.com/4061043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Font: Including the jazz font for chords (issue3972048)
I can't comment on the appropriateness of the makefile stuff. Everything else looks good to me. Does this match up with the Gonville font stuff that was included earlier on the LSR? We probably ought to put Gonville and lilyjazzchords in the same parent directory. http://codereview.appspot.com/3972048/diff/1/input/regression/chords-jazz-font.ly File input/regression/chords-jazz-font.ly (right): http://codereview.appspot.com/3972048/diff/1/input/regression/chords-jazz-font.ly#newcode4 input/regression/chords-jazz-font.ly:4: texidoc = A contemporary jazz font style for chords is should be ChordNames, not chords http://codereview.appspot.com/3972048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: mensural notation improvements (issue3797046)
On 2011/01/24 21:27:35, benko.pal wrote: new patchset up. please advise me about regression tests: can I modify ligatures within mensural-ligatures.ly? if not, can I add new ones to the same file? Ancient music has been abandoned by developers for a number of years. IMO, you may do whatever you think makes the most sense as you try to bring ancient music back into the mainstream of LilyPond development. Thanks, Carl http://codereview.appspot.com/3797046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix critical regressions to lyric spacing (issue4061043)
On 2011/01/24 16:33:22, Graham Percival wrote: Almost there. Could you run makelsr and then don't forget to do git add Documentation/snippets/*.ly Done, and new patch set uploaded. Thanks, Carl http://codereview.appspot.com/4061043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix 1120 in a way to avoid issues 1472, 1474 (issue4095041)
This patch also resolves the problem in issue 1229 of notes extending beyond the right-hand bar line. Adding additional extra-spacing-height to the TimeSignature grob resolves the 1229 issue of overlapping the time signature. This patch seems to have some very good benefits. http://codereview.appspot.com/4095041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix critical regressions to lyric spacing (issue4061043)
On 2011/01/23 09:09:09, Trevor Daniels wrote: I think the templates would be improved with system-system-spacing #'basic-distance = #20 added to \paper. Otherwise the bass lyrics will be too close to the soprano lyrics on the following system. Done. I also added top-system-spacing and last-bottom-spacing to take care of the first and last systems of the score. http://codereview.appspot.com/4061043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix critical regressions to lyric spacing (issue4061043)
I've responded to the comments in detail below.
Graham, relative to your comments on issues 1483 and 1486, both are
addressed in this patch.
The templates now have the lyrics attached properly to the staves, as do
the examples in the NR. This takes care of 1483.
The templates also have sufficient space for the lyrics at the top and
bottom of the score. This takes care of 1486.
For good measure, there is a snippet in the docs that demonstrates the
way to get the same spacing behavior as in 2.12.
As far as I can see, all of the problems associated with both issues are
resolved by this patch.
http://codereview.appspot.com/4061043/diff/1/Documentation/changes.tely
File Documentation/changes.tely (right):
http://codereview.appspot.com/4061043/diff/1/Documentation/changes.tely#newcode70
Documentation/changes.tely:70: Lyrics above a staff must have their
@code{staff-affinity} set to
I think it is appropriate, because people who used the old vocal
templates will suddenly find that the soprano lyrics will be attached to
the previous system, and the tenor lyrics will be attached to the
womens' staff. One rational place for them to go to investigate is to
look at CHANGES.
What if, instead of a snippet, it referenced the appropriate section in
the Notation Reference?
I've gone ahead and added the reference; I'll change if you think it
necessary.
http://codereview.appspot.com/4061043/diff/1/Documentation/notation/vocal.itely
File Documentation/notation/vocal.itely (right):
http://codereview.appspot.com/4061043/diff/1/Documentation/notation/vocal.itely#newcode1021
Documentation/notation/vocal.itely:1021: % lyrics above a stave should
have this override
Changed stave to staff
http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/lyrics-old-spacing-settings.ly
File Documentation/snippets/new/lyrics-old-spacing-settings.ly (right):
http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/lyrics-old-spacing-settings.ly#newcode19
Documentation/snippets/new/lyrics-old-spacing-settings.ly:19: % VERSE
ONE
On 2011/01/23 14:20:35, Graham Percival wrote:
This differs from our normal indentation style... and in particular,
it
conflicts with the two-space indentation style that you used above in
the global
section... but meh, go ahead.
No, I'll fix it. This is what happens when I copy somebody else's code
and don't review it carefully. I made sure it worked, and that we had
the right header stuff, but didn't pay a lot of attention to the coding
style.
Thanks for the catch.
I've fixed the indentation, along with removing the unnecessary { s1 }
from each \new Lyrics construct.
http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly
File
Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly
(right):
http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly#newcode6
Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly:6:
%% Translation of GIT committish:
fa19277d20f8ab0397c560eb0e7b814bd804ecec
On 2011/01/23 22:55:54, Neil Puttock wrote:
remove all translation stuff
Done.
http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template.ly
File Documentation/snippets/new/vocal-ensemble-template.ly (right):
http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template.ly#newcode6
Documentation/snippets/new/vocal-ensemble-template.ly:6: %% Translation
of GIT committish: fa19277d20f8ab0397c560eb0e7b814bd804ecec
On 2011/01/23 22:55:54, Neil Puttock wrote:
remove
Done.
http://codereview.appspot.com/4061043/diff/1/input/regression/baerenreiter-sarabande.ly
File input/regression/baerenreiter-sarabande.ly (right):
http://codereview.appspot.com/4061043/diff/1/input/regression/baerenreiter-sarabande.ly#newcode174
input/regression/baerenreiter-sarabande.ly:174:
obsolete-between-system-space = 25\mm system-system-spacing
#'basic-distance = #(/ obsolete-between-system-space staff-space)
score-system-spacing #'basic-distance = #(/
obsolete-between-system-space staff-space)
On 2011/01/23 14:20:35, Graham Percival wrote:
woah, what happened here? Could we get some linebreaks? (is this some
weird
osx-linebreaks+git malfunction?)
No, this is exactly the way convert-ly was intended to work for this
change, because in some cases the lines were commented.
This was discussed before the convert-ly rule was pushed.
The convert-ly rule was pushed in commit
4921d3518f4961abcfaf9ea243bec33efc943574 IIUC. The author was Keith
O'Hara and the committer was Graham Percival ;-)
But I will do a manual fix and make it the way it would be if we were
writing it from scratch.
Done.
http://codereview.appspot.com/4061043/diff/1/input/regression/mozart-hrn-3.ly
File input/regression/mozart-hrn-3.ly (right):
Re: Better support for beat slashes (multi-slash mixed duration). (issue212048)
LGTM. Carl http://codereview.appspot.com/212048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: mensural notation improvements (issue3797046)
Let me start by saying I know *nothing* about mensural notation.
The code looks good to me.
I found only one real issue:
LilyPond coding standards for C++ say that if there is only one
statement in an if clause, we omit {} around that clause.
I also had a question (and it probably doesn't matter much). When I've
written font glyphs that are sometimes solid and sometimes hollow, I
always calculate both the inner and outer paths. And then when I create
the glyph I only use the paths that are actually needed. In your code,
you didn't create the path unless it was needed. It probably makes no
difference at all, but I'd like to hear from the font gurus if there is
a preference.
My take was we only make the fonts at install, so the code doesn't need
to be optimized for speed, so I optimized for readability, which in my
mind, meant not putting the inner path inside a conditional.
Great work!
Thanks,
Carl
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc
File lily/mensural-ligature-engraver.cc (right):
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode277
lily/mensural-ligature-engraver.cc:277: {
Remove unneeded {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode315
lily/mensural-ligature-engraver.cc:315: prev_primitive-set_property
(add-join, ly_bool2scm (true));
Remove unneeded {} from previous line and next line
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode380
lily/mensural-ligature-engraver.cc:380: {
Why put the {} in this case?
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode448
lily/mensural-ligature-engraver.cc:448: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode453
lily/mensural-ligature-engraver.cc:453: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode455
lily/mensural-ligature-engraver.cc:455: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature-engraver.cc#newcode460
lily/mensural-ligature-engraver.cc:460: {
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc
File lily/mensural-ligature.cc (right):
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc#newcode101
lily/mensural-ligature.cc:101: {
remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc#newcode144
lily/mensural-ligature.cc:144: flexa_width = robust_scm2double
(me-get_property (flexa-width), 2.0)
Remove {}
http://codereview.appspot.com/3797046/diff/1/lily/mensural-ligature.cc#newcode221
lily/mensural-ligature.cc:221: {
Remove {}
http://codereview.appspot.com/3797046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix issue 1376 ambitus two accidentals. (issue4099044)
LGTM. Carl http://codereview.appspot.com/4099044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixing issue 37 with extra position callback (issue3928041)
New patch set uploaded. http://codereview.appspot.com/3928041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Add new merge-function.ly file for rests (issue4005046)
Great start, James!
Getting this far is more than half the battle.
Are you up for the next round of changes now?
Thanks,
Carl
http://codereview.appspot.com/4005046/diff/1/ly/declarations-init.ly
File ly/declarations-init.ly (right):
http://codereview.appspot.com/4005046/diff/1/ly/declarations-init.ly#newcode31
ly/declarations-init.ly:31: \include merge-function.ly
Once we move ly/merge-function.ly to scm/merge-rests.scm, this will need
to move to scm/lily.scm as part of the definition of init-scheme-files
(see lines 393 and thereabouts.
http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly
File ly/merge-function.ly (right):
http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode1
ly/merge-function.ly:1: %{
Once you have moved the commands out to ly/property-init.ly and
ly/engraver-init.ly, there's nothing but scheme code in this file, so it
should become a scheme file.
Let's call it scm/merge-rests.scm.
Put a LilyPond copyright statement at the top of it (use one from any
other .scm file), with Wilbert Berendsen and James Lowe as the copyright
holders.
http://codereview.appspot.com/4005046/diff/1/ly/merge-function.ly#newcode118
ly/merge-function.ly:118: mergeRestsOn = {
You have properly defined mergeRestsOn and mergeRestsOff in
ly/property-init.ly, so they should be removed here.
mergeRests should be moved to ly/engraver-init.ly (and removed here).
http://codereview.appspot.com/4005046/diff/1/ly/property-init.ly
File ly/property-init.ly (right):
http://codereview.appspot.com/4005046/diff/1/ly/property-init.ly#newcode282
ly/property-init.ly:282: mergeRests = \layout {
mergeRests should have the \layout{} block removed, so that it
can be included in anybody's layout block.
It should be in ly/engraver-init.ly.
http://codereview.appspot.com/4005046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix critical regressions to lyric spacing (issue4061043)
Reviewers: , Message: Here is a potential patch for fixing the lyric spacing regressions. It updates the documentation to clarify what is needed to make lyrics behave as desired. It also adds a snippet that demonstrates how to achieve the old spacing behavior. Please respond with any comments. Thanks, Carl Description: Fix critical regressions to lyric spacing Cleaned up documentation Add snippet that shows how to get old spacing behavior if desired Add old spacing snippet to Docs; run makelsr.py Update changes.tely Modify vocal templates Run convert-ly on everything Please review this at http://codereview.appspot.com/4061043/ Affected files: M Documentation/changes.tely M Documentation/notation/vocal.itely A Documentation/snippets/new/lyrics-old-spacing-settings.ly A Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly A Documentation/snippets/new/vocal-ensemble-template.ly M Documentation/snippets/vocal-ensemble-template-with-automatic-piano-reduction.ly M Documentation/snippets/vocal-ensemble-template.ly M Documentation/snippets/vocal-music.snippet-list M input/regression/baerenreiter-sarabande.ly M input/regression/mozart-hrn-3.ly M input/regression/page-spacing.ly M input/regression/page-top-space.ly ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1035 -- Add context property for negative frets (issue4056041)
Thanks, Marc. Good suggestion. http://codereview.appspot.com/4056041/diff/1/scm/translation-functions.scm File scm/translation-functions.scm (right): http://codereview.appspot.com/4056041/diff/1/scm/translation-functions.scm#newcode394 scm/translation-functions.scm:394: ((eq? handle-negative 'recalculate) On 2011/01/18 19:42:41, marc wrote: In case of a resulting negative fret there should be at least a warning that lilypond recalculates the fret on another string IMHO. Thanks for the idea. I agree. Done. http://codereview.appspot.com/4056041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix Issue 1035 -- Add context property for negative frets (issue4056041)
Reviewers: marc, Message: I've posted a patch for fixing issue 1035, by giving the user control over what to do with negative fret numbers demanded by an assigned string. The default behavior is to recalculate the note and put it in the tablature or fretboard as if it had not had a string assigned. It can also be set to ignore the note (leaving it out of the tablature or fretboard) or to include the note (giving a negative fret number in the tablature or fretboard. Please review. Thanks, Carl Description: Fix Issue 1035 -- Add context property for negative frets Add handleNegativeFrets, with possibilities of 'ignore, 'recalculate, 'include Reorder functions in scm/translation-functions.scm to put context in scope of calculate-frets-and-strings Add regression test. Please review this at http://codereview.appspot.com/4056041/ Affected files: A input/regression/tablature-negative-fret.ly M ly/engraver-init.ly M scm/define-context-properties.scm M scm/translation-functions.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Implement compound time signatures (issue3992042)
LGTM. Thanks, Carl http://codereview.appspot.com/3992042/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 39 by raising stems (issue3934041)
Looks very good to me. I'd like to see the property name changed to extra-stem-length. Thanks, Carl http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/3934041/diff/11001/lily/stem.cc#newcode322 lily/stem.cc:322: Real extra_raise_tip = scm_to_double (me-get_property (extra-raise-tip)); I'd prefer extra-stem-length. I realize that when the flags are down we already have an adjustment. But I think that extra-stem-length is a better description than extra-raise-tip. http://codereview.appspot.com/3934041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fixing issue 37 with extra position callback (issue3928041)
Reviewers: MikeSol,
Message:
Looks great, Mike!
You have some code indentation issues that don't match our style. Other
than that, I think it's good to go.
Of course, we do need a regression test as well.
Thanks,
Carl
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode4
lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen
Nienhuys han...@xs4all.nl
Copyright should be 2011 Mike Solomon.
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode48
lily/beam-collision-engraver.cc:48: {
brace not needed
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode55
lily/beam-collision-engraver.cc:55: {
remove {
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode57
lily/beam-collision-engraver.cc:57: {
remove {
http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode83
lily/beam-collision-engraver.cc:83: {
remove {
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode944
lily/beam.cc:944: {
indent { 2 spaces
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode962
lily/beam.cc:962: {
indent 2 spaces
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode967
lily/beam.cc:967: magic = -2.0;
{ on separate line, indented 2 spaces
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode972
lily/beam.cc:972: {
indent
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode992
lily/beam.cc:992: {
indentation
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode997
lily/beam.cc:997: } else {
indentation
http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode1008
lily/beam.cc:1008: } else
else on its own line, same indentation as if
http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly
File ly/engraver-init.ly (right):
http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly#newcode68
ly/engraver-init.ly:68: \consists Beam_collision_engraver
Also add to TabStaff
Description:
Fixing issue 37 with extra position callback
Adds beam collision engraver
Goodbye whitespace
Please review this at http://codereview.appspot.com/3928041/
Affected files:
A lily/beam-collision-engraver.cc
M lily/beam.cc
M lily/include/beam.hh
M ly/engraver-init.ly
M scm/define-grobs.scm
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixing issue 37 with extra position callback (issue3928041)
Latest patch set uploaded with full side-by-side-diffs. Thanks, Carl http://codereview.appspot.com/3928041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixing issue 37 with extra position callback (issue3928041)
LGTM.
Don't forget to fix your copyright on beam-collision-engraver.
One set of braces to be removed.
Thanks,
Carl
http://codereview.appspot.com/3928041/diff/17001/lily/beam-collision-engraver.cc
File lily/beam-collision-engraver.cc (right):
http://codereview.appspot.com/3928041/diff/17001/lily/beam-collision-engraver.cc#newcode4
lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen
Nienhuys han...@xs4all.nl
Copyright 2011 Mike Solomon
http://codereview.appspot.com/3928041/diff/17001/lily/beam.cc
File lily/beam.cc (right):
http://codereview.appspot.com/3928041/diff/17001/lily/beam.cc#newcode969
lily/beam.cc:969: {
No brackets here
http://codereview.appspot.com/3928041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixing issue 37 with extra position callback (issue3928041)
New patches pushed. Thanks, Carl http://codereview.appspot.com/3928041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fixes issue 39 by raising stems (issue3934041)
LGTM. Carl http://codereview.appspot.com/3934041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
I've added comments about the need for the horizontal-padding in the distance call to be used with System grobs, and I've moved the skyline-horizontal-padding from an override to a default value for the System grob. I think this should make everything work right out of the box now, without requiring overrides. Any further comments? Thanks, Carl http://codereview.appspot.com/3832046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
Thanks for the feedback. I've responded to each of the suggestions
you've given me.
Carl
http://codereview.appspot.com/3832046/diff/27001/input/regression/skyline-horizontal-padding.ly
File input/regression/skyline-horizontal-padding.ly (right):
http://codereview.appspot.com/3832046/diff/27001/input/regression/skyline-horizontal-padding.ly#newcode12
input/regression/skyline-horizontal-padding.ly:12: \score {
On 2011/01/07 21:59:41, Neil Puttock wrote:
needs a \book { } block to prevent lilypond-book hijacking the
system-system
spacing
Done.
http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc
File lily/skyline.cc (right):
http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode391
lily/skyline.cc:391: {
It worked, once I changed boxes from a vector to a list.
Thanks for the insight!
http://codereview.appspot.com/3832046/diff/27001/lily/skyline.cc#newcode419
lily/skyline.cc:419: Skyline padded_skyline = Skyline (boxes,
horizon_padding, a, UP);
On 2011/01/07 16:47:27, joeneeman wrote:
This should have X_AXIS instead of a, because you put the horizon
axis into
the X_AXIS of your boxes.
Done.
http://codereview.appspot.com/3832046/diff/27001/scm/define-grobs.scm
File scm/define-grobs.scm (right):
http://codereview.appspot.com/3832046/diff/27001/scm/define-grobs.scm#newcode1940
scm/define-grobs.scm:1940: (skyline-horizontal-padding . 2)
On 2011/01/07 23:53:34, Keith wrote:
too big for a default. I suggest 0.5.
A horizontal space of _4-times_ this value is maintained between
potential
interleavers, because the diagonal starts one padding a way from the
grob, the
vertical is two paddings away, and grobs from each system are padded.
I realize that a value of 1.5 or greater is required to affect your
regression
test, but an \override in the score would make a more useful
regression test
anyway.
The only musical example I have seen where I would want the padding is
the
regtest les-nereides.ly, which greatly improved with
horizontal-padding 0.5.
Done. 0.5 also works fine with stem-length-estimation.ly.
http://codereview.appspot.com/3832046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
On 2011/01/07 21:41:51, Neil Puttock wrote: Hi Carl, Do we have to set a default for skyline-horizontal-padding? It has a detrimental effect on some of the regtests (particularly stem-length-estimation.ly). I've set the default to 0.5, in accordance with Keith's suggestions. It leaves stem-length-estimation.ly alone now. http://codereview.appspot.com/3832046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
Thanks, Keith for identifying the problem with bar numbers at the beginning of the line. I've fixed that problem, and I'm more confident in the logic of the box extraction and padded skyline creation. I've modified the regression test so it has 3 systems, and thus would catch the problem that Keith identified with the bar numbers. Please review. Thanks, Carl http://codereview.appspot.com/3832046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
I've made the changes, and now the patch actually works. Thanks all for your comments! Carl http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc File lily/page-layout-problem.cc (right): http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#newcode266 lily/page-layout-problem.cc:266: Page_layout_problem::append_prob (Prob *prob, Spring const spring, Real padding) On 2011/01/02 23:09:02, Keith wrote: This function is not called for either of our test cases. Yes, I guess this is called for adding a prob that is not a system. So I'm removing the code here. http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#newcode274 lily/page-layout-problem.cc:274: minimum_distance = (*sky)[UP].distance (bottom_skyline_, scm_to_double (prob-get_property (skyline-horizontal-padding))); On 2011/01/02 05:19:58, joeneeman wrote: robust_scm2double is probably better removed (see above) http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc File lily/skyline.cc (right): http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396 lily/skyline.cc:396: if ((i-slope_ == 0) !isinf (i-y_intercept_) i-y_intercept_ = 0) OK, I got it to work properly now. I have a better algorithm for handling the bottom of the extracted box. http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398 lily/skyline.cc:398: Interval (i-y_intercept_ 0 ? i-y_intercept_ : 0 , i-y_intercept_))); Now I handle the y_intercept_ 0 problem properly here. http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399 lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding, a, (Direction) 1); Using sky_ now. http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498 lily/skyline.cc:498: const Skyline *padded_other = other; On 2011/01/02 05:19:58, joeneeman wrote: I'm not sure, but I think Skyline const * is more consistent with the rest of the code. Done. http://codereview.appspot.com/3832046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
On 2011/01/04 01:46:45, Keith wrote: For a 2-staff system (with non-protruding bass clefs to make the math easier) the patch computes minimum_distance 3.05, 7.33, 29.41, 29.38 as it adds four systems to a page. Can you send me a test file so I can check it out? Thanks, Carl http://codereview.appspot.com/3832046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 1290 (issue3832046)
Thanks for all the comments.
Keith has identified the correct place to put include the system
horizontal padding, and it now works properly.
New patch set coming shortly.
Carl
http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-horizontal-padding.ly
File input/regression/skyline-horizontal-padding.ly (right):
http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-horizontal-padding.ly#newcode13
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 {
c'''-1 e'''-3 g'''-5 c' c,-1 e,-3 g,-5 c' }
On 2011/01/02 09:01:11, Trevor Daniels wrote:
Can not a more precise and shorter test using \break be devised? With
this code
interleaving occurs only once right at the bottom even without the
override.
Yes, I'll fix it in the next patch.
http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-horizontal-padding.ly#newcode13
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 {
c'''-1 e'''-3 g'''-5 c' c,-1 e,-3 g,-5 c' }
On 2011/01/02 23:09:02, Keith wrote:
On 2011/01/02 09:01:11, Trevor Daniels wrote:
Can not a more precise and shorter test
I had a test file for 1290, and related, that shows what does and does
not get
displaced by the various skylines:
#(ly:set-option 'debug-skylines #t)
\score {
{
\repeat unfold 2 {
\mark mark
a,2_fa gisis'''!^gg |
\mark m
b,4 a'_tx c'2 \break
}
} \layout {
ragged-right = ##t
indent = #0
\context {
\Score
\override System #'skyline-horizontal-padding = #0.0
\override TimeSignature #'stencil = ##f
}
}
}
Thanks, Keith. I'll look into this as a regression test.
http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):
http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#newcode204
lily/page-layout-problem.cc:204: Real minimum_distance =
up_skyline.distance (bottom_skyline_) + padding;
On 2011/01/02 23:09:02, Keith wrote:
We need to call your distance here, no?
Yes, thanks for finding this! You saved me the time of doing it!
http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):
http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i-slope_ == 0) !isinf (i-y_intercept_)
i-y_intercept_ = 0)
On 2011/01/02 05:19:58, joeneeman wrote:
Why restrict to y_intercept_ 0?
Because if I have a y intercept that is less than zero, and create a
box with that y intercept, the padded skyline comes out with all zero
heights. I don't know why it works that way, but it does.
I tried several things to get it to behave differently, but was
unsuccessful. And I don't think that a negative skyline will ever be
limiting in inter-system spacing, so I didn't feel that it was a
problem.
http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398
lily/skyline.cc:398: Interval (i-y_intercept_ 0 ? i-y_intercept_ : 0
, i-y_intercept_)));
On 2011/01/02 05:19:58, joeneeman wrote:
...and if you do restrict to y_intercept_ 0, then this is redundant.
Yes, this is redundant, left over from one of my tries to properly deal
with negative skylines.
http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399
lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding,
a, (Direction) 1);
On 2011/01/02 05:19:58, joeneeman wrote:
pad_sky instead of padSky
UP instead of (Direction) 1
and perhaps a comment about why you need to use UP instead of sky_
(which would
seem intuitive)
Yes, thank you. These are good suggestions. I will fix them.
http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498
lily/skyline.cc:498: const Skyline *padded_other = other;
On 2011/01/02 05:19:58, joeneeman wrote:
I'm not sure, but I think Skyline const * is more consistent with
the rest of
the code.
Thanks, I'll try to see if that works.
http://codereview.appspot.com/3832046/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix Issue 1290 (issue3832046)
Reviewers: ,
Message:
Here is a patch to fix issue 1290.
It works, but it may need to be cleaned up. I'm not sure the code is as
elegant as it could be. I'm not really comfortable with all of the C++
syntax used in lilypond.
Please review it carefully, and let me know how it can be improved.
Thanks,
Carl
Description:
Fix Issue 1290
Create an optional horizontal-padding parameter to Skyline::distance
When horizontal-padding is not zero, padding is added to both systems
when calculating the distance.
Get the System's skyline-horizontal-padding property, and pass
it to distance () in page-layout-problem.cc
Add regression test for this feature.
Please review this at http://codereview.appspot.com/3832046/
Affected files:
A input/regression/skyline-horizontal-padding.ly
M lily/include/skyline.hh
M lily/page-layout-problem.cc
M lily/skyline.cc
Index: input/regression/skyline-horizontal-padding.ly
diff --git a/input/regression/skyline-horizontal-padding.ly
b/input/regression/skyline-horizontal-padding.ly
new file mode 100644
index
..c3e96211304a824c5d8304c2faf43d1b638aa24d
--- /dev/null
+++ b/input/regression/skyline-horizontal-padding.ly
@@ -0,0 +1,20 @@
+\version 2.13.45
+
+\header {
+ texidoc =
+The skyline-horizontal-padding property can be set for System
+in order to keep systems from being spaced too closely together.
+In this example, the low notes from a system should not be
+interleaved with the high notes from the next system.
+
+}
+
+\score {
+\repeat unfold 80 { c'''-1 e'''-3 g'''-5 c' c,-1 e,-3 g,-5 c' }
+ \layout {
+\context {
+ \Score
+ \override System #'skyline-horizontal-padding = #2
+}
+ }
+}
Index: lily/include/skyline.hh
diff --git a/lily/include/skyline.hh b/lily/include/skyline.hh
index
a11b21dde6ce27d06a3e54b177e32940da779589..93b6e1155605d4c03fb8d8b566c40bf8ce80bd6e
100644
--- a/lily/include/skyline.hh
+++ b/lily/include/skyline.hh
@@ -53,7 +53,7 @@ class Skyline
private:
listBuilding buildings_;
Direction sky_;
-
+
void internal_merge_skyline (listBuilding*, listBuilding*,
listBuilding *const result);
listBuilding internal_build_skyline (listBox*, Real, Axis,
Direction);
@@ -63,10 +63,11 @@ private:
public:
Skyline ();
Skyline (Skyline const src);
+ Skyline (Skyline const src, Real horizon_padding, Axis a);
Skyline (Direction sky);
Skyline (vectorBox const bldgs, Real horizon_padding, Axis a,
Direction sky);
Skyline (Box const b, Real horizon_padding, Axis a, Direction sky);
-
+
vectorOffset to_points (Axis) const;
void merge (Skyline const );
void insert (Box const , Real horizon_padding, Axis);
@@ -74,7 +75,7 @@ public:
void print_points () const;
void raise (Real);
void shift (Real);
- Real distance (Skyline const ) const;
+ Real distance (Skyline const , Real horizon_padding = 0) const;
Real height (Real airplane) const;
Real max_height () const;
void set_minimum_height (Real height);
Index: lily/page-layout-problem.cc
diff --git a/lily/page-layout-problem.cc b/lily/page-layout-problem.cc
index
673278f69c6fc9490ab118d406610531064b6d9e..c30f43d84a40464278e078313fe34ae0b3d89a9d
100644
--- a/lily/page-layout-problem.cc
+++ b/lily/page-layout-problem.cc
@@ -271,7 +271,7 @@ Page_layout_problem::append_prob (Prob *prob, Spring
const spring, Real padding
if (sky)
{
- minimum_distance = (*sky)[UP].distance (bottom_skyline_);
+ minimum_distance = (*sky)[UP].distance (bottom_skyline_,
scm_to_double (prob-get_property (skyline-horizontal-padding)));
bottom_skyline_ = (*sky)[DOWN];
}
else if (Stencil *sten = unsmob_stencil (prob-get_property (stencil)))
Index: lily/skyline.cc
diff --git a/lily/skyline.cc b/lily/skyline.cc
index
d8105e669b910eaadc3fe8e34286ae9d613c8e50..5283cd25cee6f1bce634269eb0b5fa33e48acd90
100644
--- a/lily/skyline.cc
+++ b/lily/skyline.cc
@@ -119,7 +119,7 @@ Building::precompute (Real start, Real start_height,
Real end_height, Real end)
y_intercept_ = start_height - slope_ * start;
}
-Real
+Real
Building::height (Real x) const
{
return isinf (x) ? y_intercept_ : slope_*x + y_intercept_;
@@ -248,7 +248,7 @@ single_skyline (Building b, Real start, Real
horizon_padding, listBuilding *co
-infinity_f, infinity_f));
if (sloped_neighbours)
ret-push_front (b.sloped_neighbour (start, horizon_padding, RIGHT));
-
+
if (b.end_ start + EPS)
ret-push_front (b);
@@ -367,7 +367,7 @@ Skyline::Skyline ()
Skyline::Skyline (Skyline const src)
{
sky_ = src.sky_;
-
+
/* doesn't a list's copy constructor do this? -- jneem */
for (listBuilding::const_iterator i = src.buildings_.begin ();
i != src.buildings_.end (); i++)
@@ -383,6 +383,26 @@ Skyline::Skyline (Direction sky)
}
/*
+ build padded skyline from an existing skyline with padding
+ added to it.
+*/
+
Re: Change stringTunings entries from semitones to pitches (issue3842041)
On 2010/12/29 05:18:07, Keith wrote: Agreed. My earlier 'arbitrary' was a mental slip. I was thinking the choice was sensible, but even if it were arbitrary I would be scared of change. The order for the chord entry was requested by the users. Chords are generally entered lowest note first. Yes, but were the users right? I would have asked for that order of entry, too, but would have changed my mind for fear of bugs when reminded that the old way to specify tuning was in order of string numbering. (Bugs made by me in my .ly files, that is) If you found this reversal not too error-prone, then maybe the users were right. As far as old files go, the convert-ly rule automatically does the conversion, so you don't even need to think about it. As far as new files go, I think the user can learn whatever syntax is necessary. Thanks, Carl http://codereview.appspot.com/3842041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Change stringTunings entries from semitones to pitches (issue3842041)
On 2010/12/29 05:18:07, Keith wrote:
ly/string-tunings-init.ly:43: (make-music 'SequentialMusic 'void #t)))
We need to save the string tuning in a Scheme variable...
But if it is possible to set the variable as you do now, and then
return a
PropertySet instead of the void event,
(begin
(chord-tuning parser tuning chord)
(context-spec-music
(make-property-set 'stringTunings tuning )
'TabStaff)
)
then we could
\new TabStaff {
\setStringTuning #'a-fiddle-tuning g d' a' d''
I'll add a \setStringTuning function. I can see its utility.
Thanks,
Carl
http://codereview.appspot.com/3842041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Change stringTunings entries from semitones to pitches (issue3842041)
I've responded to all the commandments and put up a new patch.
Thanks for all of your input.
Please review.
Carl
http://codereview.appspot.com/3842041/diff/6001/Documentation/notation/fretted-strings.itely
File Documentation/notation/fretted-strings.itely (right):
http://codereview.appspot.com/3842041/diff/6001/Documentation/notation/fretted-strings.itely#newcode524
Documentation/notation/fretted-strings.itely:524: chord construct must
be in absolute note mode, and the string
On 2010/12/29 09:13:24, pkx166h wrote:
Hello, referred to 'Absolute octave entry' in the NR, not 'Absoulte
note mode'.
The heading is Absolute octave entry. The mode is absolute octave
mode or absolute mode -- see Relative octave entry.
I've changed to absolute octave mode.
Could we also have an @ref{} here too please (and thus an addition to
the
@seealso).
Done.
http://codereview.appspot.com/3842041/diff/6001/ly/string-tunings-init.ly
File ly/string-tunings-init.ly (right):
http://codereview.appspot.com/3842041/diff/6001/ly/string-tunings-init.ly#newcode43
ly/string-tunings-init.ly:43: (make-music 'SequentialMusic 'void #t)))
I've added a new music function contextStringTunings (the name is
negotiable) that does the property set as well.
It sets stringTunings for both TabStaff and FretBoards.
I don't want to call it setStringTuning because it will be confused with
\set stringTunings.
http://codereview.appspot.com/3842041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Change stringTunings entries from semitones to pitches (issue3842041)
On 2010/12/29 02:25:44, Keith wrote: I'm not a programmer, but accustomed to doing code review as a systems engineer. I very much appreciate the review. Thanks! Change stringTunings entries from semitones to pitches This lays the foundation for creating a TabKey grob Presumably the idea is to store the correct spelling of the note in the future TabKey, should anyone tune a string to des or cis. Tell us if there is a less-obvious reason. That is exactly the reason. I *thought* that this would make \transpose, when applied to the whole score, shift the tuning as well. When I tried your patch, though, only the music was transposed and not the tuning. I think that is fine; I'm just jiggling our brains looking for side-effects. StringTunings is a characteristic of an instrument, not a characteristic of music. So I don't think that transpose should affect StringTunings. I'll respond to your detailed comments in the body. Thanks, Carl http://codereview.appspot.com/3842041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
