Re: Doc: NR Section on Upbeats made clearer (issue 5520056)

http://codereview.appspot.com/5520056/diff/1/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right): http://codereview.appspot.com/5520056/diff/1/Documentation/notation/rhythms.itely#newcode1366 Documentation/notation/rhythms.itely:1366: The @code{\partial

Re: Doc: NR Section on Upbeats made clearer (issue 5520056)

On 2012/01/09 12:58:58, Neil Puttock wrote: http://codereview.appspot.com/5520056/diff/1/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right): http://codereview.appspot.com/5520056/diff/1/Documentation/notation/rhythms.itely#newcode1366

Re: Doc: NR Section on Upbeats made clearer (issue 5520056)

Note that this is misleading: \relative c' { \partial 4 c4 \applyContext #(lambda (ctx) (newline) (display (ly:context-current-moment ctx))) c1 } - #Mom 1/4 The Rational class doesn't display negative rationals unless they're infinite.

Re: changes.tely: mention Flag changes, remove duplicate does (issue 5540058)

http://codereview.appspot.com/5540058/diff/3003/Documentation/changes.tely File Documentation/changes.tely (right): http://codereview.appspot.com/5540058/diff/3003/Documentation/changes.tely#newcode67 Documentation/changes.tely:67: \override Flag #'color = #red g8 Please put the note on a new

Re: Glyphs for Kievan Notation (issue 4951062)

http://codereview.appspot.com/4951062/diff/99002/lily/stem.cc File lily/stem.cc (right): http://codereview.appspot.com/4951062/diff/99002/lily/stem.cc#newcode284 lily/stem.cc:284: string style = robust_symbol2string (heads[0]-get_property (style), default); A bit fussy. You can safely check

Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)

Hi David, Should I wait for a new patch or can I test using the latest one here? I've tried it out briefly on a real music example, and have a problem with identifiers: foo = \mark \default \relative c' { \foo c1 } - error: syntax error, unexpected EVENT_IDENTIFIER \foo If I add a note

Re: Don't wrap EventChord around rhythmic events by default. (issue 5440084)

Great work David. I like how \harmonic works properly for single notes now. :) http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc File lily/music-scheme.cc (right): http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc#newcode79 lily/music-scheme.cc:79: Is

Overhauls broken bound coordinate checking (issue 5602054)

http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly File input/regression/metronome-mark-broken-bound.ly (right): http://codereview.appspot.com/5602054/diff/2001/input/regression/metronome-mark-broken-bound.ly#newcode1

Re: Gets vertical skylines from grob stencils (issue 5626052)

Hi Mike, I've tested the latest patch on a few real-world scores and there appears to be a bit of weirdness going on with TupletNumbers when a DynamicText's nearby: \relative c' { \times 2/3 { c8^\p c c } } Cheers, Neil http://codereview.appspot.com/5626052/

Re: Uses derived_mark to avoid segfault (issue 5729051)

http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/5729051/diff/6001/lily/span-bar-stub-engraver.cc#newcode160 lily/span-bar-stub-engraver.cc:160: // removes all unused contexts Is it feasible

Re: hideNotes should hide also TabNoteHead (issue 2480). (issue 6105049)

On 2012/04/23 04:50:21, Keith wrote: I need an \override NoteColumn #'ignore-collision = ##t here or else I get a warning (either before or after this patch). I mention this only because I broke the doc build not too long ago by causing a warning from Lilypond input in the docs. (patchy

Re: hideNotes should hide also TabNoteHead (issue 2480). (issue 6105049)

On 2012/04/23 12:09:15, dak wrote: \voiceOne is already used for the upper voice. The problem is that the voice settings are totally messed up by the standard grace synchronization problem. Remove the graces, and the score typesets fine. Ah, I didn't scroll down that far. :)

Fix a number of display-lily shortcomings (issue 6203056)

LGTM. Just some redundant spaces in the ArticulationEvent display method needs removing, but then they were already there. Cheers, Neil http://codereview.appspot.com/6203056/ ___ lilypond-devel mailing list lilypond-devel@gnu.org

Re: add \shape (issue 6255056)

http://codereview.appspot.com/6255056/diff/5001/input/regression/shape-slurs.ly File input/regression/shape-slurs.ly (right): http://codereview.appspot.com/6255056/diff/5001/input/regression/shape-slurs.ly#newcode5 input/regression/shape-slurs.ly:5: @code{\\shape}. The blue slurs are modified

Re: Control mid-measure beams in 3/4 time; Issue 2566 and 2246 (issue 6247055)

LGTM. http://codereview.appspot.com/6247055/diff/9002/input/regression/autobeam-3-4-rules.ly File input/regression/autobeam-3-4-rules.ly (right): http://codereview.appspot.com/6247055/diff/9002/input/regression/autobeam-3-4-rules.ly#newcode17 input/regression/autobeam-3-4-rules.ly:17: \set

Cleanups in one-line-page-breaking. (issue 6248056)

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

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 ___

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.

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

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.

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

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

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,

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:

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

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

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've

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

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

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

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

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

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

* 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):

* 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.

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

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

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

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):

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

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

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

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)

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.

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.

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)

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):

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

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

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

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: 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

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

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

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 pasted between

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

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.

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.

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

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 #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_,

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',

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

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):

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

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 (make

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

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 for

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

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

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

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

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: 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

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?

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

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

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

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

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: 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

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

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

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

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

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

On 2010/06/19 04:55:25, Carl wrote: Both versions can be used, according to Ross. The reason we have 2/8 standing alone is to prevent 2/8, 3/8, and 4/8 from being beamed together in 3/4 time, where that would be an error. I don't consider that an error; it's traditional typesetting

Dependency of 'transparent and 'whiteout (issue1681043)

LGTM. http://codereview.appspot.com/1681043/diff/1/2 File lily/grob.cc (right): http://codereview.appspot.com/1681043/diff/1/2#newcode126 lily/grob.cc:126: bool grob_transparent = to_boolean (get_property (transparent)); This is a bit fussy. `transparent' will do, since it's clear from the

  1   2   3   4   5   >