Re: Avoid failed assertion in stem tremolo code (issue 572550043 by d...@gnu.org)

https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc File lily/stem-tremolo.cc (left): https://codereview.appspot.com/572550043/diff/560590043/lily/stem-tremolo.cc#oldcode175 lily/stem-tremolo.cc:175: Interval ph = stem->pure_y_extent (stem, 0, INT_MAX); My suggested cha

Avoid failed assertion in stem tremolo code (issue 572550043 by d...@gnu.org)

If I understand, the example \relative { a32 8..:32 } fails the assertion in the call to center() on line 182, because the interval 'ph' is empty. There are several problems in the existing code, that would seem to prevent it from working. The proposed change deviates further from what seems t

Re: Beautify Grob_array and stop using std::vector::data() (issue 264950043 by nine.fierce.ball...@gmail.com)

On 2015/09/03 08:49:13, dak wrote: On 2015/09/02 23:27:41, Dan Eble wrote: > If the sorting criterion uses attributes of the Grob that are changed by > replacement, the array might not be sorted afterward (or might be sorted when it > wasn't before). Grob_array does not appear to dictate

Re: Remove redundant occurences of this-> (issue 258870043 by d...@gnu.org)

https://codereview.appspot.com/258870043/diff/1/lily/grob.cc File lily/grob.cc (right): https://codereview.appspot.com/258870043/diff/1/lily/grob.cc#newcode232 lily/grob.cc:232: /* This version of get_system is more reliable than get_system () What do you mean by "get_system is more reliable tha

Re: Doc: Glossary - add notes for High Bass Clef (issue 256090043 by pkx1...@gmail.com)

https://codereview.appspot.com/256090043/diff/20001/Documentation/music-glossary.tely File Documentation/music-glossary.tely (right): https://codereview.appspot.com/256090043/diff/20001/Documentation/music-glossary.tely#newcode4309 Documentation/music-glossary.tely:4309: Since the 18th century t

Remove redundant occurences of this-> (issue 258870043 by d...@gnu.org)

Why bother ? It is probably not a good idea to remove the this-> where it was used in comments, particularly to distinguish the function() in virtual-function table for the object from function() in a parent class. https://codereview.appspot.com/258870043/ ___

Re: Doc: avoid implicit \relative; issue 4371 (issue 237340043 by k-ohara5...@oco.net)

The part of notation.pdf before the tables grows from 613 to 622 pages, 2%, even with the closing }s always on their own line. https://codereview.appspot.com/237340043/diff/110001/Documentation/learning/common-notation.itely File Documentation/learning/common-notation.itely (right): https://cod

Re: Avoid treating x as relative in \relative { \autochange {x} } (issue 250170043 by nine.fierce.ball...@gmail.com)

\relative does its job just fine on the output of \autochange and \partcombine. The only problem is that \absolute and \partcombine assume they have been given the final pitches, so applying relative afterwards messes up the work of \autochange and \partcombine. So \autochange and \partcombine d

Doc: Issue 4059: Document MIDI mapping and MIDI effects (issue 249980043 by tdanielsmu...@googlemail.com)

Sorry I'm late. You might still slip in a correction, or just remove mention of midiChannelMapping='voice since we haven't found a use for it. https://codereview.appspot.com/249980043/diff/1/Documentation/notation/input.itely File Documentation/notation/input.itely (right): https://codereview.a

Re: Doc: avoid implicit \relative; issue 4371 (issue 237340043 by k-ohara5...@oco.net)

https://codereview.appspot.com/237340043/diff/110001/Documentation/learning/tweaks.itely File Documentation/learning/tweaks.itely (right): https://codereview.appspot.com/237340043/diff/110001/Documentation/learning/tweaks.itely#newcode330 Documentation/learning/tweaks.itely:330: b c } On 2015/06

Re: Doc: avoid implicit \relative; issue 4371 (issue 237340043 by k-ohara5...@oco.net)

I started this with the belief that relative octave notation is easier to write (for some people) but absolute octave notation is easier to read, and thus the expectation that many examples would be simpler using absolute pitches, or the new \fixed c'' {}. However, there were not so many examples

Re: Make music functions callable from Scheme (issue 244840043 by d...@gnu.org)

LGTM The comments make it make sense. https://codereview.appspot.com/244840043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Make music functions callable from Scheme (issue 244840043 by d...@gnu.org)

It bothered me that I said 'LGTM' without figuring out the logic. I don't understand it. https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc File lily/music-function.cc (right): https://codereview.appspot.com/244840043/diff/80001/lily/music-function.cc#newcode63 lily/mus

Re: Make music functions callable from Scheme (issue 244840043 by d...@gnu.org)

https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/244840043/diff/80001/Documentation/changes.tely#newcode68 Documentation/changes.tely:68: as if they were proper Scheme functions. Argument checki

Re: Make music functions callable from Scheme (issue 244840043 by d...@gnu.org)

LGTM I gather that Scheme fluids are variables with global scope but distinct storage for each Scheme thread, so they can be used to reference to the specific parser (if any) in use by the thread that called the music function. https://codereview.appspot.com/244840043/diff/80001/Documentation/c

Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)

lgtm, if you can narrow the convert-ly rule a bit and test it (the semi-automated 'patchy' testing doesn't use convert-ly) Also, I think you need to add Documentation/snippets/new/blanking-staff-lines-using-the--whiteout-command.ly and let the makelsr script update the copy in /snippets/, but I'm

Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)

https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py File python/convertrules.py (right): https://codereview.appspot.com/236480043/diff/40001/python/convertrules.py#newcode3751 python/convertrules.py:3751: str = re.sub (r'\bwhiteout\b', 'whiteout-box', str) On 2015/06/02 01

Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)

I realize I am changing my opinion here on a few points: 1) leave the old whiteout as a simple box and give the new function a new name 2) enabling either type of whiteout through a single property looks better 3) the thickness should be in terms of line-thickness rather than staff-space I hope y

Re: Docs: clean up after \relative conversion (issue 239250043 by k-ohara5...@oco.net)

Reviewers: Trevor Daniels, dak, Message: On 2015/05/26 16:21:48, dak wrote: The script had about 90% coverage or so and working over Learning manually to make it consistent makes sense. I think we can live with incomplete coverage elsewhere. I completed (I'm pretty sure) the conversion i

Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)

The additional copies of the stencils do make the PDF files noticeably larger. 8% larger when I used 16-copy whiteout around all dynamics and text marks in a string quartet score. The compilation time increased by 2%, desipte the repeated computations of sines and cosines. With \pointAndClickOn

Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)

On 2015/05/25 20:43:30, Carl wrote: In my opinion, whiteout needs to create a layer of white paint. That's what whiteout means. Well, "whiteout" is the name chosen by a couple modern Dutch programmers, but we don't really want whiteout. We want that thing that happens when you fill the neigh

Re: Add French-specific note names (issue 239930043 by v.villen...@gmail.com)

There seems to be a way to do this as a simple expansion of capabilities, so you do need to change existing input files. Have \language"français" accept 'ré', and also 're' for backward compatibility with the former use of Italian names. https://codereview.appspot.com/239930043/diff/40001/Docum

Re: add stencil-whiteout-outline function (issue 236480043 by paulwmor...@gmail.com)

We certainly want this in LilyPond, and for it to be easy to use. I think whiteout-stencil should simply call this function, with the defaults suggested, so that the existing \set Xx.whiteout=##t and \markup\whiteout use this new method. We could provide access to the old whiteout method with \s

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

I'm proposing to push a \fixed c' {} that always takes a reference pitch, as in the current patch. It costs nothing to leave \absolute in place for those who have learned it, but it is simplest to document instead the equivalent \fixed c {...}. That gives us the benefit of less typing and keeps

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/17 22:06:10, Trevor Daniels wrote: > ... I'd prefer > the syntax and options [of \absolute] to parallel those of > \relative. That is, an optional prefix pitch to indicate > the starting octave, and taking the starting octave from > the first contained note if the prefix is omitted.

Re: Doc: avoid implicit \relative; issue 4371 (issue 237340043 by k-ohara5...@oco.net)

Reviewers: Trevor Daniels, Message: On 2015/05/17 21:54:07, Trevor Daniels wrote: But I think if are to make this change it would also be good to say what leaving out \relative means. It was difficult to do this before as all the examples left it out (apparently). Now we can do it. Placin

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/17 22:06:10, Trevor Daniels wrote: I strongly prefer just two input modes, \relative and \absolute, Okay. I won't split the job to complement \relative between two functions. That leaves the question of what to name the one function. The proper name for this would be \absoluteWi

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

https://codereview.appspot.com/235010043/diff/140001/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): https://codereview.appspot.com/235010043/diff/140001/Documentation/notation/pitches.itely#newcode112 Documentation/notation/pitches.itely:112: The referenc

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/15 06:12:38, lemzwerg wrote: Given that we are currently producing development releases, I suggest that this gets implemented, then we simply wait a few months so that people can test it in real life, and then we do a final decision. If we don't come back with another patch, the curr

Re: Issue 2787: Sanitize usage of -DDEBUG, -DNDEBUG and assert (issue 236190043 by d...@gnu.org)

Probably a good idea, if assert() is really used like this. assert is essentially stating that the program cannot usefully continue if the assertion is not met There are uses of assert() where I suspect the output on conditions that would violate the assertion, would be better than no output a

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/12 13:11:52, paul_paulwmorris.com wrote: > On May 7, 2015, at 9:16 PM, Dan Eble wrote: > > \octave sounds like it should be a function returning the octave of a note. It does not sound like an entry mode. Yes, after further thought it’s probably better to

Changes: long english notenames switched from aflat to a-flat (issue 232330043 by d...@gnu.org)

This would be a good commit in which to also put something like @code{s}/-@code{-sharp} at Documentation/notation/pitches.itely @item @code{english} @tab -s/--sharp @tab -f/--flat @tab -ss/-x/--sharpsharp @tab -ff/--flatflat The -- produces an em-dash, only slightly longer than a -, so in the ma

Re: Issue 4205: Improve part combiner's rest analysis (issue 174610043 by nine.fierce.ball...@gmail.com)

I noticed the same bug with rests and skips. I'm using the patch below with success on my own music, but I haven't tried it on the regtest suite yet. https://codereview.appspot.com/174610043/diff/20001/scm/part-combiner.scm File scm/part-combiner.scm (right): https://codereview.appspot.com/174

Issue 3229: Make \relative { ... } interpret the first pitch as an absolute one (issue 235000043 by d...@gnu.org)

https://codereview.appspot.com/23543/diff/1/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): https://codereview.appspot.com/23543/diff/1/Documentation/notation/pitches.itely#newcode169 Documentation/notation/pitches.itely:169: like @code{\relative @

Re: Part_combine_iterator: simplify context substitution (issue 238070043 by nine.fierce.ball...@gmail.com)

This and the MMrest patch look sensible, but it is a good idea to test them on some orchestral music from mutopiaproject. I tried them on a piece I have and saw no output differences. https://codereview.appspot.com/238070043/diff/1/lily/part-combine-iterator.cc File lily/part-combine-iterator.c

Re: Make \shiftOff an assertive \override, not a \revert (issue 190500043 by k-ohara5...@oco.net)

On 2015/05/10 22:50:36, dak wrote: The \revert version generated "too many clashing notecolumns" warnings, which is now "need a \shiftXxx", and I thought it a bit safer to keep a warning. What's safer here? For old code where people did not bother heeding the warning anyway (possibly bec

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

I tried out a some of these suggestions. On 2015/05/03 16:42:02, Trevor Daniels wrote: a continuous scale would be \relativeOctave { c, d e f g a b c' d e f ...}. I like that the octave marks indicate transitions to the next octave where octaves run CDEFGAB, because that is consistent with usu

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/06 09:58:25, pacovila wrote: I love \relative mode because it fits perfectly with a certain kind of content. But if relative mode didn't exist, people would be much more efficient using clever combinations of \transpose and sequential expressions. Even though \relative does exist, I

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/03 16:42:02, Trevor Daniels wrote: a continuous scale would be \relativeOctave { c, d e f g a b c' d e f ... }. The c' resets the octave. This doesn't work so well for a melody oscillating a tone or two above and below a c, of course, but it does avoid multiple ''' and ,,,. T

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

On 2015/05/03 20:25:22, dak wrote: On 2015/05/03 16:42:02, Trevor Daniels wrote: I find it awkward when \absolute c'' and \absolute g'' mean exactly the same thing. But it's not like I could not live with it. But I still would recommend just using c to keep one's options for possible lat

Re: absolute pitch entry: accept an offset octave (issue 235010043 by k-ohara5...@oco.net)

Reviewers: dak, lemzwerg, Trevor Daniels, Dan Eble, pwm, Message: On 2015/05/03 08:20:03, Trevor Daniels wrote: I'd prefer the syntax and options to parallel those of \relative. That is, an optional prefix pitch to indicate the starting octave, and taking the starting octave from the first cont

Re: Partcombiner documentation (Issue 4307) (issue 233110043 by philehol...@googlemail.com)

Looks fine. https://codereview.appspot.com/233110043/diff/1/Documentation/notation/simultaneous.itely File Documentation/notation/simultaneous.itely (right): https://codereview.appspot.com/233110043/diff/1/Documentation/notation/simultaneous.itely#newcode937 Documentation/notation/simultaneous.

Re: Updated patch for issue 155 (issue 230860044 by carl.d.soren...@gmail.com)

https://codereview.appspot.com/230860044/diff/20001/input/regression/parenthesize-notes-accidentals.ly File input/regression/parenthesize-notes-accidentals.ly (right): https://codereview.appspot.com/230860044/diff/20001/input/regression/parenthesize-notes-accidentals.ly#newcode10 input/regressio

Re: Updated patch for issue 155 (issue 230860044 by carl.d.soren...@gmail.com)

LGTM. The main difference is the change in handling pure functions. I can't see any difference here relative to Joe's patch, but it looks fine. https://codereview.appspot.com/230860044/diff/20001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.

Re: Part combiner: move direction handling out of iterator (issue 226430043 by nine.fierce.ball...@gmail.com)

Looks good to me. https://codereview.appspot.com/226430043/diff/20001/input/regression/display-lily-tests.ly File input/regression/display-lily-tests.ly (right): https://codereview.appspot.com/226430043/diff/20001/input/regression/display-lily-tests.ly#newcode224 input/regression/display-lily-t

Re: Avoid floating-point compares on Stem:head_positions; issue 4310 (issue 217720043 by k-ohara5...@oco.net)

On 2015/03/11 18:14:37, dak wrote: On 2015/03/11 16:36:44, Keith wrote: > I think a standard-conforming implementation should give bit-identical > floating-point results for these parallel computations, No, not at all. In C++, any computation may be carried out at any precision at least a

Re: Let \displayLilyMusic print even repeated durations (issue 206770043 by d...@gnu.org)

looks fine. https://codereview.appspot.com/206770043/diff/40001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/206770043/diff/40001/Documentation/changes.tely#newcode522 Documentation/changes.tely:522: longer omit redundant note durations. Thi

Let \displayLilyMusic print even repeated durations (issue 206770043 by d...@gnu.org)

That is unfortunate, but I can't think of anything better. I use \displayLilyMusic mostly to transpose a section of music as a starting point for music for another instrument, so the brief form was nice to have, but not essential. This needs an entry in changes.itely to explain "\displayLilyMusi

Re: Issue 4275: Allow user-defined rest styles. (issue 200860043 by nine.fierce.ball...@gmail.com)

Looks pretty good. The change to tabstaff looks suspicious the way you describe it. Keeping regtests unchanged is a bad reason to put invisible rests in tablature, but keeping users' existing scores unchanged might be a good reason. I suppose the former behavior of giving Rests an extent regard

Re: Issue 4274: Fix a cyclic dependency in Rest_collision (issue 193590043 by nine.fierce.ball...@gmail.com)

Are you sure there is a cycle? Rest_collision::calc_positioning_done now uses the pure height of rests because the height of certain rests depends on their position on the staff, which Rest_collision is responsible for adjusting. Rest_collision does use the height of the rests to determine the

Re: Reduce size of PDF files when inc. in *TeX docs (issue 194090043 by pkx1...@gmail.com)

I have not yet used LilyPond with TeX, so I have no opinion. I looked up the history: Use of PostScript glyphshow, rather than show, for all characters seems to have started with http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commitdiff;h=c3d6497157576dc0d4ae77c44978d54e7a212074 after some

Re: Remove Unfolded_repeat_iterator (issue 194100043 by nine.fierce.ball...@gmail.com)

On 2015/01/09 05:03:04, Dan Eble wrote: Does this make sense? TIA. It seems to work fine. I don't know your goal. Simply removing the strange duplication between the unfolded-repeat iterator and the function unfold-repeats-fully would be enough of a goal. Put the reason for the change in th

Re: property to set voiced-rest positions; issue 3902 (issue 188580043 by k-ohara5...@oco.net)

Reviewers: dak, Message: Shouldn't the fallback remain 4 rather than 0? Either one. I couldn't decide. If anyone says \override MultiMeasureRest #'voiced-position = #'() should we default to the historical value of 4, or the less-surprising 0 ? Description: property to set voiced-rest posi

Re: skyline.cc: merge algorithm terminates independent of floating-point (issue 186530043 by k-ohara5...@oco.net)

On 2014/12/22 06:10:48, lemzwerg wrote: LGTM. Maybe it makes sense to commit the removal of the `deholify' stuff separately. Yes. There were three commits with header lines as in the Description here. Now there are four. A single loop merging skylines one building at a time runs 1% faster wi

Re: skyline.cc: merge algorithm terminates independent of floating-point (issue 186530043 by k-ohara5...@oco.net)

Reviewers: lemzwerg, https://codereview.appspot.com/186530043/diff/1/lily/skyline.cc File lily/skyline.cc (right): https://codereview.appspot.com/186530043/diff/1/lily/skyline.cc#newcode249 lily/skyline.cc:249: printf ("We are here"); On 2014/12/22 04:25:43, lemzwerg wrote: Debugging remnants?

Re: Issue 4204: Convert ly::time-signature::print from C++ to Scheme. (issue 176180043 by nine.fierce.ball...@gmail.com)

Looks good. Having the built-in time signature markup in Scheme, available as building-blocks for custom time signatures, will probably be useful for several people. You might just leave out the regression test. https://codereview.appspot.com/176180043/diff/20001/input/regression/time-signature-

Re: Add chord range to make-part-combine-music (issue 144170043 by nine.fierce.ball...@gmail.com)

Still good, still useful. The hesitation on the user interface seems foolish. Without a stable user interface this becomes a function for just one user. Changing the current global variable 'partcombine-chord-range' will not become the user interface. There is no context in which to set that r

Re: Issue 1321: Enhancement: add partcombineUp and partcombineDown functions (issue 13242056)

No need to pass the context modifications through the lower-level functions, when they can be applied at the level of the \partcombineUp music function as Dan does now in his lilypond input. These modifications do not affect the operation of \partcombine, but are merely added to its output so the

Re: Add chord range to make-part-combine-music (issue 144170043 by nine.fierce.ball...@gmail.com)

This seems to allow the style of partcombine where unisons are double-stemmed which would be very nice. You can, if you like, put a small image (like a 2kB png) on the lilypond issue tracker showing the desired output; then people can understand the purpose of the patch. I see that recording-gro

Re: Reimplement forced partcombine decisions via context properties (issue 144600043 by d...@gnu.org)

Now I see the problem. The properties you are using are those created in a pre-engraving step, done separately on each of the inputs to \partcombine. Using properties of those separate contexts for the force-part-combine state is a bad idea. Right now, \partCombineApartOnce, etc., affects the s

Re: Issue 4169: Line and page breaking syntactic sugar (issue 156400043 by tdanielsmu...@googlemail.com)

On 2014/10/22 07:12:16, dak wrote: I never really got \overrideProperty. Would it cooperate with \once? Does it only take effect once? \overrideProperty uses an ApplyOutput event (as opposed to an OverrideProperty event) to change a property in grobs that *already* *exist* at the current t

Re: Issue 4169: Line and page breaking syntactic sugar (issue 156400043 by tdanielsmu...@googlemail.com)

On 2014/10/22 11:20:48, Trevor Daniels wrote: On 2014/10/22 10:33:30, Trevor Daniels wrote: ... but only for line breaking. It seems that page breaking needs a window of opportunity to break a page that is several bars long. The number of bars required varies with the length of the music

Doc: Issue 3324: Add explanation of clashing note columns warning (issue 154640043 by tdanielsmu...@googlemail.com)

LGTM Just for your interest, I have been thinking that when the user gives explicit \voiceOne or \shiftOn to the voices, Lily should set those voices at the shifts requested, even if two voices have the same shift Currently users are required to set 'ig

Re: Issue 4169: Line and page breaking syntactic sugar (issue 156400043 by tdanielsmu...@googlemail.com)

Looks good to me. I'm just suggesting more changes that you might want to do at the same time. https://codereview.appspot.com/156400043/diff/20001/Documentation/notation/spacing.itely File Documentation/notation/spacing.itely (right): https://codereview.appspot.com/156400043/diff/20001/Document

Re: Issue 4169: Line and page breaking syntactic sugar (issue 156400043 by tdanielsmu...@googlemail.com)

https://codereview.appspot.com/156400043/diff/1/ly/property-init.ly#newcode359 > > ly/property-init.ly:359: lineBreaksOn = using \override's for re-establishing the default settings, What do others think? Yes, \autoLineBreaksOn is an assertive statement, so I would expect it to set the stat

Re: Add original-breaks.ly commands (issue 150670043 by lilyli...@googlemail.com)

On 2014/10/09 08:11:29, uliska wrote: On 2014/10/08 05:01:40, Keith wrote: > Why not store the common music in a variable, and show > \score {\music} > \discaredOriginalBreaks > \score {\music} > ? Because it wouldn't work that way. \discardOriginalBreaks would be interpreted when the comma

Re: Add original-breaks.ly commands (issue 150670043 by lilyli...@googlemail.com)

For this particular task, you should also show the user that he can define at the top of the input, or in a header file if he uses one, originalBreak = \break and insert before music where we are done with the original breaks, originalBreak = {} %\break That is easy enough to understand, and

Re: Check for Note_column interface before using grob. (issue 141190043 by e...@ticalc.org)

LGTM https://codereview.appspot.com/141190043/diff/1/lily/rest-collision.cc File lily/rest-collision.cc (right): https://codereview.appspot.com/141190043/diff/1/lily/rest-collision.cc#newcode111 lily/rest-collision.cc:111: if (Note_column::has_interface(e)) We'll run this through the 'fixcc.py

Re: Limit looping in Grob::common_refpoint (issue 4079) (issue 134600043 by e...@ticalc.org)

I would prefer to know what pattern of data makes this loop endless, but the patch seems fine. https://codereview.appspot.com/134600043/diff/20001/flower/include/strict-counter.hh File flower/include/strict-counter.hh (right): https://codereview.appspot.com/134600043/diff/20001/flower/include/s

Re: Issue 346: monochord issues (issue 138150043 by d...@gnu.org)

https://codereview.appspot.com/138150043/diff/20001/lily/stem.cc File lily/stem.cc (right): https://codereview.appspot.com/138150043/diff/20001/lily/stem.cc#newcode597 lily/stem.cc:597: reverse_overlap = 0.1; This seems not enough overlap to make it look like a chord. In the rare cases where we

Re: Issue 4083: Implement \tagGroup command (issue 137920043 by d...@gnu.org)

The behavior of the old patch seemed better, in the case where someone does combine a \keepWithTags A.C with A and C from different groups. In that case, the user knows about both tag-groups and might be thinking of them as a unified group, and expect that command to keep music tagged \tag#'A \ta

Re: Issue 4083: Implement \tagGroup command (issue 137920043 by d...@gnu.org)

LGTM It worked fine on (adapted) examples from the divisi staff issue and emails. Looking at the code, I was confused how it worked without the \cleansed concept of the LSR entries, until I realized un-grouped tags effectively form their own group, so old uses of \keepWithTag ignore tags in the n

Re: Issue 4078: Doc: Use variables rather than instrument definitions (issue 138950043 by tdanielsmu...@googlemail.com)

still LGTM https://codereview.appspot.com/138950043/diff/1/Documentation/notation/vocal.itely File Documentation/notation/vocal.itely (right): https://codereview.appspot.com/138950043/diff/1/Documentation/notation/vocal.itely#newcode2580 Documentation/notation/vocal.itely:2580: kaspar = { On 20

Issue 4078: Doc: Use variables rather than instrument definitions (issue 138950043 by tdanielsmu...@googlemail.com)

LGTM I was thinking of completing the deprecation of instrumentSwitch, https://codereview.appspot.com/133390043/ unless you want to incorporate some or all of that here. https://codereview.appspot.com/138950043/diff/1/Documentation/notation/vocal.itely File Documentation/notation/vocal.itely (r

Re: Issue 3299: better alignment with PaperColumn parent (issue 132280043 by janek.lilyp...@gmail.com)

LGTM in principle, but expect to spend a lot of time figuring out how to deal with all the existing cases where we have tweaked alignment to the zero-width paper columns. The commit message makes sense as well, but talks mostly about the background, and only hints at the solution, asi if it were

Re: indclude notnames bn, etc., in English (issue 133840043 by k-ohara5...@oco.net)

On 2014/08/25 09:17:54, Jean-Charles wrote: Too bad, I prefer English to French because "g" is much less typing than "sol", and have always sung "mi" and "si" even when flattened or "fa" even when sharpened... So is there a similar need among French users to have a name for si-bécarre, so

Re: Fix clef transposition alignment (issue 3186) (issue 8363044)

https://codereview.appspot.com/8363044/diff/6001/lily/clef-modifier.cc File lily/clef-modifier.cc (right): https://codereview.appspot.com/8363044/diff/6001/lily/clef-modifier.cc#newcode42 lily/clef-modifier.cc:42: me->get_property ("clef-alignments")); On 2014/08/16 22:23:38, dak wrote: Up to t

Re: Issue 2507: Stop the entanglement of context properties and grob property internals (issue 126280043 by d...@gnu.org)

Big change, looks good, just one question. https://codereview.appspot.com/126280043/diff/40001/lily/context-property.cc File lily/context-property.cc (right): https://codereview.appspot.com/126280043/diff/40001/lily/context-property.cc#newcode152 lily/context-property.cc:152: Don't mess with MI

Re: Use aligned-on-x-parent instead of other callbacks for some grobs (issue 127860043 by janek.lilyp...@gmail.com)

Looks fine; just a minor unrelated change in code-organization looks unwise. https://codereview.appspot.com/127860043/diff/40001/lily/lily-guile.cc File lily/lily-guile.cc (right): https://codereview.appspot.com/127860043/diff/40001/lily/lily-guile.cc#newcode201 lily/lily-guile.cc:201: robust_s

Use aligned-on-x-parent instead of other callbacks for some grobs (issue 127860043 by janek.lilyp...@gmail.com)

https://codereview.appspot.com/127860043/diff/1/lily/flag.cc File lily/flag.cc (left): https://codereview.appspot.com/127860043/diff/1/lily/flag.cc#oldcode40 lily/flag.cc:40: DECLARE_SCHEME_CALLBACK (calc_x_offset, (SCM)); The docs use this, so we need either convert-ly, or a change by hand as a

Re: Remove tied accidentals after line-breaking (issue 46060045)

This clears up issues 3749 and 3646 but my motivation was to avoid the excessive font-lookups for accidentals. For example \transpose c cis { cisis } generates 5 error messages, one for each attempt to lookup a triple sharp. The Accidental.stencil callback had responsibility to delete the Accide

Re: Remove tied accidentals after line-breaking (issue 46060045)

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

Re: Issue 4024: Clarify break-align symbols and space-alist args in IR. (issue 114160044 by markpole...@gmail.com)

Looks good, except for details I couldn't confirm, noted below. If you confirmed them, looks good. https://codereview.appspot.com/114160044/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/114160044/diff/1/scm/define-grob-propert

Re: Simplify the NullVoice context (issue 117050043 by k-ohara5...@oco.net)

On 2014/07/27 11:50:17, dak wrote: https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newcode787 ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete 'rhythmic-head-interface Ugh. This looks incredibly ugly. I guess I lack a sense of aesthetics for Sc

Re: Simplify the NullVoice context (issue 117050043 by k-ohara5...@oco.net)

Reviewers: janek, dak, Message: On 2014/07/27 11:25:41, janek wrote: https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-combine-nullvoice.ly#newcode15 input/regression/lyric-combine-nullvoice.ly:15: >> } This regtest is nice, but as far as i can see it doesn't check whet

Re: Set X-parent of TextScript to NoteColumn instead of PaperColumn (issue 106640043 by janek.lilyp...@gmail.com)

On 2014/07/26 06:49:41, janek wrote: Setting TextScript.cross-staff property to #f is required to ensure that there are no collisions between TextScripts and cross-staff notes: (see also beam-cross-staff-auto-knee.ly) As far as I can see, we don't want TextScript.cros

Re: fix Issue 2462. (issue 108280044 by janek.lilyp...@gmail.com)

On 2014/07/07 10:21:42, janek wrote: ok - this looks like a good path to follow. Unfortunately, i don't know when i'll have enough time to chew on this stuff - it seems that i'll have a busy week (or two). Let's countdown and push this as-is, then. https://codereview.appspot.com/108280044/ _

Re: Issue 2245: always align dynamics and lyrics on "main" notehead (issue 108270044 by janek.lilyp...@gmail.com)

https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm#newcode87 scm/define-grobs.scm:87: (X-extent . ,ly:accidental-interface::width) I opened a tracker item http://code

Re: Changes.tely updated - 2.19.x up to June 2014 (issue 108130043 by pkx1...@gmail.com)

https://codereview.appspot.com/108130043/diff/90001/Documentation/changes.tely File Documentation/changes.tely (right): https://codereview.appspot.com/108130043/diff/90001/Documentation/changes.tely#newcode143 Documentation/changes.tely:143: spaced using only its @emph{left-edge} as reference po

fix Issue 2462. (issue 108280044 by janek.lilyp...@gmail.com)

This does make sense, but if you change the basic rules of a Spring (allowing the natural length to be less than the minimum-distance constraint, so it stays at the minimum length until the line is considerably stretched) you should try to make the change consistently everywhere ? https://codere

Re: Doc: NR Pitches.itely - added 2 new snippets (issue 110240044 by pkx1...@gmail.com)

Is it worth making the Notation Reference longer? https://codereview.appspot.com/110240044/diff/40001/Documentation/notation/pitches.itely File Documentation/notation/pitches.itely (right): https://codereview.appspot.com/110240044/diff/40001/Documentation/notation/pitches.itely#newcode1325 Docu

Re: Issue 3958: Give examples of \applyContext usage. (issue 110060045 by markpole...@gmail.com)

Looks fine as-is. You might look at it again, and see if you like the idea of shortening it so more people read it. https://codereview.appspot.com/110060045/diff/1/Documentation/extending/programming-interface.itely File Documentation/extending/programming-interface.itely (right): https://coder

use X-parents to align MMRNumbers, MMRTexts and PercentRepeatCounters (issue 108110045 by janek.lilyp...@gmail.com)

looks good, and logical. https://codereview.appspot.com/108110045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3254: align unassociated lyrics using NoteColumn extent. (issue 108110044 by janek.lilyp...@gmail.com)

Looks very good. I looked for unwanted side-effects, and found no problems. The old code did a very bad thing, ignoring the "self-alignment-X/Y" request if the parent was a paper-column. https://codereview.appspot.com/108110044/diff/80001/lily/paper-column.cc File lily/paper-column.cc (right):

Re: Reposition voiced rests (Issue 3902) (issue 101720045)

Looks like a new property is the only way to get an overall improvement. LilyPond has good infrastructure for using properties, so there is not much code to add. https://codereview.appspot.com/101720045/diff/2/lily/rest.cc File lily/rest.cc (right): https://codereview.appspot.com/101720045/dif

Re: Issue 3917: Add \alternatingTimeSignatures (issue 97110045)

On 2014/05/10 09:52:41, Trevor Daniels wrote: (We try to avoid referring to the reader with 'you', Instruction manuals, especially those for a machine, usually make clear what actions the user takes versus what the machine does. This manual doesn't even use third person (as in "The user indica

Re: Issue 3917: Add \alternatingTimeSignatures (issue 97110045)

The score is harder to understand with this function, than it was after I expanded the function's contents in-line. The boundary of the action of this function (part timing, mostly printed representation) is hard to understand and remember. This version of the function does partial changes to th

Re: note-spacing: pad accidentals from neighbor as far as from parent; issue 3869 (issue 65260044)

Reviewers: janek, Message: On 2014/02/20 22:21:37, janek wrote: I would add a comment "TODO: this hard-coded value should be removed when http://code.google.com/p/lilypond/issues/detail?id=2142 will be taken care of" What does 'hard-coded' mean here? If there is a setting of 'padding' padding

Re: note-spacing: stretch somewhat uniformly; issue 3304 (issue 36830045)

Forgot to move one magic number in the reorganization. The behavior of the line-breaker is strange in the presence of zero-stretchable lines; maybe the enforcement of minimum stretchability should go there instead. https://codereview.appspot.com/36830045/diff/120001/lily/note-spacing.cc File li

Re: NR: Removed example for alternate repeats (issue 61170044)

I suggest just removing the obsolete text, and skipping the additions (unless you are worried that the manual is getting too short). https://codereview.appspot.com/61170044/diff/20001/Documentation/notation/repeats.itely File Documentation/notation/repeats.itely (left): https://codereview.appsp

  1   2   3   4   5   6   7   >