Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)

On 2020/01/30 09:54:48, hanwenn wrote: > On 2020/01/29 11:44:57, dak wrote: > > > BTW - I don't want to tell a C++ expert how to use the language > > > in general. If > > > we were in an alternate reality where we could start from scratch we could > >

Re: Grow heap aggressively during music interpretation (issue 561390043 by hanw...@gmail.com)

On 2020/02/01 20:49:09, hanwenn wrote: > On 2020/02/01 11:00:41, hahnjo wrote: > > > https://codereview.appspot.com/561390043/diff/557260051/lily/include/score-engraver.hh > > File lily/include/score-engraver.hh (right): > > > > > https://codereview.appspot.com/561390043/diff/557260051/lily/includ

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm File scm/lily.scm (right): https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode111 scm/lily.scm:111: "This defines a variable @var{name} with the starting value An interesting DOC string method... defin

Re: Grow heap aggressively during music interpretation (issue 561390043 by hanw...@gmail.com)

On 2020/02/01 21:23:51, hanwenn wrote: > On 2020/02/01 20:59:06, dak wrote: > > On 2020/02/01 20:49:09, hanwenn wrote: > > > On 2020/02/01 11:00:41, hahnjo wrote: > > > > > > > > > > https://codereview.appspot.com/561390043/diff/557260051/lily/inc

Remove check for rational bugfix. (issue 555230043 by hanw...@gmail.com)

LGTM We probably have quite a number of those leftovers that are completely irrelevant by now. I almost lean towards stipulating that any code for pre-Guile-1.8.8 can be removed directly without review, but then if something does go wrong by accident, we at least have a bit of a trace to see what

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

e} with the starting value > On 2020/02/01 21:37:50, dak wrote: > > An interesting DOC string method... define-syntax doesn't have anything like > > that? > > > no, doesn't look like it. > > I added all the GUILE repos I could find to cs.bazel.build, no

Fix SyntaxWarning's (issue 559440043 by jonas.hahnf...@gmail.com)

> However as far as I understand the MusicXML specification, a must always have either a , or , or . Wouldn't percussion notes generally be "unpitched"? What happens for such notes? https://codereview.appspot.com/559440043/

Re: Cast to Real in C++ style throughout (issue 547560044 by hanw...@gmail.com)

On 2020/02/02 18:51:08, Dan Eble wrote: > LGTM > > https://codereview.appspot.com/547560044/diff/555240043/lily/spring.cc > File lily/spring.cc (right): > > https://codereview.appspot.com/547560044/diff/555240043/lily/spring.cc#newcode119 > lily/spring.cc:119: avg_stretch /= static_cast (springs.

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

On 2020/02/03 09:49:32, hanwenn wrote: > On 2020/02/02 15:28:44, dak wrote: > > That's what I am trying right now, but you cannot quote outer functions as a > > value either (the problem is the value, not the innerness) so you need to do > it > > by name and I would

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

On 2020/02/03 12:11:08, dak wrote: > On 2020/02/03 09:49:32, hanwenn wrote: > > On 2020/02/02 15:28:44, dak wrote: > > > > That's what I am trying right now, but you cannot quote outer functions as a > > > value either (the problem is the value, not the

Re: Issue 5732: Use unique_ptr in layout code (issue 573500043 by nine.fierce.ball...@gmail.com)

Stupid question: unique_ptr has the purpose of deallocating memory when the last reference is gone. But we have an entire Scheme allocation system exactly for that purpose for which we are already paying the price in overhead. Any chance this can be usefully tracked in the SCM scheme of things?

Re: Issue 5732: Use unique_ptr in layout code (issue 573500043 by nine.fierce.ball...@gmail.com)

On 2020/02/03 18:13:00, hahnjo wrote: > On 2020/02/03 18:01:09, dak wrote: > > Stupid question: unique_ptr has the purpose of deallocating memory when the > last > > reference is gone. But we have an entire Scheme allocation system exactly for > > that purpose for which w

Re: Issue 5732: Use unique_ptr in layout code (issue 573500043 by nine.fierce.ball...@gmail.com)

On 2020/02/03 18:27:27, dak wrote: > On 2020/02/03 18:13:00, hahnjo wrote: > > On 2020/02/03 18:01:09, dak wrote: > > > Stupid question: unique_ptr has the purpose of deallocating memory when the > > last > > > reference is gone. But we have an entire Schem

Re: Issue 5705: int->long in Stem::get_beaming and set_beaming (issue 561350044 by nine.fierce.ball...@gmail.com)

On 2020/02/03 21:33:11, Dan Eble wrote: > Next try: http://codereview.appspot.com/565610043 It's probably more an academical remark, but a "kosher" way of doing that might be using scm_to_int (scm_length (...)) instead of scm_ilength (...). This will take out the length in the form it has been pr

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

On 2020/02/04 08:56:49, hanwenn wrote: > On 2020/02/03 12:11:08, dak wrote: > > > What is wrong with the code I propose in this change? > > > > You mean define-syntax? I'd like to avoid it for now since people have no way > > of getting acquainted with it,

Fix convert-ly with Python 3 (issue 555260044 by jonas.hahnf...@gmail.com)

Foreseeable consequences for Python 2.7? https://codereview.appspot.com/555260044/

Re: Fix convert-ly with Python 3 (issue 555260044 by jonas.hahnf...@gmail.com)

https://codereview.appspot.com/555260044/diff/567150044/scripts/convert-ly.py File scripts/convert-ly.py (right): https://codereview.appspot.com/555260044/diff/567150044/scripts/convert-ly.py#newcode361 scripts/convert-ly.py:361: f = f That line looks spurious. Any reason for keeping it in? ht

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

NLGTM This patch just punts on using an internal function, and that could be equally well be done without involving syntax-case. An alternative proposal that just relies on a single external symbol in order to achieve the original design is given as Tracker issue: 5735 (https://sourceforge.net/p

Re: Add Code of Conduct (issue 575620043 by janek.lilyp...@gmail.com)

What problem are we trying to solve here? https://codereview.appspot.com/575620043/

Re: Add a tentative .clang-format for LilyPond. (issue 561340043 by hanw...@gmail.com)

On 2020/02/04 22:18:23, hanwenn wrote: > On 2020/02/04 20:35:40, Dan Eble wrote: > > I'm running some of my patches through clang-format as I prepare to push them. > > > > This is an example of a kind of change it wants to make: > > > > - const array key {column_rank, dir}; > > + const array ke

Re: Issue 5739: Add makefile targets for formatting all C++ code (issue 565620043 by nine.fierce.ball...@gmail.com)

https://codereview.appspot.com/565620043/diff/549530043/GNUmakefile.in File GNUmakefile.in (right): https://codereview.appspot.com/565620043/diff/549530043/GNUmakefile.in#newcode319 GNUmakefile.in:319: ## TODO: This condition speeds up make when formatting targets are not I would not really work

Re: Make define-builtin-markup{,-list}-command #:category #:properties keywords (issue160048)

On 2009/12/05 10:52:52, nicolas.sceaux wrote: Le 5 déc. 2009 à 09:48, mailto:d...@gnu.org a écrit : > Currently I don't see a better way forward than using a special command > line option, something like -dindex-markup on documentation runs. > If I get no objections, I'll try making a patch

Re: Fix #887: Use ly:string-percent-encode for textedit URIs. (issue193077)

http://codereview.appspot.com/193077/diff/1001/12 File lily/general-scheme.cc (right): http://codereview.appspot.com/193077/diff/1001/12#newcode230 lily/general-scheme.cc:230: return ((c >= 0x2D && c <= 0x2F) // hyphen, full-stop, and forward-slash On 2010/01/25 06:38:18, lemzwerg wrote: Wouldn

Re: Instanciable scheme engraver (issue216066)

The use of modules oop and goops appears to me as part of a programming practice at a different knowledge level than to be expected from the audience of this example. If I rewrite the example to get along without those modules, are there chances that the results will get accepted? http://coderev

Re: Change lilypond-book's LaTeX environment option placement (issue813048)

Reviewers: carl.d.sorensen_gmail.com, Message: Amended. http://codereview.appspot.com/813048/diff/5001/6001 File Documentation/changes.tely (right): http://codereview.appspot.com/813048/diff/5001/6001#newcode72 Documentation/changes.tely:72: @example On 2010/04/20 13:02:15, Carl wrote: I thin

Re: Don't hardcode a limited set of markup signatures. (issue969046)

Reviewers: hanwenn, http://codereview.appspot.com/969046/diff/2001/3002 File lily/lexer.ll (right): http://codereview.appspot.com/969046/diff/2001/3002#newcode542 lily/lexer.ll:542: yylval.scm = scm_car(s); On 2010/05/01 17:13:32, hanwenn wrote: can you document the contents of s in a comment

Re: Don't hardcode a limited set of markup signatures. (issue969046)

http://codereview.appspot.com/969046/diff/7001/8002 File lily/lexer.ll (right): http://codereview.appspot.com/969046/diff/7001/8002#newcode545 lily/lexer.ll:545: // loop will be EXPECT_NO_MORE_ARGS. On 2010/05/01 19:56:08, hanwenn wrote: wouldnt it be clearer to have a function void trans

Re: Don't hardcode a limited set of markup signatures. (issue969046)

On 2010/05/02 16:34:12, hanwenn wrote: On Sun, May 2, 2010 at 8:04 AM, wrote: > > http://codereview.appspot.com/969046/diff/7001/8002 > File lily/lexer.ll (right): > > http://codereview.appspot.com/969046/diff/7001/8002#newcode545 > lily/lexer.ll:545: // loop will be EXPECT

Re: Don't hardcode a limited set of markup signatures. (issue969046)

Another patch set has been made that removes the order restrictions on the markup function arguments and consequently throws out all the code for which change suggestions or commenting or refactoring requests have been made. Most changes in the patch set provide a significant reduction in (quite

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

> Note that this description isn't correct anymore. This patch doesn't actually prevent those functions from segfaulting but adds a new interface ly:parser-include-string. I think ly:parser-parse-string and ly:parser-parse-file should actually be banned in .ly files. If ly:parser-include-strin

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

http://codereview.appspot.com/1908041/diff/2001/3002 File mf/feta-accordion.mf (right): http://codereview.appspot.com/1908041/diff/2001/3002#newcode117 mf/feta-accordion.mf:117: fet_beginchar ("accordion register freebass", "freebass") On 2010/07/28 17:11:20, Carl wrote: I made these changes, a

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

Reviewers: carl.d.sorensen_gmail.com, Message: On 2010/07/28 17:11:20, Carl wrote: Looks good, with the exception of the version being wrong for the convert rule. Care to elaborate? I have no idea how to do this correctly, and I believe that convert-ly should somehow cater for notation manual

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

On 2010/07/29 19:29:39, Neil Puttock wrote: http://codereview.appspot.com/1908041/diff/2001/3003 File python/convertrules.py (right): http://codereview.appspot.com/1908041/diff/2001/3003#newcode3044 python/convertrules.py:3044: str = re.sub (r'(\\musicglyph\s*#"accordion\.)([a-zA-Z]+)"', Could

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

On 2010/07/29 17:43:37, Graham Percival wrote: On 2010/07/29 13:01:47, c_sorensen_byu.edu wrote: > Right now you have put these conversions into a separate rule. Add them to > the 2.13.29 rule. Then I think you have to do make to get the rules into > convert-ly, but I'm not sure on that.

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

http://codereview.appspot.com/1908041/diff/2001/3003 File python/convertrules.py (right): http://codereview.appspot.com/1908041/diff/2001/3003#newcode3044 python/convertrules.py:3044: str = re.sub (r'(\\musicglyph\s*#"accordion\.)([a-zA-Z]+)"', On 2010/07/29 19:29:40, Neil Puttock wrote: Could

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

On 2010/07/29 20:14:57, Neil Puttock wrote: On 2010/07/29 20:05:30, dak wrote: > I don't think that convert-ly is supposed to deal with Scheme markup conversion. > Searching for #: in the convertrules turns up exactly nothing. Why would it? You don't need that to catch

Re: feta-accordion.mf, convertrules.py, musicxml2ly.py: sanitize accordion symbol names (issue1908041)

On 2010/08/01 14:27:05, Neil Puttock wrote: http://codereview.appspot.com/1908041/diff/10002/18002 File python/convertrules.py (right): http://codereview.appspot.com/1908041/diff/10002/18002#newcode3014 python/convertrules.py:3014: 'accDot': 'dot', indent http://codereview.appspot.com/19080

Re: convert: properly escape some single-backslashes. (issue2401042)

http://codereview.appspot.com/2401042/diff/1/python/convertrules.py File python/convertrules.py (right): http://codereview.appspot.com/2401042/diff/1/python/convertrules.py#newcode2992 python/convertrules.py:2992: _ ("\\RemoveEmpty*StaffContext -> \*Staff \\RemoveEmptyStaves")) Any particular re

Re: convert: properly escape some single-backslashes. (issue2401042)

http://codereview.appspot.com/2401042/diff/1/python/convertrules.py File python/convertrules.py (right): http://codereview.appspot.com/2401042/diff/1/python/convertrules.py#newcode2992 python/convertrules.py:2992: _ ("\\RemoveEmpty*StaffContext -> \*Staff \\RemoveEmptyStaves")) Any particular re

Re: which-page (issue 6352049)

A feature should be documented, or it will not be discoverable. If feasible, a regtest should be added in input/regression. Whether this is possible for essentially multi-page features, I don't know, so it is possible that this particular feature does not really lend itself to a regtest. http:

Re: Doc: document \on-the-fly (2579) (issue 6347062)

http://codereview.appspot.com/6347062/diff/1002/Documentation/notation/input.itely File Documentation/notation/input.itely (right): http://codereview.appspot.com/6347062/diff/1002/Documentation/notation/input.itely#newcode1017 Documentation/notation/input.itely:1017: @item (on-page nmbr) @ta

Doc: more comprehensive documentation of layout block (2560) (issue 6354085)

http://codereview.appspot.com/6354085/diff/1/Documentation/notation/spacing.itely File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/6354085/diff/1/Documentation/notation/spacing.itely#newcode1182 Documentation/notation/spacing.itely:1182: to structure the layout de

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

I actually only wanted to get rid of the whitespace errors plagueing every test of this patch, but one programming error jumped out so badly at me that I had to flag it as well. http://codereview.appspot.com/5626052/diff/79059/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right

Re: Doc: Update changing context default settings (2322) (issue 6345086)

http://codereview.appspot.com/6345086/diff/1/Documentation/notation/changing-defaults.itely File Documentation/notation/changing-defaults.itely (right): http://codereview.appspot.com/6345086/diff/1/Documentation/notation/changing-defaults.itely#newcode698 Documentation/notation/changing-defaults

Re: Doc: Update changing context default settings (2322) (issue 6345086)

http://codereview.appspot.com/6345086/diff/5001/Documentation/notation/changing-defaults.itely File Documentation/notation/changing-defaults.itely (right): http://codereview.appspot.com/6345086/diff/5001/Documentation/notation/changing-defaults.itely#newcode844 Documentation/notation/changing-de

Re: Doc: Update changing context default settings (2322) (issue 6345086)

-defaults.itely#newcode844 Documentation/notation/changing-defaults.itely:844: \override Stem #'thickness = #4.0 On 2012/07/11 21:47:58, dak wrote: > That overrides Stem thickness in all Bottom contexts. > It seems unlikely that a single setting will be nice > for MensuralVoice, TabVoice a

Re: Doc: Update changing context default settings (2322) (issue 6345086)

> Documentation/notation/changing-defaults.itely:844: \override Stem > #'thickness = #4.0 >> On 2012/07/11 21:47:58, dak wrote: > >> > That overrides Stem thickness in all Bottom contexts. >> > It seems unlikely that a single setting will be nice >> > for

Re: Doc: Update changing context default settings (2322) (issue 6345086)

On 2012/07/12 10:44:04, Trevor Daniels wrote: On 2012/07/12 10:20:01, dak wrote: > Uhm, wrong? [etc] Thanks David - that's the explanation I needed :) I am not happy with all those fine distinctions and almost, but not quite, similar syntax for property manipulations in d

Re: parser.yy: remove `fraction' (issue 6399045)

Reviewers: Keith, Message: On 2012/07/14 21:29:42, Keith wrote: It is too unfriendly to say "unexpected '/'" to someone who wrote {\times 2/ 3 { b2 b b }}. We need at least a conversion rule like "(\d)\s*/\s*(\d)" => "\1/\2" In the existing code base, that would reach the following: input/

Re: Check in Texinfo 2012-07-03.16 (issue 6374068)

Reviewers: Graham Percival, Message: On 2012/07/15 01:45:06, Graham Percival wrote: I can't see any diff on rietveld, but presumably there's so much being changed that a diff wouldn't be helpful anyway. If a "make doc" looks good to you, and you're willing to handle any potential fall-out

Re: Fix Issue 2146 "Illegal entry in bfrange block in ToUnicode CMap" (issue 6399046)

http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itexi File Documentation/common-macros.itexi (right): http://codereview.appspot.com/6399046/diff/1/Documentation/common-macros.itexi#newcode16 Documentation/common-macros.itexi:16: % code stolen from Heiko Oberdiek's `ifpdf

Re: Fix Issue 2146 "Illegal entry in bfrange block in ToUnicode CMap" (issue 6399046)

http://codereview.appspot.com/6399046/diff/1/configure.in File configure.in (right): http://codereview.appspot.com/6399046/diff/1/configure.in#newcode66 configure.in:66: _NCSB_SOURCE_FILES_="" On 2012/07/15 00:37:15, John Mandereau wrote: On 2012/07/15 00:21:16, Graham Percival wrote: > what's

Re: parser.yy: remove `fraction' (issue 6399045)

message: On 2012/07/15 05:07:13, Keith wrote: On Sat, 14 Jul 2012 19:14:41 -0700, wrote: > Do we have evidence of people using 2/ 3 anywhere? I don't know of any scores with spaces alongside the / . As far as I know, it has never been documented or demonstrated. The

Re: parser.yy: remove `fraction' (issue 6399045)

On 2012/07/15 09:14:00, dak wrote: message: On 2012/07/15 05:07:13, Keith wrote: > On Sat, 14 Jul 2012 19:14:41 -0700, <mailto:d...@gnu.org> wrote: > > > Do we have evidence of people using 2/ 3 anywhere? > > I don't know of any scores with spaces alongside the /

Re: Volta enhancements tranche 1 (issue 6398055)

http://codereview.appspot.com/6398055/diff/1/Documentation/notation/rhythms.itely File Documentation/notation/rhythms.itely (right): http://codereview.appspot.com/6398055/diff/1/Documentation/notation/rhythms.itely#newcode2676 Documentation/notation/rhythms.itely:2676: Additionally there is an @

Re: Check in Texinfo 2012-07-03.16 (issue 6374068)

On 2012/07/18 12:40:12, lemzwerg wrote: I'm currently in Japan, with bad internet access (and additionally, I can only use the web interface for writing emails, which I heartily dislike, since SMTP doesn't work here in the hotel for yet unknown reasons), so it will take some time until I'm

Re: CG: clarify staging branch policy (issue 6425045)

Other than that, LGTM. http://codereview.appspot.com/6425045/diff/1/Documentation/contributor/administration.itexi File Documentation/contributor/administration.itexi (right): http://codereview.appspot.com/6425045/diff/1/Documentation/contributor/administration.itexi#newcode280 Documentation/co

Re: reopened Issue 2584: please make partcombine merge slurs (issue 6432047)

Reviewers: Trevor Daniels, Message: On 2012/07/19 09:30:15, Trevor Daniels wrote: Just nitpicking; can't comment substantively. Trevor http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc File lily/phrasing-slur-engraver.cc (right): http://codereview.appspot.com/

Re: reopened Issue 2584: please make partcombine merge slurs (issue 6432047)

On 2012/07/19 17:12:21, Keith wrote: LGTM Again, I suggest first committing the code in a state that merely fixes the reported bugs about warnings, and then a commit to add the new capability to set two slurs if they have opposite directions. Again, I state that the "new capability" is a si

Re: reopened Issue 2584: please make partcombine merge slurs (issue 6432047)

On 2012/07/19 17:52:13, Keith wrote: On 2012/07/19 17:25:45, dak wrote: > On 2012/07/19 17:12:21, Keith wrote: > > Again, I suggest first committing the code in a state that > > merely fixes the reported bugs about warnings, > One could remove line 264 in slur.cc Sim

Re: Fix Issue 2146 "Illegal entry in bfrange block in ToUnicode CMap" (issue 6399046)

On 2012/07/20 08:48:32, Graham Percival wrote: LGTM I am somewhat concerned (meaning that I have no clue whatsoever, but a queasy feeling) about the implications for running/installing LilyPond in various directories. Will this do the right thing for testing with and without separate build dir

Re: Fix Issue 2146 "Illegal entry in bfrange block in ToUnicode CMap" (issue 6399046)

On 2012/07/20 09:34:09, John Mandereau wrote: I propose to fix the issue w.r.t. LilyPond doc build, closing issue 2146, and not fuss with distributing lilypond.map until I can investigate this with help of experts like you and Werner. Ah ok, I was not aware that this was a doc-build only p

Re: Issue 1320: Scheme bar line interface (issue 6305115)

On 2012/07/20 10:15:41, marc wrote: I think the differences concerning MIDI output can safely be ignored, What makes it safe to ignore them? http://codereview.appspot.com/6305115/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists

Re: Issue 1320: Scheme bar line interface (issue 6305115)

http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm File scm/bar-line.scm (right): http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode649 scm/bar-line.scm:649: Empty line at end of file makes git complain about whitespace error. http://codereview.appspot.com/

Re: Second set of volta changes (Issue 2673) (issue 6422061)

http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly File ly/engraver-init.ly (right): http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly#newcode621 ly/engraver-init.ly:621: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|" Trailing whitespace. http://codereview.appspot.c

Re: Second set of volta changes (Issue 2673) (issue 6422061)

On 2012/07/24 19:40:35, email_philholmes.net wrote: - Original Message - From: To: ; ; Cc: ; Sent: Tuesday, July 24, 2012 8:15 PM Subject: Re: Second set of volta changes (Issue 2673) (i

Re: Syntax change: REAL requires at least one digit before decimal point (issue 6446048)

Reviewers: Keith, Message: On 2012/07/26 02:37:19, Keith wrote: Oh, I don't know. It's only mildly annoying, but I don't see what good it does. What else could ".3" be besides a number ? A chord step. But chordmode is pretty awful anyway. I agree that syntactically 4. is quite the more

Issue 1650: Multiple header blocks shall be merged rather than replace a previous one (issue 6441058)

http://codereview.appspot.com/6441058/diff/1/lily/parser.yy File lily/parser.yy (right): http://codereview.appspot.com/6441058/diff/1/lily/parser.yy#newcode932 lily/parser.yy:932: /* Copy the new header fields, i.e. overwrite existing, but preserve unchanged ones */ I think this is the wrong way

Re: Issue 1650: Multiple header blocks shall be merged rather than replace a previous one (issue 6441058)

On 2012/07/27 05:09:22, dak wrote: http://codereview.appspot.com/6441058/diff/1/lily/parser.yy File lily/parser.yy (right): http://codereview.appspot.com/6441058/diff/1/lily/parser.yy#newcode932 lily/parser.yy:932: /* Copy the new header fields, i.e. overwrite existing, but preserve

Re: Issue 1650: Multiple header blocks shall be merged rather than replace a previous one (issue 6441058)

http://codereview.appspot.com/6441058/diff/1/lily/parser.yy File lily/parser.yy (right): http://codereview.appspot.com/6441058/diff/1/lily/parser.yy#newcode993 lily/parser.yy:993: /* Copy the new header fields, i.e. overwrite existing, but preserve unchanged ones */ The problem with this approac

Properly implement fromproperty markup handing in the pdftitle header field (issue 6447052)

http://codereview.appspot.com/6447052/diff/1/scm/markup.scm File scm/markup.scm (right): http://codereview.appspot.com/6447052/diff/1/scm/markup.scm#newcode82 scm/markup.scm:82: (define-public (markup->string props m) This changes the interface of a public function in an incompatible manner. Th

Re: Set indent based on instrument name (issue 6457049)

On 2012/07/29 18:20:16, PhilEHolmes wrote: My first attempt at coding in LilyPond. Treat me gently... I think we should be able to do better than just counting characters in a string. It should be possible calculating physical string width, shouldn't it? http://codereview.appspot.com/6457049

Re: Issue 1650: merge multiple header specifications. (issue 6445053)

Reviewers: Graham Percival, Message: On 2012/07/30 14:35:05, Graham Percival wrote: LGTM, and I really like the comments in the regtests. Not me who can claim credit. In a few instances they were slightly unclear, though. I did not even bother looking at them. You'll probably tear your ha

Re: Unify the lexer's idea of words and commands across all modes. (issue 6445056)

Reviewers: Trevor Daniels, Message: On 2012/07/31 08:47:53, Trevor Daniels wrote: Just a query really, to help my understanding. Trevor http://codereview.appspot.com/6445056/diff/1/lily/lexer.ll File lily/lexer.ll (right): http://codereview.appspot.com/6445056/diff/1/lily/lexer.ll#newco

Re: Set indent based on instrument name (issue 6457049)

http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc File lily/output-def.cc (right): http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc#newcode275 lily/output-def.cc:275: set_inst_name_len (Real long_inst_name_len, Real short_inst_name_len) Correct me if I am wrong

Re: Unify the lexer's idea of words and commands across all modes. (issue 6445056)

On 2012/07/31 16:56:51, t.daniels_treda.co.uk wrote: wrote Tuesday, July 31, 2012 11:52 AM >> http://codereview.appspot.com/6445056/diff/1/lily/lexer.ll#newcode390 >> lily/lexer.ll:390: {RESTNAME}/[-_] | >> Why is this trailing context added? I don't see >> what this w

Re: Remove weird quotes in \set "instrumentCueName" (issue 6449082)

Reviewers: Trevor Daniels, Message: On 2012/08/02 18:40:56, Trevor Daniels wrote: LGTM Uhm yes. This does not really warrant a review, I just forgot pushing after it passed Patchy. Description: Remove weird quotes in \set "instrumentCueName" Please review this at http://codereview.appspot.c

Re: Doc: Clarify automatic beam setting (2701) (issue 6452072)

On 2012/08/03 09:30:55, Graham Percival wrote: LGTM although I'm not wild about the @i{} bits, I can't immediately think of a better alternative. The obvious way would be to reorganize into fewer layers. Nobody is going to keep track of that many anyway. That @i{} stuff is not really good:

Re: Unify the lexer's idea of words and commands across all modes. (issue 6445056)

On 2012/08/04 06:34:57, Keith wrote: I'm pouting a little that this moves further from the ability to write \violin1mvt3a = { c d } but it still looks fine to me. While I agree that allowing digits into identifiers would not make sense unless one gets them allowed in all modes (or the whole

Re: Set indent based on instrument name (issue 6457049)

On 2012/08/04 07:28:29, Keith wrote: On 2012/08/01 06:45:22, MikeSol wrote: > Avoid measuring extents when engraving is happening because they could be > dependent on other callbacks which could trigger many layout decisions before > engraving is finished. > This is a case where measuri

Re: Doc: Clarify automatic beam setting (2701) (issue 6452072)

On 2012/08/04 08:06:38, Trevor Daniels wrote: On 2012/08/03 09:35:47, dak wrote: > The obvious way would be to reorganize into fewer layers. No. This is a long and complicated subsubsec Reorganizing into fewer layers would mean reorganizing the containing structure so that we do

Re: Doc: Clarify automatic beam setting (2701) (issue 6452072)

On 2012/08/04 09:20:57, Trevor Daniels wrote: On 2012/08/04 08:27:10, dak wrote: > But again: the real issue to me seems that we are > hiding a long chapter inside of a subsubsec already. I agree. But to fix that would require rewriting pretty well the whole of chapter 1 of the NR

Re: Doc: Clarify automatic beam setting (2701) (issue 6452072)

On 2012/08/04 15:11:55, Trevor Daniels wrote: On 2012/08/04 09:33:33, dak wrote: I'm sorry - I completely missed this. Probably because I was hung up by the earlier sneer and didn't take in much else. Since you ask, the bit that affected me was > A heading indented as o

Re: Properly implement fromproperty markup handing in the pdftitle header field (issue 6447052)

http://codereview.appspot.com/6447052/diff/4001/scm/markup.scm File scm/markup.scm (right): http://codereview.appspot.com/6447052/diff/4001/scm/markup.scm#newcode109 scm/markup.scm:109: (define (markup-cons->string-cons c props) "props" seems like a total misnomer: that is usually used for a lis

Re: Properly implement fromproperty markup handing in the pdftitle header field (issue 6447052)

On 2012/08/05 23:26:46, reinhold_kainhofer.com wrote: On 2012-08-06 00:44, mailto:d...@gnu.org wrote: > > http://codereview.appspot.com/6447052/diff/4001/scm/markup.scm > File scm/markup.scm (right): > > http://codereview.appspot.com/6447052/diff/4001/scm/markup.scm#newcode109 > scm/markup.scm:

Re: Fix for beaming in Kievan notation (issue 6305080)

http://codereview.appspot.com/6305080/diff/4001/lily/beam.cc File lily/beam.cc (right): http://codereview.appspot.com/6305080/diff/4001/lily/beam.cc#newcode636 lily/beam.cc:636: extract_grob_set (stems[0], "note-heads", heads); This looks like the wrong place to do this calculation. You overwri

Re: Second set of volta changes (Issue 2673) (issue 6422061)

On 2012/08/08 08:28:32, email_philholmes.net wrote: - Original Message - > ly/engraver-init.ly:622: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|" > trim trailing whitespace before pushing if you remember > > http://codereview.appspot.com/6422061/ I didn't trim the whitespace beca

Re: Doc: Clarify automatic beam setting (2701) (issue 6452072)

On 2012/08/08 18:58:08, Trevor Daniels wrote: On 2012/08/07 21:19:00, Trevor Daniels wrote: > Leaving open until pdf has been checked In the pdf all the headings below subsection are in the same typeface, so it makes no difference to the pdf what we do for these level 5 headings. In html

Brain surgery on the build system, first stage (issue 6446096)

http://codereview.appspot.com/6446096/diff/2001/aclocal.m4 File aclocal.m4 (right): http://codereview.appspot.com/6446096/diff/2001/aclocal.m4#newcode1350 aclocal.m4:1350: +if test "$CYGWIN$MINGW32" = "nono"; then This does not look like aclocal.m4 at all, more like the output of git log -p.

Re: When cloning a lexer/parser, don't copy the scopes. (issue 6460064)

Reviewers: Trevor Daniels, http://codereview.appspot.com/6460064/diff/4001/lily/lily-parser.cc File lily/lily-parser.cc (right): http://codereview.appspot.com/6460064/diff/4001/lily/lily-parser.cc#newcode160 lily/lily-parser.cc:160: // TODO: use $parser On 2012/08/10 06:58:57, Trevor Daniels wr

Re: Doc: Clarify automatic beam setting (2701) (issue 6452072)

On 2012/08/10 07:54:51, Trevor Daniels wrote: On 2012/08/09 07:36:46, Trevor Daniels wrote: > Changed level 5 headings to @subsubheading @i{ .. } These look fine to me in both pdf and html, so I'm closing this review. I'll open a new issue tracker to cover using level 5 headings uniformly.

Re: When cloning a lexer/parser, don't copy the scopes. (issue 6460064)

, dak wrote: > On 2012/08/10 06:58:57, Trevor Daniels wrote: > > Either this comment should be removed, or the > > earlier one should be left in. Sorry, I don't > > understand enough of this to know which. > > There is not enough context to figure out what the author o

Re: When cloning a lexer/parser, don't copy the scopes. (issue 6460064)

On 2012/08/10 16:20:30, hanwenn wrote: On 2012/08/10 16:16:47, dak wrote: > > and #{ ... #} looks for "parser", and you don't want to change the name of the > first argument of music functions. the first name of the music function argument is irrelevant, as it

Fix 2732: Extract the full page geometry in lilypond-book (issue 6454139)

http://codereview.appspot.com/6454139/diff/1/python/book_latex.py File python/book_latex.py (right): http://codereview.appspot.com/6454139/diff/1/python/book_latex.py#newcode262 python/book_latex.py:262: textwidth = (textwidth + columnsep) / columns - columnsep Why not use the value of \columnwi

Re: Issue 2728: count pairs of brackets (issue 6450114)

Reviewers: Graham Percival, Message: On 2012/08/12 14:32:01, Graham Percival wrote: LGTM, and wow it's amazing that such an improvement in usability can come from so few lines of code! It is probably even more amazing for how long people put up with the old behavior. Description: Issue 2728

Re: Fix Issue 2366 "THANKS needs updating or deleting" (issue 6454140)

You have taken the path of removing THANKS altogether if I understand correctly. I am not sure whether we should not replace it by the edited output of git shortlog -s release/2.14.2-1..release/2.15.95-1 instead, manually. That would cause the least disruption to the code. At any rate, regardin

Re: Add support for Cyrillic in texinfo. (issue 6459081)

http://codereview.appspot.com/6459081/diff/1/Documentation/cyrillic.itexi File Documentation/cyrillic.itexi (right): http://codereview.appspot.com/6459081/diff/1/Documentation/cyrillic.itexi#newcode14 Documentation/cyrillic.itexi:14: \gdef\cyrfont{% The lack of kerning seems quite unnecessary.

Re: Add support for Cyrillic in texinfo. (issue 6459081)

http://codereview.appspot.com/6459081/diff/1/Documentation/macros.itexi File Documentation/macros.itexi (right): http://codereview.appspot.com/6459081/diff/1/Documentation/macros.itexi#newcode14 Documentation/macros.itexi:14: @include cyrillic.itexi On 2012/08/13 19:37:26, lemzwerg wrote: It's

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

On 2012/08/14 02:45:39, Keith wrote: Needs a tweak for \endSpanners http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly File ly/music-functions-init.ly (right): http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly#newcode296 ly/music-function

Properties to control placement of accidentals in KeySignatures (issue 6461085)

Will the added generality be enough? Should we not try to come up with some sort of exception list instead, like we do for mapping chords to fretboards? I think the principal aim should be to be able to avoid having to write overrides in the music itself: changing key/scale and clef should be al

<    1   2   3   4   5   6   7   8   9   10   >