Re: Add chordChanges capability to FretBoard grobs

http://codereview.appspot.com/24044/diff/1/2 File input/new/fretboard-chordchanges.ly (right): http://codereview.appspot.com/24044/diff/1/2#newcode8 Line 8: \version "2.13.0" "2.13.1" http://codereview.appspot.com/24044/diff/1/3 File input/regression/fretboard-chordchanges.ly (right): http://c

Re: Fix key signatures with accidentals in specific octave.

Reviewers: joeneeman, Message: On 2009/03/24 23:06:42, joeneeman wrote: lgtm, except for the description "move check_pitch_against_signature () to SCM," since ly:check-pitch-against-signature is still implemented in C++. Erk. I misinterpreted that to mean exporting check_pitch_against_signa

Re: Fix #670: Chained trills

Reviewers: joeneeman, Message: On 2009/04/08 20:37:56, joeneeman wrote: lgtm Cheers. I've amended trill-spanner-auto-stop.ly since there's a rogue space after @code. Description: Fix #670: Chained trills - if trill spanner isn't stopped using \stopTrillSpan, make next start-span right bound.

Re: Move left-broken line-spanner check to callback.

Reviewers: joeneeman, Message: On 2009/04/08 20:42:51, joeneeman wrote: http://codereview.appspot.com/32148/diff/1/7#newcode426 Line 426: Spanner::after_line_breaking (SCM grob) Can you think of a name that describes what the function actually does? Like Spanner::suicide_if_spanned_time_is_emp

Implement framework for post-fix text (de)cresc spanners

I never realized this would be so simple, but it strikes me as a bit of a hack. In your sample text dynamic spanners, there seems to be an element of redundancy in the event properties; you could easily junk '(de)crescendoSpanner and '(de)crescendoText, using 'text only to trigger the change, tho

Re: Move left-broken line-spanner check to callback.

On 2009/04/11 19:22:54, joeneeman wrote: I prefer the second one. OK, ly:spanner::kill-zero-spanned-time it is. I've added a convert rule just in case anybody's using ly:hairpin::after-line-breaking. http://codereview.appspot.com/32148 ___ lilypo

Re: Fix key signatures with accidentals in specific octave.

On 2009/03/24 23:06:42, joeneeman wrote: lgtm, except for the description "move check_pitch_against_signature () to SCM," since ly:check-pitch-against-signature is still implemented in C++. OK, I've junked the C++ code completely and reimplemented check-pitch-against-signature in SCM. http:/

Improve implementation of dashed slurs

http://codereview.appspot.com/41099/diff/1021/52 File Documentation/user/expressive.itely (right): http://codereview.appspot.com/41099/diff/1021/52#newcode634 Line 634: @lilypondfile[verbatim,lilyquote,texidoc,doctitle] @ignore this unless you're going to run makelsr.py (or create input/lsr file

Re: Fix key signatures with accidentals in specific octave.

On 2009/04/17 19:25:31, joeneeman wrote: http://codereview.appspot.com/11052/diff/3409/2410 File scm/music-functions.scm (right): http://codereview.appspot.com/11052/diff/3409/2410#newcode1047 Line 1047: ((and (equal? ignore-octave #f) I think eq? is more appropriate here Done. http://code

Add N.C. entry to ChordNames context.

http://codereview.appspot.com/62076/diff/7/1008 File lily/chord-name-engraver.cc (right): http://codereview.appspot.com/62076/diff/7/1008#newcode65 Line 65: SCM no_chord_markup = get_property ("noChordSymbol"); \set noChordSymbol = ##f will cause (harmless but annoying) errors here. You could ch

Re: Add N.C. entry to ChordNames context.

http://codereview.appspot.com/62076/diff/2001/1016 File lily/chord-name-engraver.cc (right): http://codereview.appspot.com/62076/diff/2001/1016#newcode20 Line 20: #include "text-interface.hh" Goes above #include "warn.hh" http://codereview.appspot.com/62076/diff/2001/1016#newcode63 Line 63: SCM

New format for autobeaming rules

http://codereview.appspot.com/88155/diff/43/1044 File Documentation/user/rhythms.itely (right): http://codereview.appspot.com/88155/diff/43/1044#newcode1662 Line 1662: of the beam, e.g. @code{#'(1 . 16)}, or @code{#'*} to indicate a This could be confusing to users unfamiliar with Scheme, since

Re: New format for autobeaming rules

Carl, I haven't commenting on them directly, but there are quite a few indentation errors in the .scm files. http://codereview.appspot.com/88155/diff/95/1147 File Documentation/topdocs/NEWS.tely (right): http://codereview.appspot.com/88155/diff/95/1147#newcode69 Line 69: section 1.2.4 Beams, fo

Re: New format for autobeaming rules

http://codereview.appspot.com/88155/diff/2005/3086 File scm/music-functions.scm (right): http://codereview.appspot.com/88155/diff/2005/3086#newcode519 Line 519: (make-simultaneous-music output) This breaks all the Festival regression tests which use \time (song-associated-voice.ly, song-basic.ly

Re: New markup commands: \left-brace & \right-brace.

Reviewers: Patrick McCarty, Message: Thanks for the review, Patrick. On 2009/07/16 03:40:58, Patrick McCarty wrote: http://codereview.appspot.com/8874/diff/2201/3202 File scm/define-markup-commands.scm (right): http://codereview.appspot.com/8874/diff/2201/3202#newcode2625 Line 2625: (find-br

Re: New instrument name positioning in Scheme.

Reviewers: joeneeman, Message: On 2009/07/16 05:52:35, joeneeman wrote: Just one corner case, otherwise lgtm Thanks, Joe. I'll add a convert-ly rule for ly:system-start-text::print, a regtest and NEWS entry before pushing. http://codereview.appspot.com/91119/diff/1/10 File scm/output-lib.s

Re: New markup commands: \left-brace & \right-brace.

Add exported function ly:otf-glyph-count. * use this function to remove hard-coded value in binary search and brace lookup. * normalize type assertion messages * fix potential null dereference in ly:otf-font-glyph-info * tidy formatting http://codereview.appspot.com/8874 __

Re: New format for autobeaming rules

http://codereview.appspot.com/88155/diff/3101/4027 File input/new/grouping-beats.ly (right): http://codereview.appspot.com/88155/diff/3101/4027#newcode7 Line 7: Beaming patterns may be altered with the @code{beatGrouping} property: new texidoc using \overrideBeamSettings http://codereview.appsp

Re: New markup commands: \left-brace & \right-brace.

On 2009/07/17 01:40:04, Carl wrote: Code looks good to me. Thanks for taking a look. http://codereview.appspot.com/8874/diff/5202/4204#newcode2623 Line 2623: (ly:font-get-glyph font (string-append "brace" (number->string n) Do we want to keep line length to <80 chars? Definitely. I'v

Re: Move `easy notation' print callback to scheme.

Reviewers: hanwenn, Message: On 2009/08/17 02:57:15, hanwenn wrote: http://codereview.appspot.com/107046/diff/1/3 File lily/staff-symbol-referencer-scheme.cc (right): http://codereview.appspot.com/107046/diff/1/3#newcode45 Line 45: " with @var{grob}.") fix indents Which ones? I've double-

Re: Implement new handling for \paper margin settings.

http://codereview.appspot.com/109051/diff/1/5 File lily/output-def.cc (right): http://codereview.appspot.com/109051/diff/1/5#newcode146 Line 146: if (scm_paper_width != SCM_UNDEFINED On 2009/08/20 12:54:08, Carl wrote: I'd prefer to see this and all of your checks for SCM_UNDEFINED) be written

Re: Move ambitus print callback to scheme.

Reviewers: hanwenn, Message: On 2009/08/20 03:12:26, hanwenn wrote: http://codereview.appspot.com/110047/diff/1/11 File scm/output-lib.scm (right): http://codereview.appspot.com/110047/diff/1/11#newcode778 Line 778: (ly:grob-suicide! grob) this does not make sense. did you forget to check th

Implement framework for post-fix text (de)cresc spanners (backend only)

LGTM. Don't forget to add 'span-text to all-music-properties. http://codereview.appspot.com/109072/diff/1/2 File input/regression/dynamics-custom-text-spanner-postfix.ly (right): http://codereview.appspot.com/109072/diff/1/2#newcode1 Line 1: \version "2.13.1" 2.13.4 http://codereview.appspot.

Re: Fix #743: reinstate warning for unterminated span dynamics.

Reviewers: Reinhold, Message: On 2009/09/01 12:04:52, Reinhold wrote: I can't really judge the internals of the patch. But at least I don't see any obvious problem. It should be harmless, since it's basically the same code lifted from dynamic-engraver.cc with a slight refinement for the warn

Re: Fix collisions between hairpins and dynamic text spanners.

Reviewers: Reinhold, Message: On 2009/09/01 11:41:15, Reinhold wrote: LGTM. But a regtest is missing ;-) Ah, that was just me being lazy. :) I'll add a regtest when I commit the patch. I'm wondering whether adjacent-hairpins needs a convert rule. I'm thinking not, since it's an internal pro

* Fix quoting overrides, set etc.

LGTM. There are a few trailing spaces in the regtest though. Some of the lines in recording-group-emulate are far too long (particularly where you've added the comment). http://codereview.appspot.com/124064/diff/1/2 File input/regression/quote-overrides.ly (right): http://codereview.appspot.c

* killCues should not remove music quoted with \quoteDuring, only \cueDuring!

LGTM. http://codereview.appspot.com/126048/diff/1001/4 File input/regression/quote-kill-cues.ly (right): http://codereview.appspot.com/126048/diff/1001/4#newcode11 Line 11: q = \relative c' { d2 \quoteDuring #"M" { s1 } e2 \cueDuring #"M" #UP {s1} f2 } { s1 } http://codereview.appspot.com/1260

* Allow unsetting Voice.instrumentCueName

http://codereview.appspot.com/124110/diff/1/3 File lily/instrument-switch-engraver.cc (right): http://codereview.appspot.com/124110/diff/1/3#newcode44 Line 44: if (!scm_is_null (cue_text)) I think if (Text_interface::is_markup (cue_text)) would be more idiomatic here. http://codereview.appspo

Re: Tracker 836: Add facility to change output file-name for a \book block

Hi Ian, I haven't commented on lily-library.scm, since there are too many formatting issues. I'd suggest installing emacs to sort out the indentation (even if you prefer to use another editor for your main work), otherwise we're going to spend ages pointing out every little nitpick when we shoul

New twoside mode.

LGTM. http://codereview.appspot.com/144049/diff/1/2 File input/regression/paper-twoside.ly (right): http://codereview.appspot.com/144049/diff/1/2#newcode15 Line 15: binding-offset = 5 \mm I think this deserves a separate test, otherwise it just looks the same as an ordinary page with slightly b

Re: New twoside mode.

Hi Michael, This looks OK, apart from a few indentation issues. In particular, the whole of `set-paper-dimensions' is slightly off. Cheers, Neil http://codereview.appspot.com/144049/diff/23/1010 File scm/page.scm (right): http://codereview.appspot.com/144049/diff/23/1010#newcode316 Line 31

Re: Tracker 836: Add facility to change output file-name for a \book block

Hi Ian, Carl's already commented on \pitchedTrill, but there's absolutely nothing wrong with its current indentation; the only thing that you might change is the newline after the let*. Regards, Neil http://codereview.appspot.com/143055/diff/19/1017 File lily/parser.yy (right): http://coderev

Re: Start work on adding annotations for horizontal paper variables.

Reviewers: xmichael-k, Message: On 2009/11/02 08:01:38, xmichael-k wrote: Nice work, very helpful! Cheers. Don't worry if my comments are stupid... ;)) No, they're very useful. http://codereview.appspot.com/143071/diff/1/2 File scm/page.scm (right): http://codereview.appspot.com/1430

Re: Tracker 836: Add facility to change output file-name for a \book block

On 2009/11/04 23:03:27, Ian Hulin wrote: Thanks for the feedback. Is the now patch readu to push now? There are several lines with trailing spaces. You've reverted some recent changes in lily-library.scm which breaks compilation; see `flatten-list' and `eval-carefully'. Regards, Neil http:/

Re: Tracker 836: Add facility to change output file-name for a \book block

http://codereview.appspot.com/143055/diff/2010/2013 File ly/music-functions-init.ly (left): http://codereview.appspot.com/143055/diff/2010/2013#oldcode604 Line 604: (ly:music-property main-note 'elements goes with (lambda http://codereview.appspot.com/143055/diff/2010/2013 File ly/music-fun

Re: Tracker 836: Add facility to change output file-name for a \book block

On 2009/11/08 20:17:37, Ian Hulin wrote: The latest version of this patch is now ready for review. `warning: 15 lines add whitespace errors.' There were twelve in the last patchset. :) Five are space-before-tab issues, the rest trailing spaces. Generally looks OK, apart from a few remaining

Re: Tracker 836: Add facility to change output file-name for a \book block

http://codereview.appspot.com/150044/diff/1/4 File ly/music-functions-init.ly (left): http://codereview.appspot.com/150044/diff/1/4#oldcode604 ly/music-functions-init.ly:604: (ly:music-property main-note 'elements with (lambda http://codereview.appspot.com/150044/diff/1/4 File ly/music-func

Source_file: remove mbrtowc() in favor of utf8_char_len()

Hi Patrick, This looks fine to me. Cheers, Neil http://codereview.appspot.com/154046 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Tracker 836: Add facility to change output file-name for a \book block

http://codereview.appspot.com/150044/diff/11/1015 File ly/music-functions-init.ly (right): http://codereview.appspot.com/150044/diff/11/1015#newcode621 ly/music-functions-init.ly:621: (forced (ly:music-property (car sec-note-events ) 'force-accidental))) (car sec-note-events) http://codereview.

Re: Tracker 836: Add facility to change output file-name for a \book block

On 2009/11/12 01:12:35, Carl wrote: A few whitespace errors (tab following spaces) and one indenting mistake. Then I think it's good to go. A useful tip for catching these errors is to apply the patch locally: git will tell you where the whitespace errors are. http://codereview.appspot.com/

Re: Tracker 836: Add facility to change output file-name for a \book block

On 2009/11/12 01:12:35, Carl wrote: A few whitespace errors (tab following spaces) and one indenting mistake. Then I think it's good to go. A useful tip for catching these errors is to apply the patch locally: git will tell you where the whitespace errors are. http://codereview.appspot.com/

Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."

Hi Chris, Thanks for working on these issues. I've applied your patch to master. Cheers, Neil http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix crash in script_column when suggestAccidentals = ##t is used (issue163041)

Hi Reinhold, This looks OK, but there's still a remaining problem which I've mentioned in the bugtracker for #787. The comment is too specific though, since it's nothing to do with suggestAccidentals; it just happens that this regtest has two scripts (i.e., an articulation + the accidental) stac

787 -- Eliminate segfaults due to empty list and unitialized properties (issue163048)

LGTM. http://codereview.appspot.com/163048/diff/1/2 File lily/script-column.cc (left): http://codereview.appspot.com/163048/diff/1/2#oldcode161 lily/script-column.cc:161: g->set_property ("outside-staff-priority", indent (+ following lines) http://codereview.appspot.com/163048/diff/1/2#oldcode

Add option to indicate frets by letters in tablature (issue164063)

Hi Trevor, This looks OK apart from a few minor details (I've mentioned the interface/doc issues in the main thread). I look forward to the next instalment. Cheers, Neil http://codereview.appspot.com/164063/diff/1/2 File input/regression/tablature-letter.ly (right): http://codereview.appspot

Re: Fix Tracker 918, Add extra RemoveEmpty*StaffContext functions to support "Frenched" scores (issue165096)

Hi Ian, LGTM, apart from some formatting issues and a few incorrect \version numbers. Can you sort out the naming of the new regression tests? For consistency with the existing test, I'd advise amending them as follows: hara-kiri-drumstaff.ly hara-kiri-rhythmicstaff.ly hara-kiri-tabstaff.ly C

Re: Fix Tracker 918, Add extra RemoveEmpty*StaffContext functions to support "Frenched" scores (issue165096)

On 2009/12/12 00:12:16, Ian Hulin wrote: I've implemented Neil's comments, re-run regression tests locally and uploaded amended patches to Rietveld. Thanks! I think this should be ready to push now. Nearly there: You're playing catch-up with the \version strings. :) Both hara-kiri-rhyt

Re: Add option to indicate frets by letters in tablature (issue164063)

Hi Trevor, Here are a few comments as promised earlier. Cheers, Neil http://codereview.appspot.com/164063/diff/5001/5002 File Documentation/changes.tely (right): http://codereview.appspot.com/164063/diff/5001/5002#newcode73 Documentation/changes.tely:73: \new TabStaff trailing space http://c

Try to fix ties in midi. (issue174080)

Hi Reinhold, This looks OK, but I don't think you need to use a list any more. Cheers, Neil http://codereview.appspot.com/174080/diff/1003/1004 File lily/tie-performer.cc (right): http://codereview.appspot.com/174080/diff/1003/1004#newcode34 lily/tie-performer.cc:34: Head_audio_event_tuple ()

Re: Fix Tracker 918, Add extra RemoveEmpty*StaffContext functions to support "Frenched" scores (issue165096)

On 2009/12/14 17:15:13, Ian Hulin wrote: Version numbers sorted, also formatting of music in hara-kiri-drumstaff and hara-kiri-tabstaff. Regression tests re-run locally. LGTM. You just need to rebase against master to get rid of the reverts in engraver-init.ly, then it'll be ready for pus

Re: Add option to indicate frets by letters in tablature (issue164063)

On 2009/12/14 12:13:07, t.daniels_treda.co.uk wrote: I think I've corrected the formatting errors as you suggested. Could you please explain when I should use a 1-space indent and when 2-space, please, otherwise this seems just arbitrary. I do find a 1-space indent makes it more difficult to se

Re: Fix Tracker 918, Add extra RemoveEmpty*StaffContext functions to support "Frenched" scores (issue180095)

On 2009/12/21 00:33:59, Ian Hulin wrote: New patch-set uploaded with regressions in engraver-init.ly fixed. Now OK to push? LGTM, applied. Thanks, Neil http://codereview.appspot.com/180095 ___ lilypond-devel mailing list lilypond-devel@gnu.org

Re: Issue 659: alternate segno symbol (issue181144)

http://codereview.appspot.com/181144/diff/1002/1003 File input/regression/bar-line-segno.ly (right): http://codereview.appspot.com/181144/diff/1002/1003#newcode9 input/regression/bar-line-segno.ly:9: \relative \new StaffGroup << \relative c' http://codereview.appspot.com/181144/diff/1002/1004 F

Re: Issue 659: alternate segno symbol (issue181144)

http://codereview.appspot.com/181144/diff/1009/29 File lily/span-bar.cc (right): http://codereview.appspot.com/181144/diff/1009/29#newcode204 lily/span-bar.cc:204: else if (type == "S") You also need to pick up "S."/".S" here, otherwise you'll get a nasty surprise between staves ;) else if (typ

Re: Issue 659: alternate segno symbol (issue181144)

On 2010/01/11 01:05:07, Reinhold wrote: AFAICS, Bar_line::compound_barline resets both "S." and ".S" to "S", so this should be fine. Or am I missing something? If "S." or ".S" is (incorrectly) set in the middle of a line, the glyph calculation won't be reset, resulting in a segno glyph paste

Enhancement: Providing ly:grob-set-nested-property! (issue183159)

LGTM, pushed to master. Cheers, Neil http://codereview.appspot.com/183159 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Make tab-note-heads and fretboards use common code. (issue186268)

http://codereview.appspot.com/186268/diff/1/2 File lily/fretboard-engraver.cc (left): http://codereview.appspot.com/186268/diff/1/2#oldcode100 lily/fretboard-engraver.cc:100: SCM changes = get_property("chordChanges"); get_property ( http://codereview.appspot.com/186268/diff/1/2#oldcode101 lily

Re: Make tab-note-heads and fretboards use common code. (issue186268)

On 2010/01/24 01:36:35, Carl wrote: I think I've dealt with everything, but there is still an open question on ly:context-property. As far as I can see, there is not currently a means of putting a default in the ly:context-property call. I can see that it would be good to have that. I'm

Re: Make tab-note-heads and fretboards use common code. (issue186268)

Hi Carl, I've just tested this, and it breaks two regtests: 1) dead-notes.ly The \deadNote command inside a chord is ignored (i.e., the tabs appear as numbers rather than crosses). Replacing \deadNote with \tweak also fails. 2) tablature-harmonic.ly The harmonic brackets have disappeared. W

Re: Issue 659: alternate segno symbol (issue181144)

Hi Marc, LGTM, though I still think this is a lot of effort for a very obscure and little-used symbol. Cheers, Neil http://codereview.appspot.com/181144/diff/5028/4010 File input/regression/bar-line-segno.ly (right): http://codereview.appspot.com/181144/diff/5028/4010#newcode2 input/regressio

Re: Fix #943 (input/regression/slur-broken-trend.ly broken) (issue203054)

Reviewers: Patrick McCarty, Message: On 2010/02/21 21:09:54, Patrick McCarty wrote: Is the 'after-line-breaking callback for BarNumber necessary? I'm not quite sure; though it's unlikely anbody's going to change the BarNumber stencil to a tall column (which would need the callback to prevent

Re: Fix #943 (input/regression/slur-broken-trend.ly broken) (issue203054)

On 2010/02/22 21:10:25, joeneeman wrote: For vertical positioning to work, it's important that after-line-breaking be called before Page_layout_problem does its work. Can you check that this is still the case? The regression tests check out (though that's hardly surprising, considering all

Re: Fix #189: Episema over single neume. (issue186189)

Reviewers: hanwenn, Message: Hi Han-Wen, Thanks for your review. Cheers, Neil http://codereview.appspot.com/186189/diff/2001/2005 File lily/episema-engraver.cc (right): http://codereview.appspot.com/186189/diff/2001/2005#newcode92 lily/episema-engraver.cc:92: announce_end_grob (finished_, SC

Re: Fix #943 (input/regression/slur-broken-trend.ly broken) (issue203054)

On 2010/03/04 20:40:08, joeneeman wrote: Just for the record, I posted to lily-devel (because I wanted to attach a file and I can't seem to do that here) to point out that this patch breaks input/regression/page-spacing-rehearsal-mark.ly. Unfortunately, it didn't show up with `make check', so

Re: Fix #305: Allow alignment spanner to be broken for dynamics. (issue129073)

Reviewers: hanwenn, Message: On 2010/03/04 15:02:37, hanwenn wrote: LGTM One doubt about the naming (that I proposed in the issue) - break_alignment could be misconstrued to be alignment-for-break. Maybe alignment-breaker, or similar? OK, alignment_breaker_ sounds fine. BTW, I'm not r

Context mods stored in variable, can be inserted into \with or \context (issue475041)

Hi Reinhold, LGTM. You need to check your indentation in parser.yy, since you're adding spaces instead of hard tabs. Cheers, Neil http://codereview.appspot.com/475041/diff/1/5 File input/regression/hara-kiri-keep-previous-settings.ly (right): http://codereview.appspot.com/475041/diff/1/5#ne

Re: Enhancement: tabalture chord repetition (issue224082)

Hi Marc, Here are a few more comments for you. Cheers, Neil http://codereview.appspot.com/224082/diff/1012/17 File Documentation/notation/fretted-strings.itely (right): http://codereview.appspot.com/224082/diff/1012/17#newcode256 Documentation/notation/fretted-strings.itely:256: is provided.

Re: Enhancement: tabalture chord repetition (issue224082)

On 2010/03/15 09:53:31, marc wrote: > http://codereview.appspot.com/224082/diff/1012/19 > File ly/chord-repetition-init.ly (right): > > http://codereview.appspot.com/224082/diff/1012/19#newcode71 > ly/chord-repetition-init.ly:71: (make-repeat-chord-function '() '())) > indent > > #(define >

Fix #786. (issue885044)

Reviewers: , Message: Hi, This patch implements the suggestion outlined here: http://lists.gnu.org/archive/html/lilypond-devel/2010-01/msg00120.html Please review. Thanks, Neil Description: Fix #786. Send a CompletizeExtenderEvent at the end of each lyrics block so that the Extender_engra

Re: Fix #786. (issue885044)

On 2010/04/06 08:13:20, Trevor Daniels wrote: Typo http://codereview.appspot.com/885044/diff/1/2 File input/regression/display-lily-tests.ly (right): http://codereview.appspot.com/885044/diff/1/2#newcode1 input/regression/display-lily-tests.ly:1: \version "2.13.8" 2.13.18 Done. Thanks fo

Rationalize string number handling for notes and chords (issue906045)

Hi Carl, LGTM. Regarding the style nitpicks, fixcc.py will sort all these out automatically (though it does still have the annoying habit of reformatting the trailing closing parenthesis in the ADD_TRANSLATOR/ADD_INTERFACE macros). Cheers, Neil http://codereview.appspot.com/906045/diff/1/3 Fi

Re: Fix #915 (faulty full-bar rest positioning with clef). (issue931041)

Reviewers: carl.d.sorensen_gmail.com, Message: Hi Carl, Thanks for checking this out. On 2010/04/19 19:37:12, Carl wrote: Should the name of this property be something like ignore-prefatory-material? Hmm, possibly; it's certainly less vague. :) Actually, I've had a thought: instead of usin

Change \cresc,\dim,\decresc to post-fix operators for (de)cresc spanners (issue956048)

LGTM. http://codereview.appspot.com/956048/diff/1/4 File python/convertrules.py (right): http://codereview.appspot.com/956048/diff/1/4#newcode3001 python/convertrules.py:3001: @rule ((2,13,19), (2, 13, 19) http://codereview.appspot.com/956048/show

Re: Doc: Reorganize music functions material. (issue970044)

http://codereview.appspot.com/970044/diff/1/3 File Documentation/notation/changing-defaults.itely (right): http://codereview.appspot.com/970044/diff/1/3#newcode3641 Documentation/notation/changing-defaults.itely:3641: @ref{Music function type predicates}. There's a danger here that users might t

Re: Doc: Reorganize music functions material. (issue970044)

On 2010/04/29 07:13:18, Mark Polesky wrote: There aren't that many other predicates out there (see http://lists.gnu.org/archive/html/lilypond-devel/2009-08/msg00713.html). And I'm happy to add the remaining ones to the alist if that will justify using the word "complete". (: As Carl's poin

Fix #943. (issue956051)

LGTM. http://codereview.appspot.com/956051/diff/1/3 File lily/slur-scoring.cc (right): http://codereview.appspot.com/956051/diff/1/3#newcode86 lily/slur-scoring.cc:86: Slur_score_state::slur_direction (Grob *me) const Do you need to pass `me' here? Isn't it the same as slur_? http://coderevie

Re: Doc: Reorganize music functions material. (issue1031044)

On 2010/05/07 12:20:50, Graham Percival wrote: The patch looks ok, but I'm getting some weird build errors... quite possibly the same thing Werner noticed (wherein lilypond-book borks if it has two identical snippets). No problems here following a clean build. Cheers, Neil http://coderevi

Re: Doc: Reorganize music functions material. (issue1031044)

LGTM. http://codereview.appspot.com/1031044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: NR: Using \partial with \repeat. (issue1136044)

http://codereview.appspot.com/1136044/diff/1/2 File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/1136044/diff/1/2#newcode159 Documentation/notation/repeats.itely:159: If alternate endings are added to a repeat that includes the On 2010/05/11 02:37:54, Carl wrote:

Re: Doc: NR: Using \partial with \repeat. (issue1136044)

LGTM. I'm with Carl on removing the first example. Cheers, Neil http://codereview.appspot.com/1136044/diff/6001/7002 File Documentation/notation/rhythms.itely (right): http://codereview.appspot.com/1136044/diff/6001/7002#newcode2935 Documentation/notation/rhythms.itely:2935: \set Timing.measu

Add independent control of thickness and offset for underline markup (issue1347041)

Hi Kieren, I don't think we can remove the link between 'line-thickness and underline offset, since it should scale based on staff-size. At small staff-sizes, 'line-thickness gets progressively larger, which matches the thicker underline with a slightly bigger gap. What you could do instead is

Re: Add independent control of thickness and offset for underline markup (issue1347041)

On 2010/05/29 16:10:16, kieren_macmillan_sympatico.ca wrote: I tried #:properties ((thickness 1)) and it failed. Sorry, I meant remove the default value, but keep `offset' in the properties list, i.e., #:properties ((thickness 1) (offset)) This way it's bound to #f if the user hasn't over

Re: Woodwind diagrams (issue1425041)

Reviewers: carl.d.sorensen_gmail.com, MikeSol, Message: Hi Mike, This is super work, you're obviously a schemer extraordinaire. ;) I've copied my comments from the original set, and added a few more (you'll see some reiterate Carl's points). I think woodwind-diagrams.scm is a bit unwieldy in i

Re: Experimental support for woff fonts in svg. (issue1428042)

http://codereview.appspot.com/1428042/diff/13002/17023 File scm/output-lib.scm (right): http://codereview.appspot.com/1428042/diff/13002/17023#newcode897 scm/output-lib.scm:897: (define-public (font-name-split font-name) These look out of place here. Move to font.scm or backend-library.scm? ht

Re: Experimental support for woff fonts in svg. (issue1428042)

Hi Jan, I've just been testing this patch, and have stumbled upon a problem with chords. With this snippet, \chords { c4 } I get the following error message: /home/neil/lilypond/out/share/lilypond/current/scm/backend-library.scm:270:23: In procedure car in expression (car (ly:pango-font-phy

Align metronome mark at time signature or first musical element. Fixes #684. (issue1579041)

Hi Jan, Have you checked what happens with full-bar rests? I haven't tested your patch, but it's similar to the one I posted, which suffers from invisible tempo marks at full-bar rests. Cheers, Neil http://codereview.appspot.com/1579041/diff/2001/3001 File lily/metronome-engraver.cc (right):

Re: fix ly:parser-parse-file in an ly file (issue1345041)

On 2010/06/07 13:59:45, Reinhold wrote: The only occurrence where it might make sense to have a separate parser is in scm/parser-ly-from-scheme.scm in the function read-lily-expression, which calls parse-string-result. However, I fail to see where this is actually used... It's called whene

Re: Doc: Contributor's: Update Regression tests (issue1545043)

Hi Carl, LGTM too. Cheers, Neil http://codereview.appspot.com/1545043/diff/6001/7001 File Documentation/contributor/programming-work.itexi (right): http://codereview.appspot.com/1545043/diff/6001/7001#newcode1304 Documentation/contributor/programming-work.itexi:1304: @code{test-redo}? In my e

Re: Align metronome mark at time signature or first musical element. Fixes #684. (issue1579041)

Hi Jan, I've tested the latest patch, and it looks pretty good so far. Here are a few thoughts on positioning: -) A metronome mark at a full-bar rest should be aligned with the barline instead of the paper column to the left of the rest. -) If there's a tempo change at a key signature, the met

Re: Lilypond-book: Factor out the formatting from lilypond-book into separate classes (issue1543042)

Hi Reinhold, LGTM. Images inside info are all present and correct (using emacs). Cheers, Neil http://codereview.appspot.com/1543042/diff/17001/18007 File scripts/lilypond-book.py (right): http://codereview.appspot.com/1543042/diff/17001/18007#newcode207 scripts/lilypond-book.py:207: group =

Re: Redo autobeam settings to make resetting easier (issue1667041)

Hi Carl, Are you sure this is ready for review? I see comments like this, // I removed this to solve a bug; need to make sure it's right -- cds and commented debug code, +;(display "\nIn auto-beam.scm\n") and alarm bells start to ring. :) Also, there are several places where beamSettings is

Re: Redo autobeam settings to make resetting easier (issue1667041)

Hi Trevor, On 2010/06/14 18:18:21, t.daniels_treda.co.uk wrote: I downloaded and applied Patch set 1, but make failed with make[2]: Leaving directory `/media/Data/flower/include' make[1]: Leaving directory `/media/Data/flower' make[1]: Entering directory `/media/Data/lily' rm -f ./out/auto-b

Re: Experimental support for woff fonts in svg. (issue1428042)

Hi Jan, On 2010/06/09 21:58:13, janneke-list_xs4all.nl wrote: Thanks for looking into this! I've added a fix for this; it appears that a pango font (which specifies an existing font file) can have no matching pango physical fonts. Quite strange. Cheers, it works fine now. I've tried a docs

Re: Experimental support for woff fonts in svg. (issue1428042)

Hi Jan, On 2010/06/15 08:55:49, jan.nieuwenhuizen wrote: I just rebased the patch onto latest master and did a fresh build and doc-build make all all-doc and cannot reproduce it, the doc builds without problems. Do you compile with --disable-optimising? I've done a few more tests,

Re: Revised autobeam settings patch -- cleaned up debug comments (issue1667044)

Hi Carl, There's a file missing from this set: time-signature-settings.scm. Cheers, Neil http://codereview.appspot.com/1667044/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Align metronome mark at time signature or first musical element. Fixes #684. (issue1579041)

Hi Jan, On 2010/06/16 12:08:06, jan.nieuwenhuizen wrote: On 2010/06/08 22:22:43, Neil Puttock wrote: > -) A metronome mark at a full-bar rest should be aligned with the barline > instead of the paper column to the left of the rest. This is not in Read or #684's description... See http:

Re: Revised autobeam settings patch -- cleaned up debug comments (issue1667044)

Carl, have you looked at the regtest output? I've copied time-signature-settings.scm from the other set (I assume it's correct) and run `make check'; there's a significant number of changed tests showing up. In particular, 6/8, 9/8 and 12/8 tests are mostly unbeamed. There's some weirdness goin

Re: Experimental support for woff fonts in svg. (issue1428042)

On 2010/06/17 19:49:26, Patrick McCarty wrote: Adding the following code fixes the memory leak Neil refers to, though there might be a better way. (if (ly:get-option 'svg-woff) (module-define! (ly:outputter-module outputter) 'paper #f)) Ah, that's cute. :) I was searching throught t

Re: Experimental support for woff fonts in svg. (issue1428042)

On 2010/06/18 11:29:23, jan.nieuwenhuizen wrote: I'll go ahead and test on a 32 bit machine then. It compiles fine for me. I'm using a 64 bit machine (Ubuntu 10.04). Any tips on how I can debug via gdb? Cheers, Neil http://codereview.appspot.com/1428042/show __

Fix 1112. (issue1670042)

LGTM. http://codereview.appspot.com/1670042/diff/1/2 File input/regression/page-breaking-min-distance.ly (right): http://codereview.appspot.com/1670042/diff/1/2#newcode1 input/regression/page-breaking-min-distance.ly:1: \version "2.13.22" 2.13.26 http://codereview.appspot.com/1670042/diff/1/2#

  1   2   3   4   5   6   >