Re: Add Changes entries for bare rhythms and \beamExceptions (issue 47850043)

2014-01-04 Thread ianhulin44
Hi David Suggestion for wording the bare durations change. I like the way this lets you code tied notes with varying durations, too! Cheers and Happy New Year, Ian https://codereview.appspot.com/47850043/diff/40001/Documentation/changes.tely File Documentation/changes.tely (right):

Accidental no longer clashes with TupletNumber or TupletBracket (issue 45520044)

2014-01-03 Thread ianhulin44
Hi Mike, LGTM, after a visual check, but just some suggestions re the comments you've added. Cheers, Ian https://codereview.appspot.com/45520044/diff/1/lily/tuplet-bracket.cc File lily/tuplet-bracket.cc (right): https://codereview.appspot.com/45520044/diff/1/lily/tuplet-bracket.cc#newcode667

Doc: NR - Tidy up of 3.5.1 and 3.5.3 - MIDI (issue 45420043)

2013-12-24 Thread ianhulin44
LGTM apart from clarification needed that the \midi { \tempo ..} } example explicitly only causes a tempo change for the audio output. Merry Christmas Ian https://codereview.appspot.com/45420043/diff/1/Documentation/notation/input.itely File Documentation/notation/input.itely (right):

Re: Support articulations, slurs and breaths in MIDI (issue 26470047)

2013-11-26 Thread ianhulin44
On 2013/11/24 15:40:01, janek wrote: 2013/11/24 d...@gnu.org: On 2013/11/24 15:11:38, Devon Schudy wrote: David Kastrup wrote: the next goal for this sub-project may be to get the audio playback to honour the \repeat structures by translating the volta and tremolo flavours to unfold.

Re: Support articulations, slurs and breaths in MIDI (issue 26470047)

2013-11-17 Thread ianhulin44
Hi Devon, Like some other posters, I have not yet reviewed your changes in detail, but would like to make some comments on your patch at the design/concept level. First of all, many thanks for looking at the midi code - there are too few people apart from Jan and I think David K who have much

Re: Add backup option to convert-ly (Issue 3572) (issue 14040043)

2013-09-28 Thread ianhulin44
I'd prefer a single tilde to indicate the a backup file after the version string. https://codereview.appspot.com/14040043/diff/6001/scripts/convert-ly.py File scripts/convert-ly.py (right): https://codereview.appspot.com/14040043/diff/6001/scripts/convert-ly.py#newcode243

Re: Handle MultiMeasureRest direction in the part combiner like other elements (issue 13302048)

2013-09-09 Thread ianhulin44
texidoc string comment nit-pick, otherwise LGTM. https://codereview.appspot.com/13302048/diff/3001/input/regression/part-combine-mmrest-apart.ly File input/regression/part-combine-mmrest-apart.ly (right):

Re: Make user-generated tab-clefs possible (issue 13612043)

2013-09-07 Thread ianhulin44
Nit-pick re comment wording. is valuated by isn't really clear, is it a 'sounds-like' error for 'evaluated' or 'validated', or the suggestion I've made below. Cheers, Ian https://codereview.appspot.com/13612043/diff/6001/scm/tablature.scm File scm/tablature.scm (right):

Re: Issue 3491: Add \displayMarkup command. (issue 12732043)

2013-08-15 Thread ianhulin44
LGTM, but add suggested clarification to EM text. Ian https://codereview.appspot.com/12732043/diff/7001/Documentation/extending/programming-interface.itely File Documentation/extending/programming-interface.itely (right):

Re: Issue 3491: Add \displayMarkup command. (issue 12732043)

2013-08-15 Thread ianhulin44
Use David's wording for EM with some tweaks. Re \displayMarkup \displayScheme: Markup needs some special-casing as we have our own home-brew customisable/extensible interpreter in there for interpreting \markup arguments. The markup interpreter sometimes does some surprising things under the

Re: Fix some oversights of \layout in \book (issue 10794044)

2013-07-10 Thread ianhulin44
On 2013/07/10 21:46:00, PhilEHolmes wrote: LGTM and LGTM too. https://codereview.appspot.com/10794044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Add @code{} to middleCOffset (issue 10566043)

2013-06-25 Thread ianhulin44
On 2013/06/25 20:42:40, thomasmorley651 wrote: Please review. LGTM Ian https://codereview.appspot.com/10566043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Updates to NR chapter 1 (issue 10237048)

2013-06-21 Thread ianhulin44
LGTM apart from one nitpick listed below. Cheers, Ian https://codereview.appspot.com/10237048/diff/1/Documentation/notation/simultaneous.itely File Documentation/notation/simultaneous.itely (right):

Change prehistoric docstring in input/regression/stencil-hacking (issue 9116043)

2013-05-03 Thread ianhulin44
I spotted this howler just below your change. (Plural noun as subject, with is altered as the verb.) Any chance of fixing this too? (If you don't like my wording, at least change the is to are). Cheers, Ian https://codereview.appspot.com/9116043/diff/1/input/regression/stencil-hacking.ly

Doc: Augment section about titles (3103) (issue 8895044)

2013-04-21 Thread ianhulin44
LGTM Cheers, Ian https://codereview.appspot.com/8895044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Doc: elaborate explanation of accidentals within cadenzas (3078) (issue 8866043)

2013-04-20 Thread ianhulin44
LGTM apart from a Nitpick: can implies there just may be an option of the accidentals appearing in this case. So use must have to or need to be. Cheers, Ian https://codereview.appspot.com/8866043/diff/1/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right):

Re: Doc: Change \on-the-fly #procedure to \on-the-fly \procedure (3098) (issue 8853044)

2013-04-18 Thread ianhulin44
LGTM (including your local correction). Add some new index entries for concepts discussed in this section (please feel free to correct my suggestions if they fall foul of the doc. standards). Cheers, Ian

Doc: add missing engraver to example of a user-defined context (3195) (issue 8538046)

2013-04-14 Thread ianhulin44
LGTM Cheers, Ian https://codereview.appspot.com/8538046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Carry any exports from session initialization over to subsequent sessions (issue 8258043)

2013-04-11 Thread ianhulin44
LGTM The comments contain some Scheme comments to help hackers find their way around a bit in lily.scm. Lily.scm is getting a bit unwieldy and untidy - maybe give it a spring-clean in a future tracker? https://codereview.appspot.com/8258043/diff/1/scm/lily.scm File scm/lily.scm (right):

Re: Doc: Error message: unexpected \new (3285) (issue 8581044)

2013-04-11 Thread ianhulin44
On 2013/04/11 09:37:58, Trevor Daniels wrote: Clarify angle brackets should be double Patch Set 2 LGTM Ian https://codereview.appspot.com/8581044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org

Doc: Error message: unexpected \new (3285) (issue 8581044)

2013-04-10 Thread ianhulin44
Maybe say double angle brackets for ' ... ' blocks, as simply angle brackets implies ' ... ' https://codereview.appspot.com/8581044/diff/1/Documentation/usage/running.itely File Documentation/usage/running.itely (right):

Re: Define call-after-session for cleanup called after every file (issue 8310043)

2013-04-03 Thread ianhulin44
LGTM. You've done a load of good work on introducing the session architecture, so why not let people know about it, even if it's only via a comment in lily.scm? Cheers, Ian https://codereview.appspot.com/8310043/diff/1/scm/lily.scm File scm/lily.scm (right):

Re: Fix composition of markup lists containing markup command list calls (issue 7799048)

2013-03-27 Thread ianhulin44
On 2013/03/25 21:07:45, dak wrote: Address Ian's comment. Hopefully. LGTM Cheers, Ian https://codereview.appspot.com/7799048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Page numbering: first page format only on first page of a book (issue 7974046)

2013-03-25 Thread ianhulin44
LGTM - maybe add the comment? Cheers, Ian https://codereview.appspot.com/7974046/diff/1/ly/titling-init.ly File ly/titling-init.ly (right): https://codereview.appspot.com/7974046/diff/1/ly/titling-init.ly#newcode144 ly/titling-init.ly:144: % Check the page number in arg is not the same as the

Re: Fix composition of markup lists containing markup command list calls (issue 7799048)

2013-03-25 Thread ianhulin44
LGTM, apart from comment on the doc-string. Cheers, Ian https://codereview.appspot.com/7799048/diff/3001/scm/define-markup-commands.scm File scm/define-markup-commands.scm (right): https://codereview.appspot.com/7799048/diff/3001/scm/define-markup-commands.scm#newcode4286

Re: Make a PostEvents container class for packaging several postevents (issue 7742044)

2013-03-25 Thread ianhulin44
On 2013/03/24 13:36:42, dak wrote: https://codereview.appspot.com/7742044/diff/5001/scm/define-music-types.scm File scm/define-music-types.scm (right): https://codereview.appspot.com/7742044/diff/5001/scm/define-music-types.scm#newcode77 scm/define-music-types.scm:77: \n(no direction

Re: Make a PostEvents container class for packaging several postevents (issue 7742044)

2013-03-24 Thread ianhulin44
On 2013/03/16 20:51:05, dak wrote: On 2013/03/16 19:19:57, lemzwerg wrote: LGTM. The combination `\' `\n' `\(' is probably worth a comment in the contributors' manual. If \( was defined, things would be so much easier: one would just put \( in the first column. \ newline \n( is the

Re: Make a PostEvents container class for packaging several postevents (issue 7742044)

2013-03-24 Thread ianhulin44
LGTM bar a nit-pick with the doc-string in define-music-types.scm. Sorry for the previous message without the draft comment attached. Cheers, Ian https://codereview.appspot.com/7742044/diff/5001/scm/define-music-types.scm File scm/define-music-types.scm (right):

Re: Snaps horizontally-offset fingerings to vertical column. (issue 7988043)

2013-03-24 Thread ianhulin44
Suggestion for regression test doc-string. It's a bit wordier but clearer and shows you're covering cases for other wide accidentals on the note being fingered like half-flat etc. Cheers, Ian https://codereview.appspot.com/7988043/diff/2001/input/regression/fingering-column-snap-radius.ly File

Re: Allows slurs to break at barlines. (issue 7424049)

2013-03-20 Thread ianhulin44
\fake and \broken are concise but feel wrong, implying something's wrong with something else but the name doesn't describe it. I think a function name where we've got to resort to being clever maybe indicate we're trying to solve the wrong problem: we're trying to make the name short at the

Re: Remove obscure GNOME-backend related commands from Emacs LilyPond-mode (issue 7583044)

2013-03-16 Thread ianhulin44
LGTM too https://codereview.appspot.com/7583044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: \tuplet and \tupletSpan to replace \times and tupletSpannerDuration (issue 7094044)

2013-01-12 Thread ianhulin44
This LGTM, but with one really big question: do we really still need \tupletSpan at all? The start point for this work was to make easy triplets and tuplets, i.e. make handling of all the triplets and *uplets less daunting and a bit more readable by humans. The original \times command had a

Re: \tuplet and \tupletSpan to replace \times and tupletSpannerDuration (issue 7094044)

2013-01-12 Thread ianhulin44
OK, let's go with keeping \tupletSpan with a statement that it's not the preferred way of doing things, and use \tupletSpan \default as the unset. I know \tupletSpanOff may be less typing, but the name kind of implies it accepts #t/#f values. I've only commented on the doc-text for the

Re: \tuplet and \tupletSpan to replace \times and tupletSpannerDuration (issue 7094044)

2013-01-12 Thread ianhulin44
Thanks Trevor, good catch. Have re-drafted the doc-string accordingly. Cheers, Ian https://codereview.appspot.com/7094044/diff/2001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/7094044/diff/2001/ly/music-functions-init.ly#newcode1365

Re: \tuplet and \tupletSpan to replace \times and tupletSpannerDuration (issue 7094044)

2013-01-12 Thread ianhulin44
LGTM https://codereview.appspot.com/7094044/diff/4003/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/7094044/diff/4003/ly/music-functions-init.ly#newcode1372 ly/music-functions-init.ly:1372: ) LGTM https://codereview.appspot.com/7094044/

Re: Create a \tuplet function to complement \times and tupletSpannerDuration (issue 7058068)

2013-01-11 Thread ianhulin44
Hi David, Thanks for spotting this may get lost. Actually getting this one finished after all the [talk] and [proposal] discussions was first on my list of things to do, but I have had to spend the holiday period in hospital, with access to only half of of my e-mails etc. I've only got access

Re: Create a \tuplet function to complement \times and tupletSpannerDuration (issue 7058068)

2013-01-11 Thread ianhulin44
Hi David, Please ignore most of my last post. It looks like you're doing stage one of what the [talk] [proposal] stuff was about. Many thanks, Ian https://codereview.appspot.com/7058068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org

Re: Create a \tuplet function to complement \times and tupletSpannerDuration (issue 7058068)

2013-01-11 Thread ianhulin44
I don't think \tuplet needs the tupletSpannerDuration property. Do it all in-line using the optional @var{duration} parameter. (Maybe call this spanner-duration). See comments below. Cheers, Ian https://codereview.appspot.com/7058068/diff/4/ly/music-functions-init.ly File

Provide define-session and define-session-public commands (issue 6588056)

2012-10-01 Thread ianhulin44
Just a question about the doc-string. http://codereview.appspot.com/6588056/diff/1/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/6588056/diff/1/scm/lily.scm#newcode51 scm/lily.scm:51: A@tie{}session basically corresponds to one LilyPond file on the Is a space needed

Re: Provide define-session and define-session-public commands (issue 6588056)

2012-10-01 Thread ianhulin44
On 2012/10/01 18:24:20, dak wrote: snip 15.6 `@tie{}': Inserting an Unbreakable Space = The `@tie{}' command produces a normal interword space at which a line break may not occur. Always write it with following (empty) braces, as usual for

Re: bar-line interface part 2/2: New bar line definition standard (issue 6498052)

2012-09-27 Thread ianhulin44
Apart from a typo in changes.tely (q.v.), LGTM. http://codereview.appspot.com/6498052/diff/24001/Documentation/changes.tely File Documentation/changes.tely (right): http://codereview.appspot.com/6498052/diff/24001/Documentation/changes.tely#newcode67 Documentation/changes.tely:67: as

Re: Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. (issue 6458159)

2012-09-25 Thread ianhulin44
Closing Rietveld issue now that Tracker Issue 2758 has been verified. Ian http://codereview.appspot.com/6458159/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. (issue 6458159)

2012-09-08 Thread ianhulin44
On 2012/09/08 09:13:17, dak wrote: On 2012/09/06 23:24:04, Ian Hulin (gmail) wrote: On 2012/09/06 18:07:53, dak wrote: When we go Guilev2, we don't want to search for all the backward compatibility. So I'd say you use something like #if GUILEV1 for splicing in the compatibility

Re: Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. (issue 6458159)

2012-09-06 Thread ianhulin44
On 2012/09/03 07:25:20, Patrick McCarty wrote: On 2012/09/03 05:39:57, dak wrote: On 2012/09/03 03:41:39, Patrick McCarty wrote: LGTM Let's not get this merged. ly_lily_module_constant (module-variable) is available in _both_ Guile 1.8 as well as Guile 2.0. Thanks for mentioning

Re: Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. (issue 6458159)

2012-09-06 Thread ianhulin44
On 2012/09/06 18:07:53, dak wrote: On 2012/09/06 17:20:59, Ian Hulin (gmail) wrote: On 2012/09/03 07:25:20, Patrick McCarty wrote: On 2012/09/03 05:39:57, dak wrote: On 2012/09/03 03:41:39, Patrick McCarty wrote: LGTM Let's not get this merged. ly_lily_module_constant

Re: part-combiner.scm: replace use of when with moment (issue 6500069)

2012-09-04 Thread ianhulin44
LGTM. Very similar to the hack I have on my GuileV2 sandbox on my netbook. If partcombine regtests run OK, it's a go. http://codereview.appspot.com/6500069/ ___ lilypond-devel mailing list lilypond-devel@gnu.org

Use directory-local variables to establish some coding styles in Emacs (issue 6460109)

2012-08-19 Thread ianhulin44
Do we need to add anything to the standard .gitignore for these? They're files in the git directories for developers developing the kit rather than users writing code. Quite a lot of developers have the build directory within the git tree. There are already .el files in the kit for lilypond

Re: Use directory-local variables to establish some coding styles in Emacs (issue 6460109)

2012-08-19 Thread ianhulin44
On 2012/08/19 12:37:38, dak wrote: On 2012/08/19 12:12:36, Ian Hulin (gmail) wrote: Do we need to add anything to the standard .gitignore for these? Why should we? This adds just one file which _should_ be checked out. They're files in the git directories for developers developing the

Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. (issue 6458159)

2012-08-19 Thread ianhulin44
Reviewers: , Message: Cleaned up garbage in Subject and Description Fields. Description: Issue 2758. ly_module_lookup caused deprecation warnings with Guile V2.06. The V2.17.0 definition of ly_module_lookup in module-scheme.cc uses two functions, scm_sym2var and scm_module_lookup_closure,

Re: Volta enhancements tranche 1 (issue 6398055)

2012-07-17 Thread ianhulin44
Just some nit-picky comments re the English version of the documentation. Cheers, Ian http://codereview.appspot.com/6398055/diff/1/Documentation/notation/repeats.itely File Documentation/notation/repeats.itely (right):

Re: Document and improve other simultanous music documentation. (issue 6248080)

2012-06-01 Thread ianhulin44
Mostly LGTM. Better idea than putting the stuff in a doc-string for a \placeholder function people would hardly ever use. If you really don't like the extra @cindex that's fine. The other changes are just stylistic things so the English reads with a better flow. Cheers, Ian

Re: Issue 2100: Explanation of branches for CG (issue 5539062)

2012-01-14 Thread ianhulin44
On 2012/01/14 22:56:32, Carl wrote: Here's a revision of Graham's info for the CG on branches. I think it can go in as-is. Please review. Thanks, Carl LGTM Ian http://codereview.appspot.com/5539062/ ___ lilypond-devel mailing list

Re: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. (issue 5464045)

2012-01-02 Thread ianhulin44
I've had to do some major mangling with my local git rep, so the next patch-set is coming up as a new Rietveld issue. It's http://codereview.appspot.com/5504107 Will also update Google tracker. Ian http://codereview.appspot.com/5464045/diff/10002/ly/music-functions-init.ly File

Re: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. (issue 5464045)

2011-12-27 Thread ianhulin44
On 2011/12/27 12:05:38, dak wrote: One problem that I keep having is that I fail to see any documentation of how this is supposedly interacting with the module system. What module are user-defined markup functions placed in? Previously, this would have been a module local to the

Re: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. (issue 5464045)

2011-12-27 Thread ianhulin44
On 2011/12/27 13:00:14, Ian Hulin (gmail) wrote: On 2011/12/27 12:05:38, dak wrote: One problem that I keep having is that I fail to see any documentation of how this is supposedly interacting with the module system. What module are user-defined markup functions placed in? Previously,

Re: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. (issue 5464045)

2011-12-26 Thread ianhulin44
Remove debugging cruft, also other changes in patch-set one not needed since patchset two. Add handler to handle conditions thrown from markup module code in lily.scm. http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cyclic-reference.ly File

Re: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. (issue 5464045)

2011-12-20 Thread ianhulin44
Reviewers: carl.d.sorensen_gmail.com, dak, http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-sided-box.ly File Documentation/snippets/three-sided-box.ly (right): http://codereview.appspot.com/5464045/diff/2001/Documentation/snippets/three-sided-box.ly#newcode20

Re: Doc: Canged @table example in Usage (issue 5492066)

2011-12-17 Thread ianhulin44
Hope this isn't too late. Please apply the change to your new patch. Cheers, Ian http://codereview.appspot.com/5492066/diff/1/Documentation/usage/running.itely File Documentation/usage/running.itely (right):

Re: Provide Scheme sandbox. (issue 5453045)

2011-12-08 Thread ianhulin44
On 2011/12/08 09:07:01, dak wrote: On 2011/12/08 00:38:07, Keith wrote: Very nice. If you make it this easy, I might learn Scheme... nah. Well, it is like converting people to Emacs. Those who have gotten burnt 20 years ago are a lost cause. You can't erase their childhood memories.

Re: avoid cryptic StopIteration failure from make when 'make check' is run before 'make test-baseline' (issue 5361042)

2011-11-07 Thread ianhulin44
On 2011/11/06 21:30:18, adam.spiers wrote: I disagree - there is no way to be sure that the cause of the StopIteration really was the user failing to run make test-baseline first. For example he could have run it but then something else accidentally (or deliberately) deleted those

Re: scheme-tutorial.itely: avoid unnecessary copying (issue 5314065)

2011-10-28 Thread ianhulin44
David, I think you've updated an example in two places, and added material which needs to reference the second example after the first one. You're trying to describe things about coding within music functions before the text gets round to mentioning them. This section is trying to hand-hold the

Re: Implement optional music function arguments (issue 5023044)

2011-09-25 Thread ianhulin44
http://codereview.appspot.com/5023044/diff/22001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/5023044/diff/22001/scm/ly-syntax-constructors.scm#newcode52 scm/ly-syntax-constructors.scm:52: (format #f (_ ~a function can't return ~a) (_

Re: T1780 remove scheme format calls with no destination parameter - deprecated in Guile V2 (issue 4974078)

2011-09-20 Thread ianhulin44
On 2011/09/20 10:49:28, Bertrand Bordage wrote: Thanks for applying these! Sorry to bother you again with indentation, but you don't have to replace spaces don't have to == don't need to, I assume you mean you shouldn't with tabulators in scripts/musicxml2ly.ly . We decided that the rule

Re: T1780 remove scheme format calls with no destination parameter - deprecated in Guile V2 (issue 4974078)

2011-09-19 Thread ianhulin44
http://codereview.appspot.com/4974078/diff/3001/scm/document-identifiers.scm File scm/document-identifiers.scm (right): http://codereview.appspot.com/4974078/diff/3001/scm/document-identifiers.scm#newcode31 scm/document-identifiers.scm:31: On 2011/09/18 22:57:39, Bertrand Bordage wrote: Why a

Re: T1780 remove scheme format calls with no destination parameter - deprecated in Guile V2 (issue 4974078)

2011-09-19 Thread ianhulin44
New patch-set uploaded. Ian http://codereview.appspot.com/4974078/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Implement optional music function arguments (issue 5023044)

2011-09-15 Thread ianhulin44
Looks pretty cool, apart from some involved Scheme which I couldn't really unravel totally (see below). Will this patch allow us to get rid of the abomination of \afterGraceFraction by recasting \afterGrace to have an optional parameter \afterGrace {note} {gracenotes} [spacing-fraction] e.g. c1

Re: Introduce a maximum depth for markup evaluation (issue 5032041)

2011-09-15 Thread ianhulin44
http://codereview.appspot.com/5032041/diff/1/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/5032041/diff/1/scm/lily.scm#newcode125 scm/lily.scm:125: it will not terminate at all and print out a warning, but continue processing.) In the regression test the doc-text says

Re: Introduce a maximum depth for markup evaluation (issue 5032041)

2011-09-15 Thread ianhulin44
On 2011/09/15 20:37:07, Reinhold wrote: http://codereview.appspot.com/5032041/diff/1/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/5032041/diff/1/scm/lily.scm#newcode125 scm/lily.scm:125: it will not terminate at all and print out a warning, but continue processing.)

Re: Introduce a maximum depth for markup evaluation (issue 5032041)

2011-09-15 Thread ianhulin44
LGTM with revised doc-text in define-syntax-function. http://codereview.appspot.com/5032041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: change longas similarly to how breves were changed (issue 4962072)

2011-09-13 Thread ianhulin44
Mostly LGTM, apart from one calculation you do four times. Do it once once and save as a variable and use that. Cheers, Ian http://codereview.appspot.com/4962072/diff/1/mf/feta-noteheads.mf File mf/feta-noteheads.mf (right):

Re: change longas similarly to how breves were changed (issue 4962072)

2011-09-13 Thread ianhulin44
LGTM Ian http://codereview.appspot.com/4962072/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: include lines in breve X-extent (issue 1814) (issue 4931043)

2011-08-30 Thread ianhulin44
Janek, Bertrand posted some review comments here. I think it would be polite in the case of a newer contributor like Bertrand to post some responses one way or another (either don't worry about it, because. . . or nice catch, I'll upload an updated patch set.) Cheers, Ian

Re: include lines in breve X-extent (issue 1814) (issue 4931043)

2011-08-27 Thread ianhulin44
LGTM Patch applied and it fixes documents originally showing collisions between double-lined breves, bar-lines and accidentals. Cheers, Ian http://codereview.appspot.com/4931043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org

Re: Fix uninitialized variables when Source_file::get_counts returns early due to !contains (pos_str0) (issue 4940047)

2011-08-23 Thread ianhulin44
LGTM Maybe we should have some GOP rules for C++ about this? Only have multiple exit points from routines if you absolutely have to. Make sure any output parameters are declared and initialized at the top of a routine so that however a routine exits, they are left in a defined state Ian

Re: T1349 - Fix load order for running with Guile V2 (issue 4849054)

2011-08-22 Thread ianhulin44
Pushed to master with commit - f41963f0b6135c02c363f8401628e85b9bc60def Ian Hulin http://codereview.appspot.com/4849054/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: T1349 - Fix load order for running with Guile V2 (issue 4849054)

2011-08-14 Thread ianhulin44
Reinhold's recent commit for log-levels will require a git re-base and probably a merge for scm/lily.scm. Will do this, re-test and upload new patch-set. Ian http://codereview.appspot.com/4849054/ ___ lilypond-devel mailing list

Re: T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-11 Thread ianhulin44
http://codereview.appspot.com/4849054/diff/1/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/4849054/diff/1/scm/lily.scm#newcode116 scm/lily.scm:116: midi) On 2011/08/11 13:32:43, Ian Hulin (gmail) wrote: Sort out (increase) indentation for lines 115-116 Done.

Re: T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-11 Thread ianhulin44
New patch-set available for review. Ian http://codereview.appspot.com/4849054/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-11 Thread ianhulin44
On 2011/08/11 17:35:22, Neil Puttock wrote: On 2011/08/11 14:36:24, Ian Hulin (gmail) wrote: New patch-set available for review. Please rebase against master. Cheers, Neil Thanks for the catch. Rebased, scm/lily.scm merged and new patch-set uploaded. Cheers, Ian

Re: T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-11 Thread ianhulin44
http://codereview.appspot.com/4849054/diff/17001/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/4849054/diff/17001/scm/lily.scm#newcode324 scm/lily.scm:324: ( file-name-length 2) On 2011/08/11 20:40:46, Neil Puttock wrote: tab-space conversion has broken indentation here

Re: T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-11 Thread ianhulin44
Two more indentation problems in scm/lily.scm new patchset follows. Ian http://codereview.appspot.com/4849054/diff/8004/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/4849054/diff/8004/scm/lily.scm#newcode394 scm/lily.scm:394: (ly:version)) Needs indenting to line up

Re: T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-11 Thread ianhulin44
On 2011/08/11 23:16:59, Ian Hulin (gmail) wrote: Two more indentation problems in scm/lily.scm new patchset follows. Ian http://codereview.appspot.com/4849054/diff/8004/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/4849054/diff/8004/scm/lily.scm#newcode394

T1349 - Fix load order for running with Guile V2 (issue4849054)

2011-08-10 Thread ianhulin44
Reviewers: , Message: This patch adjusts the load order to one which works for both Guile V1.8.7 and Guile V2.0. The markup syntax macros have been moved from markup.scm to a new file markup-macros.scm, so these can be loaded before markup.scm. Attempting to move markup.scm without doing this

Re: T1780 - Guile V2 deprecation warnings re format calls with no dest parameter (issue4808063)

2011-07-31 Thread ianhulin44
On 2011/07/30 22:59:11, Reinhold wrote: I think that you forgot to rebase your branch to origin/master before uploading that patch. Your version does not have some of the latest patches in the git tree, so it appears that you are reverting some of those patches.

Re: Proper loglevels: cmd-line option --loglevel=NONE/ERROR/WARN/PROGRESS/INFO/DEBUG (issue4822055)

2011-07-30 Thread ianhulin44
FWIW LGTM apart from attached comments, I'm currently working on main.cc and lily.scm as part of T1686, and may need help with merging if this patch gets pushed first. Is your current design extensible to allow us to segment the old verbose option by function as well as debug trace level?

T1780 - Guile V2 deprecation warnings re format calls with no dest parameter (issue4808063)

2011-07-30 Thread ianhulin44
Reviewers: dak, Message: This patch removes all deprecation warnings about Guile format needing two parameters. This is the original patch as pushed by David, plus changes in ly/init.ly, ly/titling-init.ly and ly/graphviz_init.ly to use ly:format instead of format in embedded scheme statements.

Re: shortened flags: choosing appropriate flag (issue4410049)

2011-06-08 Thread ianhulin44
Spelling commenting and formatting changes only. Cheers, Ian http://codereview.appspot.com/4410049/diff/16001/input/regression/shortened-flags-cues.ly File input/regression/shortened-flags-cues.ly (right):

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2011-02-17 Thread ianhulin44
Hi Patrick, On 2011/02/17 06:50:21, Patrick McCarty wrote: Hi Ian, Please see my new comment regarding this patch (below). Thanks, Patrick http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm File scm/display-lily.scm (right):

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2011-02-17 Thread ianhulin44
http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm File scm/display-lily.scm (right): http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34 scm/display-lily.scm:34: On 2011/02/17 06:50:21, Patrick McCarty wrote: Jan recently removed all of the

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2011-02-17 Thread ianhulin44
: On 2011/02/17 15:07:00, ianhulin44 wrote: This block is the whole point of the patch and the tracker. Jan has just re-written code in display-lily.scm so it doesn't curry. If there's no currying in Lily Scheme code do we need this, or should we defend against users using currying their Scheme

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2011-02-17 Thread ianhulin44
New patchset uploaded to Rietveld Issue 2219044. Please review. Cheers, Ian http://codereview.appspot.com/2219044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2010-11-25 Thread ianhulin44
Another thought re the conditional (define-module) idea, if it's (if) making the guile compilation barf, we could try using (cond) or (cond-expand) instead. Ian http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm File scm/display-lily.scm (right):

Re: T1249 - Remove (define define-ly-syntax define-public). (issue2313044)

2010-11-05 Thread ianhulin44
http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/20001/scm/ly-syntax-constructors.scm#newcode38 scm/ly-syntax-constructors.scm:38: 'parser On 2010/11/04 23:50:25, Patrick

Re: T1249 - Remove (define define-ly-syntax define-public). (issue2313044)

2010-11-04 Thread ianhulin44
Comments actioned and tested with LilyPond using Guile V1.8.7 and V1.9.13. Ian http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (left): http://codereview.appspot.com/2313044/diff/1/scm/ly-syntax-constructors.scm#oldcode20

Re: T1249 - Remove (define define-ly-syntax define-public). (issue2313044)

2010-11-04 Thread ianhulin44
Patchset 4 uploaded partial restored to status as after T372 fix. http://codereview.appspot.com/2313044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: T1249 - Remove (define define-ly-syntax define-public). (issue2313044)

2010-11-04 Thread ianhulin44
http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.scm File scm/ly-syntax-constructors.scm (right): http://codereview.appspot.com/2313044/diff/15001/scm/ly-syntax-constructors.scm#newcode20 scm/ly-syntax-constructors.scm:20: (defmacro define-ly-syntax (args . body) On

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2010-10-27 Thread ianhulin44
http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode271 scm/lily.scm:271: (debug-enable 'debug) On 2010/10/20 21:58:07, Patrick McCarty wrote: On 2010/10/20 21:42:06, ianhulin44 wrote: This causes a deprecation warning from Guile V1.9.13 with $ declare -x GUILE_WARN_DEPRECATED

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2010-10-20 Thread ianhulin44
New patchset uploaded Ian http://codereview.appspot.com/2219044/diff/10001/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/2219044/diff/10001/scm/lily.scm#newcode231 scm/lily.scm:231: (_ module (ice-9 curried-definitions) not in Guile 1.8\n) Change --verbose

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

2010-10-20 Thread ianhulin44
OK to remove offending line even when using Guile V1.8.7? Ian http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm File scm/lily.scm (right): http://codereview.appspot.com/2219044/diff/15001/scm/lily.scm#newcode271 scm/lily.scm:271: (debug-enable 'debug) This causes a deprecation

Re: T1265 - Remove deprecation warnings when running with Guile V2 (issue2204044)

2010-10-18 Thread ianhulin44
http://codereview.appspot.com/2204044/diff/14001/flower/include/guile-compatibility.hh File flower/include/guile-compatibility.hh (right): http://codereview.appspot.com/2204044/diff/14001/flower/include/guile-compatibility.hh#newcode28 flower/include/guile-compatibility.hh:28: #define

  1   2   >