Re: Fix #5964: MM Rests shouldn’t segfault when there’s no StaffSymbol. (issue 576090043 by v.villen...@gmail.com)
Here's a thought. https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc File lily/multi-measure-rest.cc (right): https://codereview.appspot.com/576090043/diff/557790043/lily/multi-measure-rest.cc#newcode268 lily/multi-measure-rest.cc:268: bool oneline = (!staff) || Staff_symbol::line_positions (staff).size() == 1; On 2020/05/07 15:12:03, Valentin Villenave wrote: > On 2020/05/07 12:18:48, Dan Eble wrote: > > If there is no staff symbol, then there isn't even one line. Are you sure you > > don't want this? > > bool oneline = staff ? /*etc.*/ : false; > > No, I actually want oneline to be true . I did consider changing the variable > name to no_multiple_lines, because that’s what it’s really about. I also > considered adding a comment such as: > // If there is no StaffSymbol, print MMrests on one (invisible) line. > > > Incidentally, I am surprised to see the amount of work involved in determining > > whether the staff has one line. It's not the kind of thing one expects to > > involve heap allocation. > > I suspect this sort-of mirrors the way you’d do it in Scheme: > (if (> (length > (ly:grob-property > (ly:grob-object grob 'staff-symbol) > 'line-positions)) > 0) > etc. > > If you have a simpler syntax in mind, please share :-) > > Cheers, > -- V. What about changing the name from oneline to specialcase? Or maybe default_position? Then it would naturally apply to both no staff symbol and one-line staff symbol. https://codereview.appspot.com/576090043/
Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)
On 2020/04/24 21:19:57, dak wrote: > On 2020/04/24 21:18:12, dak wrote: > > > https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc > > File lily/stencil-integral.cc (right): > > > > > https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465 > > lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot path. > > Sorry for not being clear: the question was not why this change was effective > in > > saving time, but why it was valid. When thickness is zero, you only update > the > > upper skyline. Why would the lower skyline no longer need updating? > > Well, other way round, but apart from that the question stands. When thickness is zero, the upper and lower curves are the same. Either one completes the skyline. https://codereview.appspot.com/579630043/
Re: midi: convert data to bigendian encoding directly (issue 565920043 by hanw...@gmail.com)
LGTM Makes the code much easier to read. Carl https://codereview.appspot.com/565920043/
Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)
LGTM Carl https://codereview.appspot.com/583750043/
Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)
I am in favor of this patch because of the following: 1) David K. has a long history of improving things through changes like this. 2) It does not change the user interface or any files that a user accesses 3) David has done the work of making all the code changes this syntax change requires Carl https://codereview.appspot.com/573670043/
Re: Drop support for multiple configurations (issue 581630043 by jonas.hahnf...@gmail.com)
I really like the work. I just wonder if we should eliminate the instructions on how to achieve the objective. https://codereview.appspot.com/581630043/diff/563520043/Documentation/included/compile.itexi File Documentation/included/compile.itexi (left): https://codereview.appspot.com/581630043/diff/563520043/Documentation/included/compile.itexi#oldcode895 Documentation/included/compile.itexi:895: @node Compiling for multiple platforms I wonder if we should keep the heading "Compiling for multiple platforms", and just have it say "Use a separate build directory for each different platform, and run configure in each build directory." Perhaps even use the same example, and show how to handle this situation. https://codereview.appspot.com/581630043/
Re: Special-case syntax error of duration before octave marks (issue 557410043 by d...@gnu.org)
On 2020/02/11 21:46:52, lemzwerg wrote: > Good idea! From inspection only, LGTM. Sounds like a great idea! Carl https://codereview.appspot.com/557410043/
Re: Doc: Some miscellaneous suggestions from Peter Toye (issue 579280043 by michael.kaepp...@googlemail.com)
On 2020/02/09 15:32:14, lilypond_ptoye.com wrote: > Sunday, February 9, 2020, 2:16:50 PM, you wrote: > > > I'm a native US speaker. The following is my opinion. > > > Alteration is a change in pitch from the base > > pitch. Base pitch is C, > > alteration is sharp, actual pitch is C#. > > > Accidental is a change in pitch from the > > standard scale pitch. As > > mentioned by Peter, C# in a D major scale is > > not an accidental, although > > it is an alteration. > > Surely "standard scale pitch or previously altered pitch". In D major: "cis c > cis" the first note is an alteration but not an accidental, the second is an > accidental but not an alteration, the third is both. Now I'm really splitting > hairs. > > I'm beginning to think that this is all getting too theologial. I'm a practising > musician, not a theorist, and I raised the point as I'd never heard of > 'alteration' used in this rather technical sense. If people are happy with the > distinction let's just keep it and I withdraw my suggestion. I guess, strictly speaking, in lilypond we input a pitch that consists of a base pitch, an optional alteration, and an octave (which in relative music is most often omitted to accept the default octave). Then the display (or lack thereof) of an accidental is governed by the accidental display rules and any overrides we add. Interestingly, as you correctly pointed out, a natural can be an accidental (overriding a sharp or flat in the key signature). And we don't add anything to the input stream to indicate a natural. I can see that this is all potentially a bit confusing, but I think it's captured pretty easily by example. So we should probably just make sure we don't say things that are blatantly false. Carl https://codereview.appspot.com/579280043/
Re: Doc: Some miscellaneous suggestions from Peter Toye (issue 579280043 by michael.kaepp...@googlemail.com)
I'm a native US speaker. The following is my opinion. Alteration is a change in pitch from the base pitch. Base pitch is C, alteration is sharp, actual pitch is C#. Accidental is a change in pitch from the standard scale pitch. As mentioned by Peter, C# in a D major scale is not an accidental, although it is an alteration. I would totally support cleaning up this in NR 1.1.1 Accidentals (Note -- we don't have 1.1.1.4 in the NR; the lowest level of headings is not numbered). Carl https://codereview.appspot.com/579280043/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/31 18:33:09, hanwenn wrote: > On 2020/01/31 18:22:47, Dan Eble wrote: > > On 2020/01/31 17:52:45, hanwenn wrote: > > > you can do a local alias > > > > > > vector<> = *vec; > > > > > > to aid readability. > > > > The more I think about banning non-const reference parameters, the more I'm > > against it. Google's coding standards may work for them, but their rationale* > > for this one is weak. How can we resolve this disagreement quickly? Do you > > simply have the final say as the project founder? > > Can we have this discussion on a thread separate from this code review? > I want this code to go in. This code is a definite improvement in my book. I like the names of the functions, and it seems to me that eliminating the Pars_start class is a good idea. Han-Wen has responded well to comments (even making changes that are not his preferred way of doing things). This patch LGTM. I would like to see some separate discussion about the status of Input and the use of non-constant reference pointers. But we shouldn't hold up this patch to have that discussion. Carl https://codereview.appspot.com/577410045/
Re: Add a tentative .clang-format for LilyPond. (issue 561340043 by hanw...@gmail.com)
On 2020/01/31 18:06:00, hanwenn wrote: > Will james pick this up automatically now, or does it need > an LGTM? James should pick it up automatically now. https://codereview.appspot.com/561340043/
Re: Add a tentative .clang-format for LilyPond. (issue 561340043 by hanw...@gmail.com)
IIUC, this is a .clang-format that can be (but is not required to be) used to format source code and prevent comments about formatting. At this point, we are not enforcing a shift to clang-format as the code standard for LilyPond. If this is true, LGTM. If we are enforcing a shift to clang-format, then I think we need an LGTM from David K. https://codereview.appspot.com/561340043/
Re: Inline assembler fallback for _FPU_SETCW() missing in MINGW libraries (issue 577450043 by thomasmorle...@gmail.com)
While this answer is specific, it could be clearer, IMO: Reviewing the Intel Manuals at https://software.intel.com/sites/default/files/managed/a4/60/253665-sdm-vol-1.pdf Section 4.2.2. We can see that the 64 bit significand of Double Extended Precision refers to an 80-bit floating point representation, and the 53-bit significand of Double Precision refers to a 64-bit floating point representation. I think if we just leave the 64/53 bit numbers in the comment, the reader might think we are not using 64-bit floating-point representations. https://codereview.appspot.com/577450043/
Re: Only print out open type font substitution if there was a change (issue 577390043 by hanw...@gmail.com)
On 2020/01/24 17:46:33, Dan Eble wrote: > On 2020/01/24 17:37:42, lemzwerg wrote: > > Replace font name '%s' with '%s'. > > Yes, or, > > Change font name from `%s' to `%s' Or Use font name `%s' instead of `%s' Since the name of the font is not changed on the disk, but only in the currently running program. Carl https://codereview.appspot.com/577390043/
Re: Issue 5650: Use C++11 "override" keyword (issue 551320043 by nine.fierce.ball...@gmail.com)
LGTM. Thanks for using the C++11 standard stuff to go to C++ usage instead of custom LilyPond code. Carl https://codereview.appspot.com/551320043/
Re: Issue 5620: Change vector to vector (issue 545280043 by nine.fierce.ball...@gmail.com)
I'm glad to see this. It clears up some of the confusion I've had in digging into the code relative to the distinctions between Item, Grob, and Paper_column. Thanks, Carl https://codereview.appspot.com/545280043/
Re: add suggestAccidentals = #'cautionary option (issue 577130043 by lilyp...@maltemeyn.de)
Very nice. I think there should be a changes.tely entry. Carl https://codereview.appspot.com/577130043/
Re: add suggestAccidentals = #'cautionary option (issue 577130043 by lilyp...@maltemeyn.de)
Very nice. I think there should be a changes.tely entry. Carl https://codereview.appspot.com/577130043/
Re: add property label-alignments to OttavaBracket (issue 575330043 by lilyp...@maltemeyn.de)
I've added a comment about the docstring -- it's a request for consideration, not a demand for change. Also, I think a changes.tely entry should be included. https://codereview.appspot.com/575330043/diff/551180043/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/575330043/diff/551180043/scm/define-grob-properties.scm#newcode573 scm/define-grob-properties.scm:573: (label-alignments ,number-pair? "The vertical alignments of OttavaBracket In general, we like to have properties that are not defined for specific grobs so that they can be reused. This helps keep the namespace manageable. Are there other grobs that have labels? For example, Volta brackets? Nobody I know would want to put a Volta bracket below the staff, but with people regularly inventing new notation I can never say never ... Or maybe line spanners? I'm raising this concern not because I want to have this property applied to any other interface as part of this patch, but because I wonder if the property description should say "The vertical alignment of a grob label when the grob is placed below and above the staff", thus removing the OttaveBracket part from the definition. In our code, we know it applies to an OttavaBracket because we override OttavaBracket.label-alignments. This is not a requirement for change, just a request for consideration. https://codereview.appspot.com/575330043/
Re: Issue 5603: create just one tree.gittxt file (issue 559260043 by nine.fierce.ball...@gmail.com)
LGTM. Thanks for working on these annoying details! Carl https://codereview.appspot.com/559260043/
Re: Implement MeasureAttachedSpanner (issue 571180043 by david.nales...@gmail.com)
I have not reviewed the code, but this looks like a worthwhile addition. Thanks for doing it! I think the name should be changed from MeasureAttachedSpanner to BarAttachedSpanner. A measure is the interval between bar lines. The spanner is attached to the bar line. While it requires some work, I think it's worth making the change to be more clear in our terminology. Thanks, Carl https://codereview.appspot.com/571180043/
Re: Issue 5217: Fix sorting order without outside-staff-priority (issue 554960043 by jonas.hahnf...@gmail.com)
LGTM. Nice work! Carl https://codereview.appspot.com/554960043/
Re: Issue 5590: Remove dead code from Dot_formatting_problem (issue 583100043 by nine.fierce.ball...@gmail.com)
On 2019/11/01 17:18:34, Dan Eble wrote: On 2019/10/29 19:38:23, Carl wrote: > My bottom line is that the current system is definitely broken. Yes, and the thing that bugs me most is that it reallocates memory per candidate, for no benefit. > I will look into my code tonight when I get home and see how I > handled the "best" issue. If you need more time, I won't be offended if we keep this patch in "countdown" a bit longer; however, I don't want the best to become the enemy of the good. If someone later wants to return to this, it's not hard to revert it in git, nor is it hard to reimplement a loop to find the best of a series of things properly. OK. let's move ahead with the patch as proposed. If I have stuff that uses "best", it's already not in master, and my patch can add it. If not, it's really not doing much (if any) good to have NOP code hanging around. https://codereview.appspot.com/583100043/
Re: Issue 5590: Remove dead code from Dot_formatting_problem (issue 583100043 by nine.fierce.ball...@gmail.com)
I'm torn on this patch. Obviously, the current case is not good. We shouldn't have dead code around. But it is possible that we should in fact use a "best" for the dot formatting problem, and the failure to do so may lead to some of the random formatting issues we see in our regtests. It's not clear to me that the optimal solution to this is to remove the best. It might be better to keep the best and put in a FIXME that points out it is unused. I did some work a few years a go on the dot_column code, but never got it ready for submission. I will look into my code tonight when I get home and see how I handled the "best" issue. My bottom line is that the current system is definitely broken. I'm not sure if the best fix is to remove the unused code (which will hide the potential brokenness of the dot_column creation) or to highlight the unused code as being broken. https://codereview.appspot.com/583100043/
Replace file() by open() (issue 554930043 by jonas.hahnf...@gmail.com)
LGTM, although I haven't tested it. Thanks, for working on this! Carl https://codereview.appspot.com/554930043/
Re: doc: Update section on "commit access" (issue 566930043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/566930043/
Re: Improve content of tree.gittxt (issue 564990043 by knup...@gmail.com)
LGTM. THanks for doing this! Carl https://codereview.appspot.com/564990043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
build logging: Improve METAFONT tracing. (issue 554810043 by lemzw...@googlemail.com)
LGTM. I love the comments helping to understand .metafont parameters. https://codereview.appspot.com/554810043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improve content of tree.gittxt (issue 564990043 by knup...@gmail.com)
Why do we want 10 commits instead of just 1? I don't see the rationale for this patch. https://codereview.appspot.com/564990043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: stem.cc - issue 5303 - misplaced notehead (issue 570830043 by pkxgnugi...@runbox.com)
Perhaps this patch should also revert 87eb2f9fe1be3a532675fe4b7322bbba5a60ba5c since that patch was a workaround, rather than a real fix, as demonstrated during this troubleshooting thread. Carl https://codereview.appspot.com/570830043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Charles Winston's chord-semantics GSOC work (issue 568650043 by carl.d.soren...@gmail.com)
Thanks for the feedback! New patch set uploaded.
https://codereview.appspot.com/568650043/diff/572560043/Documentation/notation/chords.itely
File Documentation/notation/chords.itely (right):
https://codereview.appspot.com/568650043/diff/572560043/Documentation/notation/chords.itely#newcode472
Documentation/notation/chords.itely:472: c:m7^1 ees
On 2019/04/02 21:20:49, Valentin Villenave wrote:
I’d put the simple chord in front of the more unexpected use case.
Also, why not use c:maj7^1 and e:m chords?
Done.
https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-name-exceptions.ly
File input/regression/chord-name-exceptions.ly (right):
https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-name-exceptions.ly#newcode1
input/regression/chord-name-exceptions.ly:1: \version "2.16.0"
On 2019/04/02 21:20:49, Valentin Villenave wrote:
Why not update the regtest version number? OTOH, it doesn’t actually
introduces
a new syntax.
Done.
https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-sus.ly
File input/regression/chord-semantics-sus.ly (right):
https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-sus.ly#newcode11
input/regression/chord-semantics-sus.ly:11: }
On 2019/04/02 21:20:49, Valentin Villenave wrote:
I feel like these are way too many regtests. Sure, having a nicely
ordered list
of all features looks nice, but 1/ we might be adding quite a few
extra seconds
to `make check’ here 2/ regtests are not the place where to be listing
or
documenting features and 3/ all of these could as well be included in
a single
regtest, duly commented and explained: if anything ever breaks in the
future,
we’ll catch it just as well.
Upon review, I agree with you. This is really a set of regtests for the
chord-semantics chord namer. I've combined them into a single regtest.
https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):
https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engraver.cc#newcode99
lily/chord-name-engraver.cc:99: SCM name_proc = get_property
("chordSemanticsNameFunction");
On 2019/04/02 21:20:49, Valentin Villenave wrote:
Ugh. Is there really no way of merging this with chordNameFunction
(possibly
with additionaloptargs)? I understand this adds many additional (and
certainly
useful) features, but this looks like potential for
duplicate/semi-incompatible
subroutines down the line.
Fortunately, both approaches use chordRootNamer and therefore will be
able to
take advantage from the localized note names that are now available.
But still,
I wish we could factorize things even further.
With this patch, there are two fundamental modes of getting the
semantics necessary to define a chord name. The newly-added one is to
use the semantics entered by the user (chordSemanticsNameFunction); the
previous one was to parse the pitches in the chord and try to infer the
semantics. (There is also the pattern-matching provided by the
exceptions that allows overriding the automatic calculation of chord
names, replacing it with a lookup function.)
Since the difference between the two is how we get the semantics, I
could see that we replace the two fundamental chord name functions with
a single function having an optional argument which is a semantics
entry. If we want to use the semantics entry present in the chord, we
don't pass the optional argument. If we want to use the pitch parser
chord namer, we could create a new function whose job is to parse a set
of pitches into a semantics entry. The resulting semantics entry could
then be passed to the semantics-based namer.
Or alternatively, we could always call the semantic namer, and have the
semantic namer call the parse-semantics function if there is no
semantics entry in the chord. I believe this could be a way to achieve
your goal.
However, this will require more refactoring. I don't believe we should
hold off on this patch until we can get that part of it done better.
This patch has lanquished long enough. IMO we should just push it as-is
and get it in the code base.
https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-names.scm
File scm/chord-ignatzek-names.scm (right):
https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-names.scm#newcode342
scm/chord-ignatzek-names.scm:342: (define (make-root-markup root
lowercase-root?)
On 2019/04/02 21:20:49, Valentin Villenave wrote:
I’m not sure "make-*-markup is the most unambiguous name for that
subfunction.
Good catch.
I've changed make-*-markup to make-*-display for all of the functions
that are local to this file. Of course, the lillypond-defined
make-*-markup functions are not changed.
https://codereview.appspot.com/568650043/
___
lilypond-devel mailing list
Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwi...@gmail.com)
Thanks for figuring this out. I'm now working on make check, and will post a new patch shortly (I hope). The new patch is up at https://codereview.appspot.com/568650043 https://codereview.appspot.com/337870043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwi...@gmail.com)
On 2018/11/10 05:44:23, pwm wrote:
https://codereview.appspot.com/337870043/diff/40001/Documentation/notation/chords.itely#newcode679
Documentation/notation/chords.itely:679: represent the structure of
the chord.
When entered in node mode,
typo: "note mode"
Done.
https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode4
input/regression/chord-name-exceptions.ly:4: texidoc = "The property
@code{chordNameExceptions} can used
'can be used' (This was carried over, but might as well fix while
we're here.)
Done
https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode29
input/regression/chord-name-exceptions.ly:29: chExceptions = #(append
(chordmode->exception-entry chordVar markupVar) chExceptions)
Hmm, chExceptions is used in its own definition here? Does that work?
This is
not making sense to me.
chExceptions is previously defined to be a couple of new chords
prepended to ignatzekExceptions, so it can be used this way.
https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-lowercase-root.ly#newcode14
input/regression/chord-semantics-lowercase-root.ly:14:
Whitespace on unnecessary blank line.
Done
https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-name-exceptions.ly#newcode5
input/regression/chord-semantics-name-exceptions.ly:5: texidoc = "The
property
@code{chordSemanticsNameExceptions} can used
can be used
Done
https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-power-chord.ly#newcode12
input/regression/chord-semantics-power-chord.ly:12:
Whitespace on unneeded blank line.
Done
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode23
scm/chord-entry.scm:23:
Unnecessary new line.
Done
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode75
scm/chord-entry.scm:75: chord-semantics))
It's hard to read this code because of the way it's formatted. Would
be better
with more line-breaks to keep the lines from being too long and the
indentation
from going so far to the right and wrapping around to the next line
(in narrow
views like this code review one). Similar comment for other places in
this file
like this.
Done
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode238
scm/chord-entry.scm:238: 'chord-semantics chord-semantics))
Hmm, so there's a single 'chord-semantics property under a
'ChordSemanticsEvent
? Seems like we could avoid that extra step of nesting and just have
the
subproperties of 'chord-semantics under 'ChordSemanticsEvent ? And
that would
be more like NoteEvent and other events. Or maybe I'm missing
something?
We could do that, but it would pollute the global namespace with all the
properties
that are part of 'chord-semantics and are only used for chord semantics.
By putting them
in a single property, we avoid polluting the namespace. Similar to the
way we do fret-diagram-details.
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode243
scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
original-inv-pitch
Would be more clear and consistent if original-inv-pitch were renamed
to
original-inv-entry or similar.
Why? Not to be argumentative, but I wonder what about this name is more
clear and consistent, in your opinion?
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode267
scm/chord-entry.scm:267: (append (if bass (write-me "base3: " bass)
(list
(make-note-ev bass 'bass #t)) '())
This write-me looks like a debugging statement that was left in?
Done
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode272
scm/chord-entry.scm:272: (bass (make-elements (cons (make-note-ev bass
'bass
(cons #t #t))
Hmm, this (cons #t #t) looks like it could be related to one of the
regression
test failures that James posted about?
Will look more later.
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode306
scm/chord-entry.scm:306: (assoc-ref semantics-list key))
This is defined twice, see line 292 above.
Done
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode399
scm/chord-entry.scm:399: ((= alteration FLAT) (set! quality 'dim)
Instead of all of these (set! quality ...) why not just let the result
of this
cond be assigned to 'quality' and add an 'else' 'maj at the end?
Basically:
(quality (cond ((= alteration SHARP) 'aug) ...))
I think it's because for some intervals, you set a zero alteration as
perfect, while
for others, the zero alteration is 'maj. So it doesn't drop easily into
an else, I think.
https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode411
scm/chord-entry.scm:411: (if (= step-number 7) (set! quality 'min))
Defining quality using cond or case would be a
Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by chazwi...@gmail.com)
On 2018/11/10 19:44:47, pwm wrote: Hi Charles, Today I built and ran 'make check' with your patch applied to current master. I was able to get it to pass 'make check' by making the following two changes. 1. In `chord-entry.scm` line 267, remove `(write-me "base3: " bass)`. 2. In that same file, line 100, remove the parens to change `(chord-semantics)` to just `chord-semantics`. The first change fixed this error (but note the type check warning): input/regression/chord-names-languages.ly' ~~~ Parsing... Renaming input to: `/home/james/lilypond-git/input/regression/chord-names-languages.ly' warning: type check for `bass' failed; value `(#t . #t)' must be of type `boolean' /home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59: In procedure memoization in expression (if ba ss (write-me "base3: " bass) ...): /home/james/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:267:59: In file "/home/james/lilypond-git/build/out/s hare/lilypond/current/scm/chord-entry.scm", line 266: Missing or extra expression in (if bass (write-me "base3: " bass) (list (make -note-ev bass (quote bass) #t)) (quote ())). ~~~ And the second change fixed this error: input/regression/chord-name-entry.ly ~~~ $ /home/dev/lilypond-git/build/out/bin/lilypond input/regression/chord-name-entry.ly GNU LilyPond 2.21.0 Processing `input/regression/chord-name-entry.ly' Parsing.../home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35: In expression (chord-semantics): /home/dev/lilypond-git/build/out/share/lilypond/current/scm/chord-entry.scm:100:35: Wrong type to apply: ((modifier . #f) (root . #) (extension . 7) (additions) (removals) (bass . #f)) ~~~ So if you have a chance to upload a new patch set with those two changes, that should get things moving forward with the code review process. Cheers, -Paul Thanks for figuring this out. I'm now working on make check, and will post a new patch shortly (I hope). https://codereview.appspot.com/337870043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add a procedure to add slashes to Beams and straight Flags (issue 562550043 by thomasmorle...@gmail.com)
LGTM https://codereview.appspot.com/562550043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: add very short and Henze fermatas (issue 347080043 by lilyp...@maltemeyn.de)
Looks excellent to me. THanks for taking care of this. Carl https://codereview.appspot.com/347080043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Use "simple strings" rather than #"hash-prefixed Scheme strings" (issue 363910043 by v.villen...@gmail.com)
Because this represents a change in the documentation policy as listed in the CG, I think it should be discussed. While it is easier to not put the hash before the simple string, it is harder to have to understand which elements need the hash and which do not. I think that having the hash is actually easier for a beginner (although there may be some benefits to having the understanding of when a hash is necessary and when it is not). https://codereview.appspot.com/363910043/diff/1/Documentation/contributor/doc-work.itexi File Documentation/contributor/doc-work.itexi (right): https://codereview.appspot.com/363910043/diff/1/Documentation/contributor/doc-work.itexi#newcode487 Documentation/contributor/doc-work.itexi:487: Staff.instrumentName = "cello"} While the automatic conversion did this right, the paragraph now makes no sense. In fact, the proposed patch makes this paragraph wrong. This policy should be discussed, IMO, before we accept this patch. https://codereview.appspot.com/363910043/diff/1/Documentation/cs/learning/tweaks.itely File Documentation/cs/learning/tweaks.itely (right): https://codereview.appspot.com/363910043/diff/1/Documentation/cs/learning/tweaks.itely#newcode1587 Documentation/cs/learning/tweaks.itely:1587: alignAboveContext = "main" This piece of code shows what, IMO, is the biggest issue we need to consider with this change. In the past, one could get along by saying "All properties start with #", even though it wasn't strictly necessary. With this new change, once must understand which properties need # and which don't. I think the following is correct, although I haven't tested it: Numbers -- don't need # Simple strings -- don't need # Scm values do need # Boolean #f, #t Symbols Scheme expressions Where will this be documented? https://codereview.appspot.com/363910043/diff/1/Documentation/de/learning/fundamental.itely File Documentation/de/learning/fundamental.itely (right): https://codereview.appspot.com/363910043/diff/1/Documentation/de/learning/fundamental.itely#newcode487 Documentation/de/learning/fundamental.itely:487: alignAboveContext = "Hauptzeile" IIUC, we should not make the changes in the foreigh language versions of the documentation -- the translation process does it automatically through the use of .po file. Ami I wrong in my understanding? https://codereview.appspot.com/363910043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Use a stable sort when ordering MIDI items (issue 353790043 by mrbobo1...@gmail.com)
On 2018/11/03 12:39:41, Dan Eble wrote: The ticket for this review is https://sourceforge.net/p/testlilyissues/issues/5434/ . Carl, it sounds like James needs clarification as to whether you are still pressing for changing more sort calls to stable_sort calls. MHO is that this change stands fine on its own. I'm fine to have it submitted as-is. Carl https://codereview.appspot.com/353790043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Use a stable sort when ordering MIDI items (issue 353790043 by mrbobo1...@gmail.com)
Why not always have our sort use stable_sort? Have you tried with a large score (e.g. one from mutopia) to see what the resource implications are? https://codereview.appspot.com/353790043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue 5413: X-aligning problem with chords containing unisons (issue 369840043 by torsten.haemme...@web.de)
LGTM. Carl https://codereview.appspot.com/369840043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5381: Change intermediate PDF filename (issue 357760043 by truer...@gmail.com)
On 2018/07/15 07:44:32, trueroad wrote: Thank you for your reviewing. https://codereview.appspot.com/357760043/diff/20001/make/lilypond-book-rules.make File make/lilypond-book-rules.make (right): https://codereview.appspot.com/357760043/diff/20001/make/lilypond-book-rules.make#newcode33 make/lilypond-book-rules.make:33: mv $@ $(outdir)/$*.tmp.pdf On 2018/07/14 10:21:51, Dan Eble wrote: > On 2018/07/14 08:22:49, trueroad wrote: > > Unfortunately, we cannnot specify an output filename for LaTeX. > I'm fine with the current solution of a different directory, but I think one can specify an output filename for LaTeX. At least in my makefiles I do with the --jobname option Thanks, Carl https://codereview.appspot.com/357760043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5366: Move warnings out of find/create context functions (issue 349740043 by nine.fierce.ball...@gmail.com)
LGTM https://codereview.appspot.com/349740043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Avoid multiple evaluation of markups \pattern \fill-with-pattern (issue 357740043 by d...@gnu.org)
LGTM https://codereview.appspot.com/357740043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5345: Stop crash while merging ledger lines (issue 343270043 by d...@gnu.org)
LGTM. https://codereview.appspot.com/343270043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
On 2018/06/13 03:24:02, dan_faithful.be wrote: I perceive that we understand each other’s points and simply disagree. There is nothing new I want to counter with. I will just state that if a contributor were made uncomfortable by dynamic_cast, my two-pronged solution would be (1) gently encourage him to educate himself on this fundamental feature of C++, and (2) over time, rework the software to require fewer casts by preserving more type information in the internal interfaces and pushing the casts outward toward the interface with Scheme. I now understand more about the overhead that is involved in the encapsulation that I thought was desirable. Rather than an execution overhead, there is a coding overhead. For every type of dynamic cast I may want to use, I need to provide a getter method. And this just covers up a dynamic cast; there's not any reasonable error handling involved in the getter method. That's not very smart, I see now. I wholeheartedly agree with your changes. Thanks for running with this issue. Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I am convinced by these arguments. Thank you for your patience with me. Hopefully we can get some rats taken care of. Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5337: Create Bottom contexts in a more general way (issue 339710043 by nine.fierce.ball...@gmail.com)
This looks to me like a nice job of making the code more understandable and predictable. LGTM Carl https://codereview.appspot.com/339710043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
Dan, Thanks for the feedback. I appreciate it. I'm still not convinced that pulling the dynamic casts out of the getter/setter pair is better. You talk about performance penalties of dynamic casting. But your profiling shows that dynamic casting took about 1% of the processing time. Even if you could reduce dynamic casting by 90% (which seems unlikely), you'd only reduce execution time by 0.9%, which seems pretty insignificant. The tradeoff of having people know about dynamic casting and using it properly needs to be matched with people not needing to know about dynamic casting and being able to ignore it. It seems to me that unless there is a significant mistake that is made by code that doesn't know about the dynamic cast, it's better off to hide it. In fact, it seems to me that it would be possible (and maybe preferable) to put necessary error checking once in the setter/getter, rather than having to recreate it multiple times throughout the code base. If dynamic casting were taking up 50% of the process time, or errors in using dynamic casting were responsible for large numbers of bugs, I might feel different. But to me, the benefits I understand from your explanation seem to be not worth the cost of having dynamic casts show up in 21 different files. As I said before, I'm not asking for a reversion. I think I just have a different tradeoff value model than you have. Thanks for your explanation, Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5336: Remove downcasting methods from Grob_array and Grob_info (issue 344010043 by nine.fierce.ball...@gmail.com)
I realize this issue has already been pushed and closed. But I have a question. What is the harm of having the downcasting in the getter function? An advantage is that we can change the downcasting in one place if it is part of the member functions for the class. If not, we have to change it throughout the code base. Maintainability would seem to be a reason to keep it as it was. I'm not asking for a revert. But I would like to understand the reason it was worth it for Dan to make this change. Mostly for my own understanding of good programming practices. Thanks, Carl https://codereview.appspot.com/344010043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow use of Identity-H CMap and CID versions of Emmentaler (issue 343970043 by knup...@gmail.com)
The patch looks well put together.
However, I believe that the case problem with MacOS needs to be fixed.
I believe this patch won't work on MacOS -- only one version of the font
will be installed (the last one to be created).
https://codereview.appspot.com/343970043/diff/1/mf/GNUmakefile
File mf/GNUmakefile (right):
https://codereview.appspot.com/343970043/diff/1/mf/GNUmakefile#newcode145
mf/GNUmakefile:145: cd $(outdir) && mv $(notdir $@).otf $(notdir $@)
I think there is potential confusion where the CID file has no
extension, and just has capitalization to indicate it. Is this a
standard for CID-keyed fonts?
https://codereview.appspot.com/343970043/diff/1/mf/emmentaler-brace.pe.in
File mf/emmentaler-brace.pe.in (right):
https://codereview.appspot.com/343970043/diff/1/mf/emmentaler-brace.pe.in#newcode89
mf/emmentaler-brace.pe.in:89:
ConvertByCMap("/usr/share/ghostscript/9.23/Resource/CMap/Identity-H")
This hard-coded 9.23 seems to me to be broken in advance.
https://codereview.appspot.com/343970043/diff/1/mf/emmentaler-brace.pe.in#newcode90
mf/emmentaler-brace.pe.in:90: Generate("Emmentaler-Brace.otf")
Will this work on MacOS? IIRC, the default file system on MacOS is
case-insensitive, although it allows the used of mixed case. THis means
you can have emmentaler-brace,otf or Emmentaler-Brace.otf, but I don't
think you can have both by default on MacOS.
For compatibility, I think there needs to be a different name.
Here's an example that shows the problem:
Carls-MBP:junk carlsorensen$ echo "This is a test" > test
Carls-MBP:junk carlsorensen$ cat test
This is a test
Carls-MBP:junk carlsorensen$ echo "This is an UpperCase test " > Test
Carls-MBP:junk carlsorensen$ cat Test
This is an UpperCase test
Carls-MBP:junk carlsorensen$ cat test
This is an UpperCase test
https://codereview.appspot.com/343970043/diff/1/ps/cidres.in
File ps/cidres.in (right):
https://codereview.appspot.com/343970043/diff/1/ps/cidres.in#newcode1
ps/cidres.in:1: lilypond-datadir (/fonts/otf/Emmentaler-11)
concatstrings (r) file .loadfont
The hardcoding of all the font sizes here seems broken. If we change
the sizes to be generated from the .mf files; then this won't work any
more. It seems there should be one variable that lists all of the sizes
to be generated, and that variable should be used throughout the build.
I guess that would meant that rather than having cidres.ps be a
developer-created file, there should be some kind of a template file and
a make command that would cat together cidres.ps using the template file
and the font size variable.
But recognizing that encodingdefs.ps doesn't follow this practice in the
current distribution, this comment may be without merit (or else it
would apply equally to encodingdefs.ps as well.
https://codereview.appspot.com/343970043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow Scheme/identifiers for duration multipliers (issue 346810043 by d...@gnu.org)
LGreatTM! You have a way af making changes to the parser seem obvious, when they wouldn't have even crossed my mind. Thank you for all your hard work to rationalize the parser so that things like this can be done. https://codereview.appspot.com/346810043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5326: Generalize the special case of creating a Timing context (issue 344910043 by nine.fierce.ball...@gmail.com)
LGTM https://codereview.appspot.com/344910043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Context regression tests (issue 348760043 by nine.fierce.ball...@gmail.com)
On 2018/05/07 23:07:36, Dan Eble wrote: On 2018/05/07 22:53:29, Dan Eble wrote: > stop relying on duplicating type+ID Carl, I hope that these revisions address your concerns about the tests per se. After reviewing the revised tests, I am in favor of moving back to your original patch, with the exception of keeping the added recommendation about issuing a warning if there are two potential matches for descendants of a particular context. I had not realized that the search for the known ID contexts follows exactly the same behavior as the search for contexts without given IDs. I think that having the context ID's helps to make the behavior clearer. Maybe instead of using "FAIL" and "PASS" for the instrument names, it would be better to use "ORIGINAL" when the name is first given, and then "PARENT", "CHILD", "GRANDCHILD", "SIBLING" etc. to identify the context that is actually found. Just a thought that might help make the whole suite of tests easier to understand. Thanks for your work on this. Carl https://codereview.appspot.com/348760043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Place barres on fret diagrams if they can be inferred (issue 294570043 by carl.d.soren...@gmail.com)
Reviewers: thomasmorley651,
Message:
Harm,
Thanks for the great comments.
If the user doesn't want the barre to be displayed, they can avoid it by
setting fret-diagram-details.barre-type = #'none
Thanks,
Carl
https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm
File scm/translation-functions.scm (right):
https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm#newcode258
scm/translation-functions.scm:258: (let* ((barres (make-array 0 5 4)) ;
5 fingers, 4 elements/barre
On 2018/05/03 22:28:25, thomasmorley651 wrote:
Afaik, this will be the first use of make-array in our source,
so I'd love some more explanations in the comments, especially about 5
and 4
bounds.
5 is obviously the amount of possible fingers, as you stated already.
About 4: this will be filled lateron with
(barre-symbol
from-higher-string, p.e. 1
to-lower-string, p.e. 6
fret-number)
Why this order for second/third element?
I'm aware it makes no programmatical difference, but all NR examples
do it the
other way round, something like:
(barre 5 1 3) and not (barre 1 5 3)
Probably exchange those to be consistent with the NR.
Here, while doing array-set! and in the filtering-condition later.
Hmm. I just wanted to make a range from lower string number to higher
string number, because that's the way I think about intervals. I can't
see any problem with making it go from higher string number to lower
string number. And I guess that consistency with the NR is not a bad
idea, even though I don't think it's essential.
https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm#newcode278
scm/translation-functions.scm:278: (not (= (list-ref l 3 ) 0))
On 2018/05/03 22:28:25, thomasmorley651 wrote:
Why not use (second/third/fourth l) instead of (list-ref l 1/2/3)?
Because when I learned Scheme more than 30 years ago, first/second were
"syntactic sugar" and were frowned upon.
But I can see that for this case, first and second would be easier for
the casual reader to understand. Good suggestion.
https://codereview.appspot.com/294570043/diff/1/scm/translation-functions.scm#newcode289
scm/translation-functions.scm:289: (have-fingers (not (null? (filter
(lambda (sf)
On 2018/05/03 22:28:26, thomasmorley651 wrote:
Why (have-fingers (not ...))
and as predicate in line 306 as
(not have-fingers)
Couldn't both (not ...) be deleted?
Certainly, if we changed the name from have-fingers to no-fingers. And
getting rid of the negative logic would certainly make it easier to
understand. Good call!
Description:
Place barres on fret diagrams if they can be inferred
Please review this at https://codereview.appspot.com/294570043/
Affected files (+65, -6 lines):
A Documentation/snippets/new/automatic-fretboards-barre.ly
M scm/translation-functions.scm
Index: Documentation/snippets/new/automatic-fretboards-barre.ly
diff --git a/Documentation/snippets/new/automatic-fretboards-barre.ly
b/Documentation/snippets/new/automatic-fretboards-barre.ly
new file mode 100644
index
..6423346b1607b093d357df083bfd9fa727a7a9d1
--- /dev/null
+++ b/Documentation/snippets/new/automatic-fretboards-barre.ly
@@ -0,0 +1,22 @@
+\version "2.19.21"
+
+\header {
+ lsrtags = "fretted-strings"
+
+ texidoc = "
+When automatic fretboards are used, barre indicators will be drawn whenever
+one finger is responsible for multiple strings.
+
+If no finger indications are given in the chord from which the automatic
+fretboard is created, no barre indicators will be included, because
+there is no way to identify where barres should be placed.
+
+"
+ doctitle = "Barres in automatic fretboards"
+} % begin verbatim
+
+\new FretBoards {
+ 1
+ 1
+}
+
Index: scm/translation-functions.scm
diff --git a/scm/translation-functions.scm b/scm/translation-functions.scm
index
22f8648c31e8c0dcaacdad9a4894c6ef057bd0fc..9cd54b6ce346b594127b5ad6ba479f15cdf930fc
100644
--- a/scm/translation-functions.scm
+++ b/scm/translation-functions.scm
@@ -252,14 +252,44 @@ If the context-property @code{supportNonIntegerFret}
is set @code{#t},
micro-tones are supported for TabStaff, but not not for FretBoards."
;; helper functions
+ (define (barre-list string-frets)
+"Create a barre-list that reflects the string, fret, and finger
+entries in @var{string-frets}."
+(let* ((barres (make-array 0 5 4)) ; 5 fingers, 4 elements/barre
+ (add-string-fret
+ (lambda(sf)
+ (let ((string (car sf))
+ (fret (cadr sf))
+ (finger (if (null? (caddr sf)) 0 (caddr sf
+ (if (and (not (= fret 0)) (not (= finger 0)))
+ (begin
+ (array-set! barres 'barre finger 0)
+ (array-set! barres fret finger 3)
+ (if (or (> (array-ref barres finger 1) string)
+
Re: Context regression tests (issue 348760043 by nine.fierce.ball...@gmail.com)
On 2018/05/03 02:48:10, Dan Eble wrote: I would appreciate a close review of these tests by at least one of the long-time contributors or pro users. Contexts are a central part of LilyPond and if I've misjudged how any of these cases should work, I don't want it to slip by. Thanks. If you have suggestions on a better approach to testing, I'm all ears. James, please leave this in review until there is feedback. This is a very impressive test suite. You have done a great job of considering possibilities and developing code to demonstrate them. I'm a little bit surprised by the nature of this testing, however. I have always assumed that context id's should be unique, and that if one created two contexts of the same type with the same id, the results would be undefined. You've looked at how the code works and ferreted out the behaviors you are testing in this set of tests, but I don't believe that the behavior you have identified should be contracturally-defined behavior. In my opinion, the most ideal result when one tries to create a new context with the same type and id of an existing context would be to generate an error, something like "Error: duplicate Staff with ID of A". If we promise the behavior that your regression tests demonstrate, then we have developed an official scope for context IDs, and most LilyPond constructs do not have scope. But this is just my initial opinion, and I am certainly open to other arguments. Thanks, Carl https://codereview.appspot.com/348760043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clarify notation for slurs and beams (issue 343060043 by carl.d.soren...@gmail.com)
According to parser.yy: In line 3259, a post_event is either: 1) post_event_nofinger, or 2) '-' plus a fingering In line 3200, a post_event_nofinger is either 1) direction_less_event 2) script_dir plus a music_function 3,4 ) Lyric hyphen or lyric extender (Not relevant to this discussion). 5) script_dir plus a direction_reqd_event 6) script_dir plus a direction_less_event 7) '^' plus fingering 8) '_' plus fingering In line 3275, a direction_less_event is either 1) string_number_event 2) tremolo 3)event_function_event In line 3288, a direction_reqd_event is either 1) gen_text_def 2) script_abbreviation In line 3387, a gen_text_def is either 1) full_markup 2) STRING 3) SYMBOL 4) embedded_scm In line 3413, a fingering is 1) UNSIGNED The way I read this, NR 5.4.2 is exactly right. A direction indicator is required for 1) markups (gen_text_def case 1) 2) strings (gen_text_def case 2) 3) abbreviated forms of articulations (script_abbreviation) 4) fingerings (post_event case 2, direction_less_event case 7, direction_less_event case 8) 5) \tweak or \tag (I'm not sure about this, but I think it's gen_text_def case 4) Given this, I don't think we should say that '-' is what we use to insert post_events. I am of the opinion that we should not include '-' as a specific element in this discussion. I think it should say as-is. Thanks, Carl https://codereview.appspot.com/343060043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clarify notation for slurs and beams (issue 343060043 by carl.d.soren...@gmail.com)
Harm,
Thanks for the input. I'm not sure I agree with you on all this, but
I'm certainly open to being convinced. I've got specific replies to
your inline comments.
https://codereview.appspot.com/343060043/diff/40001/Documentation/learning/fundamental.itely
File Documentation/learning/fundamental.itely (right):
https://codereview.appspot.com/343060043/diff/40001/Documentation/learning/fundamental.itely#newcode465
Documentation/learning/fundamental.itely:465: optionally followed by one
or more post-events. Post-events add
On 2018/04/30 21:49:37, thomasmorley651 wrote:
I'd completely delete 'post-event'.
From a musical thinking it makes no sense. An articulation is not
performed
_after_ the note.
To explain it programmatical, this is not the right place, imho.
Why not simply:
"A note entry in LilyPond consists of a pitch, followed by a duration,
optionally followed by things such as articulations, fingerings,
string numbers,
slurs, ties, explanatory text, etc."
We could do this. But ultimately all the things that attach to notes
like this are called post-events in the internals reference. So I don't
think it's a bad idea to introduce the LilyPond term here, just like we
do for pitch and duration. All three terms (pitch, duration,
post-event) are LilyPond terms, not musical terms.
We aren't explaining music in this section. We are explaining LilyPond
constructs.
https://codereview.appspot.com/343060043/diff/40001/Documentation/learning/fundamental.itely#newcode481
Documentation/learning/fundamental.itely:481: Post-events follow the
note to which they are attached. Suppose we
On 2018/04/30 21:49:37, thomasmorley651 wrote:
Here as well.
Also, I think it's important to drop a sentence about the "-"-signs,
which
actually attach those optional elements to the note.
So my suggestion:
"Optional elements are added at the end of the initial
note-duration-entry.
Probably using a "-"-sign, which can be omitted, if no ambiguity
occurs.
Suppose we ..."
I don't think I agree that things are attached with "-" signs. For
example, \staccato, \mordent, \turn, \fermata, \prall, (, [. None of
these are attached with "-" signs, although they can have a direction
indicator (-, _ , ^) preceding them if desired. At least, that is what
the N.R. 5.4.2 says.
If we want to talk about direction indicators here, I think we can give
a brief introduction. If not, I think we should leave them out
completely. In the LM and the NR, the direction indicators are always
included when we add the post-events, if they are needed.
https://codereview.appspot.com/343060043/diff/40001/Documentation/learning/fundamental.itely#newcode488
Documentation/learning/fundamental.itely:488: {c'8-1--(~^\markup{"text
annotation"} c' d')}
On 2018/04/30 21:49:37, thomasmorley651 wrote:
For the sake of simplicity I'd not use direction-modifiers and enter
the text
without explicit \markup, i.e.:
{c'8-1--(~-"text annotation" c' d')}
I think it's actually nicer not to have so many "-" characters; they
make it confusing, in my opinion.
https://codereview.appspot.com/343060043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clarify notation for slurs and beams (issue 343060043 by carl.d.soren...@gmail.com)
Thanks for the feedback, Trevor.
https://codereview.appspot.com/343060043/diff/20001/Documentation/learning/fundamental.itely
File Documentation/learning/fundamental.itely (right):
https://codereview.appspot.com/343060043/diff/20001/Documentation/learning/fundamental.itely#newcode495
Documentation/learning/fundamental.itely:495: @rlearning{Adding text}.
On 2018/04/30 15:47:26, Trevor Daniels wrote:
Not sure if it makes any difference but we usually use
@ref for references within the same manual.
Fundamentals is in a different source file than common notation. If I
use @ref, it breaks the info build. So I had to use @rlearning to get
it to pass make doc.
If I'm doing something wrong, and there's some way to get @ref to work,
I'd be happy to know about it.
https://codereview.appspot.com/343060043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clarify notation for slurs and beams (issue 343060043 by carl.d.soren...@gmail.com)
On 2018/04/30 12:08:20, Trevor Daniels wrote:
https://codereview.appspot.com/343060043/diff/1/Documentation/learning/fundamental.itely#newcode488
Documentation/learning/fundamental.itely:488: @end lilypond
I'd prefer \relative or nothing in this example. \absolute
has not been introduced earlier, in fact it's not used at
all in the LM.
Good idea. I went with nothing.
https://codereview.appspot.com/343060043/diff/1/Documentation/learning/fundamental.itely#newcode492
Documentation/learning/fundamental.itely:492: @ruser{Expressive
marks},
Might be good to point to the relevant sections in the LM
as well, in Section 2.1
Also a good idea. I've added the references.
Thanks,
Carl
https://codereview.appspot.com/343060043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clarify notation for slurs and beams (issue 343060043 by carl.d.soren...@gmail.com)
Reviewers: Be-3, Trevor Daniels,
Message:
On 2018/04/30 11:37:09, Be-3 wrote:
Hi Carl,
Concise, comprehensible, - LGTM!
Perhaps it should be explicitly pointed out that the duration
shorthand does not
work for rests.
There have been some misconceptions lately on the user list, and so I
think this
detail probably deserves special attendance.
I've added a sentence that should clarify.
https://codereview.appspot.com/343060043/diff/1/Documentation/learning/fundamental.itely#newcode487
Documentation/learning/fundamental.itely:487: \absolute
{c'8-1--(~^\markup{"text
annotationi"} c' d')}
Shouldn't it be "text annotation"?
Yes. Now you know which editor I was using to create the patch -- it's
vim leftovers.
Thanks for the catch.
Description:
Clarify notation for slurs and beams
Note that the opening code for slurs and beams comes after
the first note of the slur or beam.
Also, add a section about notes containing pitches, durations,
and post-events.
Please review this at https://codereview.appspot.com/343060043/
Affected files (+41, -2 lines):
M Documentation/learning/common-notation.itely
M Documentation/learning/fundamental.itely
Index: Documentation/learning/common-notation.itely
diff --git a/Documentation/learning/common-notation.itely
b/Documentation/learning/common-notation.itely
index
b9b62cf3b321452ec117290c7f9b68cf4246e39b..a6136a421a0b84f854d793a060e17b8551ee5eb6
100644
--- a/Documentation/learning/common-notation.itely
+++ b/Documentation/learning/common-notation.itely
@@ -275,6 +275,7 @@ Notation Reference:
@funindex ( ... )
@funindex \( ... \)
+
@menu
* Ties::
* Slurs::
@@ -318,7 +319,8 @@ Music Glossary: @rglos{slur}.
A @notation{slur} is a curve drawn across many notes. The
starting note and ending note are marked with @code{(} and
-@code{)} respectively.
+@code{)} respectively. Note that the @code{(} comes after
+the first note of the slur.
@lilypond[verbatim,quote]
\relative { d''4( c16) cis( d e c cis d) e( d4) }
@@ -369,6 +371,9 @@ Notation Reference:
@node Articulation and dynamics
@subsection Articulation and dynamics
+Articulations and dynamics are indicated by adding special codes
+after the notes to which they apply,
+
@menu
* Articulations::
* Fingerings::
@@ -526,6 +531,7 @@ All @notation{beams} are drawn automatically:
If you do not like the automatic beams, they may be overridden
manually. To correct just an occasional beam mark the first note
to be beamed with @code{[} and the last one with @code{]}.
+Note that @code{[} comes after the first beamed note.
@lilypond[verbatim,quote]
\relative { a'8[ ais] d[ ees r d] c16 b a8 }
@@ -597,7 +603,7 @@ Music Glossary: @rglos{note value}, @rglos{triplet}.
@notation{Tuplets} are made with the @code{\tuplet} keyword. It
takes two arguments: a fraction and a piece of music. The
-fraction is the number of tuplet notes over the number
+fraction is the number of tuplet notes over the number
of notes normally filling the same duration.
For triplets, there are three notes instead of two, so
@notation{triplets} have 3/2 as their fraction.
Index: Documentation/learning/fundamental.itely
diff --git a/Documentation/learning/fundamental.itely
b/Documentation/learning/fundamental.itely
index
40efc3777ab567d7f440a360839e4a3d1570fa25..fc8290a14314c2b916393e414373aad305a2e301
100644
--- a/Documentation/learning/fundamental.itely
+++ b/Documentation/learning/fundamental.itely
@@ -40,6 +40,7 @@ description of the input format, see @ruser{File
structure}.
* Introduction to the LilyPond file structure::
* Score is a (single) compound musical expression::
* Nesting music expressions::
+* Structure of a note entry::
* On the un-nestedness of brackets and ties::
@end menu
@@ -457,6 +458,38 @@ These require further commands which
have not yet been introduced. See @ref{Size of objects},
and @ruser{Ossia staves}.
+@node Structure of a note entry
+@subsection Structure of a note entry
+
+A note entry in LilyPond consists of a pitch, followed by a duration,
+optionally followed by one or more post-events. Post-events add
+things such as articulations, fingerings, string numbers, slurs, ties
+and explanatory text.
+
+The pitch may be explicitly defined using the current LilyPond input
+language as described in @ruser{Note names in other languages}. The
+pitch may be omitted. If the pitch is omitted, the pitch of a current
+note will be the same as the pitch of the previous note in the input
+file, see @ruser{Durations}.
+
+The duration includes a number and optionally one or more dots. If a
+duration is not explicitly defined, the duration of a current note will
+be the same as the duration of the previous note, chord, rest, or spacer
+rest, see @ruser{Durations}.
+
+Post-events follow the note to which they are attached. Suppose we
+want to have an eighth note c' with a fingering of 1, a tenuto
+articulation, a slur beginning with the note, a tie beginning with
+the note,
Re: Syntax: lyric_mode_music might be a MUSIC_FUNCTION (issue 343820043 by knup...@gmail.com)
On 2018/04/25 12:22:18, knupero wrote: @Everyone: I won't bother you here any longer. Knut, It's not a bother. But it's important to recognize that the parser is a difficult system to work with, and we've had lots of problems over the years with lookahead not being handled properly. So the issue that David raises here about lookahead is a real issue. Your proposal is to allow music functions enclosed in braced lists, but I don't know that there is anything in the parser that explicitly says it needs a braced list. I can see the value of having lyrics provided by a music function (for instance, if you want to sing solfege). It's just important to do it properly so it works well in the parser. And David has spent a lot of time trying to make the parser work in an explainable and predictable manner. And this means that there are dozens of things that didn't work well in 2.14 that now are trivial to do in 2.19. So hang in there, and have the discussions, and I'm convinced there is a way to make both you and David happy. And LilyPond will be better because of it. Thanks, Carl https://codereview.appspot.com/343820043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue 5312: Key cancellation glyph position inconsistent (issue 343020043 by torsten.haemme...@web.de)
On 2018/04/24 18:43:45, Be-3 wrote: The intervals are just *approximating* the outlines of a run-of-the mill natural glyph. I even played around with the concept using squared paper. This approach more or less relies on the fact that the square/parallelogram part of a natural glyph will be one stave-space high independent of the font used, just like a notehead in *any* font will be one stave-space high. So it's a reasonable approximation that the box part of the natural will extend about 3/4 staff spaces above the staff position of the natural, and the descender will descend about 1.5 staff spaces below. Seems like a reasonable estimate to me, even if it's not exact for a particular font. You can't get too far away from that and still fit in a staff. Thanks, Carl https://codereview.appspot.com/343020043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: issue 5312: Key cancellation glyph position inconsistent (issue 343020043 by torsten.haemme...@web.de)
LGTM. I am just a *little* bit concerned about having the dimensions of the Emmentaler natural glyph hardcoded in the source, but we already have magic numbers reflecting the characteristics of the Emmentaler glyphs. Maybe it would be good to put a FIXME in recognizing this fact. Or maybe we just go as-is. https://codereview.appspot.com/343020043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5310: find_top_context () maintenance (issue 341150043 by nine.fierce.ball...@gmail.com)
It seems like if there is a problem that this solves, there should be a regression test that shows the problem and hence why this patch is needed. https://codereview.appspot.com/341150043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow event functions from partial functions or partial markups (issue 336670043 by d...@gnu.org)
On 2018/03/20 20:31:48, thomasmorley651 wrote: problem with \etc. So I vote for putting all in 2.20. I concur. https://codereview.appspot.com/336670043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5284: improve ASSIGN_EVENT_ONCE (issue 338540043 by nine.fierce.ball...@gmail.com)
I haven't tested the functionality, but I noticed that the warning
messages were not consistent with the LilyPond guidelines on
localization.
https://codereview.appspot.com/338540043/diff/20001/lily/stream-event.cc
File lily/stream-event.cc (right):
https://codereview.appspot.com/338540043/diff/20001/lily/stream-event.cc#newcode146
lily/stream-event.cc:146: oc.c_str ()));
According to CG 10.5.8 Localization it would probably be better to write
the warnings like this:
("conflict with event : `%s'", oc.c_str ())
("discarding event: `%s'", nc.c_str ())
https://codereview.appspot.com/338540043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add maximumFretStretch to the read properties of Tab_note_heads_engraver (issue 334430043 by thomasmorle...@gmail.com)
LGTM https://codereview.appspot.com/334430043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow left-handed fret-markups (issue 339270043 by thomasmorle...@gmail.com)
Looks good to me, but I have one suggestion. Thanks, Carl https://codereview.appspot.com/339270043/diff/1/scm/fret-diagrams.scm File scm/fret-diagrams.scm (right): https://codereview.appspot.com/339270043/diff/1/scm/fret-diagrams.scm#newcode365 scm/fret-diagrams.scm:365: ((and (eq? orientation 'landscape) left-handed) I would tend to write this as ((eq? orientation 'landscape) (cons fret-coordinate (if left-handed (- (1- string-count) string-coordinate) (- string-coordinate (1- string-count))) (eq? orientation 'opposing-landscape) (cons (- fret-coordinate) (if left-handed (- string-coordinate (1- string-count)) (- (1- string-count) string-coordinate))) (else (cons (if left-handed (- string-coordinate) string-coordinate) (- fret-coordinate) I think it shows the structure better (i.e. it shows three different orientations, and it explicitly shows where the left-handed changes things (y coordinate for landscape, x coordinate for regular). But I don't insist on this by any means. https://codereview.appspot.com/339270043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5264: fret diagram nut alignment (issue 335430043 by lilyp...@maltemeyn.de)
On 2018/01/18 19:42:01, thomasmorley651 wrote: I don't think this is the right approach to make left-handed fret-diagrams work. Ofcourse it cures a single problem while using negative string-distance, but other problems are shining up as already mentioned. Instead I think we should disallow negative string-distance, or rather (because LilyPond does no type-checking on sub-properties) always use the (abs ..). For the left-handed fret-diagrams, why not place strings (and frets) correctly right from the start and let do the already existing code all the other work? I'll post a patch following this route on the tracker. I agree that the negative string-distance is not the right way to create left-handed fret diagrams. The proper approach is to introduce a property that indicates the diagram should be left-handed, and to make the stencil-coordinates function in scm/fret-diagrams.scm respect that property. https://codereview.appspot.com/335430043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5187 Add command for Thin Aiken noteheads (issue 326510043 by karlinh...@gmail.com)
LGTM https://codereview.appspot.com/326510043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 5180: Require \version statement in main file (issue 325640043 by d...@gnu.org)
LGTM https://codereview.appspot.com/325640043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add regtest for issue 5181 (issue 327470043 by d...@gnu.org)
LGTM https://codereview.appspot.com/327470043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Don't let event-chord-reduce bomb on empty chords (issue 327480043 by d...@gnu.org)
LGTM https://codereview.appspot.com/327480043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Use -b together with -dgs-never-embed-fonts (issue 325630043 by knup...@gmail.com)
On 2017/09/24 14:32:02, trueroad wrote: * -dgs-neverembed-fonts Add behavior Current behavior - For PDF output, It does not embed fonts except TrueType fonts. Added behavior - Define and use some encodings for Emmentaler. (Also using "show" instead of "glyphshow") - For PDF output, It does not embed fonts except TrueType fonts. This option is for non-embedded font intermediate PDFs, and later uses Ghostscript to embed missing fonts. This method needs to pass the missing fonts to Ghostscript. In other words, it is for specialists. However, if it is used properly, it works even with the current Ghostscript. So if I understand this correctly, properly using -dgs-neverembed-fonts as part of our manual build process will give us *both* smaller intermediate files and a small final file? That sounds perfect to me! Carl https://codereview.appspot.com/325630043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5187 Add command for Thin Aiken noteheads (issue 326510043 by karlinh...@gmail.com)
On 2017/09/20 23:38:35, karlinhigh wrote: > On 2017/09/20 17:48:57, Carl wrote: > the documentation (Shape Note Heads, in NR 1.1.4 should also be changed I am uncertain how to go ahead here. Literally, just add one line of code to the example in Shape Note Heads, in NR 1.1.4., that uses either \aikenThinHeads or \aikenThinHeadsMinor (no need to include both). No change in the text is necessary. Then, any comments on how much documentation should be written? Would adding \aikenThinHeads and \aikenThinHeadsMinor to the list of "Predefined commands" be sufficient? Or should a snippet be included with two systems of c-major-scale white-head notes, one for each Aiken variant? All we want is a single demonstration of the command, not an exhaustive demonstration. The documentation policy is to show, don't tell, and to remove as much as possible. We assume the reader to be intelligent and able to extrapolate from given examples. The NR is, by design, NOT a tutorial. Literally, I think you should add two lines to the example -- one line for the \aikenThinHeads, and one line that is a duplicate of the scale shown for all of the other heads. THanks, Carl https://codereview.appspot.com/326510043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 5187 Add command for Thin Aiken noteheads (issue 326510043 by karlinh...@gmail.com)
This change to property-init.ly looks good. But the documentation (Shape Note Heads, in NR 1.1.4 should also be changed to show the use of either \aikenThinHeads or \aikenThinHeadsMinor. Thanks, Carl https://codereview.appspot.com/326510043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorle...@gmail.com)
After reviewing the slur code, I remove my objections to using grob.line-thickness in this patch. https://codereview.appspot.com/326970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorle...@gmail.com)
I believe that line-thickness should not be changed for the grob; just thickness. line-thickness should be the staff line-thickness, not changed per grob, IMO. https://codereview.appspot.com/326970043/diff/40001/lily/arpeggio.cc File lily/arpeggio.cc (right): https://codereview.appspot.com/326970043/diff/40001/lily/arpeggio.cc#newcode214 lily/arpeggio.cc:214: Real lt I think lt should just be line-thickness, and th should be thickness*line-thickness https://codereview.appspot.com/326970043/diff/40001/lily/arpeggio.cc#newcode261 lily/arpeggio.cc:261: "line-thickness " I think that only thickness should be added https://codereview.appspot.com/326970043/diff/40001/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/326970043/diff/40001/scm/define-grobs.scm#newcode175 scm/define-grobs.scm:175: (line-thickness . 1) I don't think line-thickness should be added here; you should just use thickness https://codereview.appspot.com/326970043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Enhance the -dpreview method for SVG output (issue 326960043 by pkx1...@gmail.com)
Oh, now I read the first message and see that the code doesn't work. My first attempt at getting the code to work would be to change "stensil" to "stencil" and see if that fixed things. https://codereview.appspot.com/326960043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Enhance the -dpreview method for SVG output (issue 326960043 by pkx1...@gmail.com)
I have listed some specific changes that must be made (move to the proper alphabetical order) and raised questions about how the code works with an apparently misspelled argument. I believe both of those need to be fixed before pushing the patch. I would also like to see crop apply to at least the pdf backend in addition to svg, if it's not too much work. https://codereview.appspot.com/326960043/diff/1/Documentation/usage/running.itely File Documentation/usage/running.itely (right): https://codereview.appspot.com/326960043/diff/1/Documentation/usage/running.itely#newcode683 Documentation/usage/running.itely:683: @tab Match the size of the normal output to the typeset image for SVGs. This should be placed in alphabetical order (about line 532) I would prefer to see the description be something like Match the size of the normal output to the typeset image (currently only implemented for svg backend). Actually, I'd even more prefer to have the crop function work on the pdf backend as well as the svg backend. Have you investigated whether your code could work on PDF as well? Also, the placement of crop in this file causes reading problems, because it's placed in the middle of preview, and the text below says "This option is supported by all backends", which applies to preview, but not to crop. https://codereview.appspot.com/326960043/diff/1/scm/framework-svg.scm File scm/framework-svg.scm (right): https://codereview.appspot.com/326960043/diff/1/scm/framework-svg.scm#newcode173 scm/framework-svg.scm:173: (define (dump-page-crop paper filename page page-number page-count stensil) Hmm -- the argument here is spelled "stensil" https://codereview.appspot.com/326960043/diff/1/scm/framework-svg.scm#newcode178 scm/framework-svg.scm:178: (x-extent (ly:stencil-extent stencil X)) But here it is spelled "stencil" How does this work? Perhaps the stencil argument is unnecessary and not doing anything here? I'm confused. https://codereview.appspot.com/326960043/diff/1/scm/lily.scm File scm/lily.scm (right): https://codereview.appspot.com/326960043/diff/1/scm/lily.scm#newcode341 scm/lily.scm:341: (crop This should be put in the proper alphabetical order (up at line 231), rather than placed under preview https://codereview.appspot.com/326960043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implementing Chord Semantics as a part of the EventChord structure, (issue 321250043 by chazwi...@gmail.com)
Looks generally good to me. It's not yet complete, so I don't think
it's a candidate for pushing yet. But I think you've got the right
stuff in and are moving forward well.
Good job!
https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):
https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc#newcode99
lily/chord-name-engraver.cc:99: SCM name_proc = get_property
("chordSemanticsNameFunction");
On 2017/07/06 21:57:23, Dan Eble wrote:
1. Should chordSemanticsNameFunction be included in the list at the
bottom of
this file? (I think so.)
chordSemanticsNameFunction should definitely be included as a property
that is read.
https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc#newcode103
lily/chord-name-engraver.cc:103: // Ugh, we created a grob, now we
better populate it.
On 2017/07/06 21:57:23, Dan Eble wrote:
If this was not expected, then as a user, I would like to see lilypond
emit a
warning.
I agree.
https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm
File scm/chord-entry.scm (right):
https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode93
scm/chord-entry.scm:93: (interpret-additions (cons
(make-chord-entry-from-pitch (car mods))
To let you fit on an 80-column line, you may wish to move (cons down to
the next line and indent it two spaces. The other arguments to
interpret additions would then align with (cons
https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode163
scm/chord-entry.scm:163: ;; TODO: this omits 3 in power chord. Change to
only do so if lead-mod is null.
Nice to note this; please make sure to add the extra clause in the if
here.
https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode206
scm/chord-entry.scm:206: (set! complete-chord (map (lambda (x)
(chord-pitch-transpose x root))
Maybe the name chord-pitch-transpose should be changed, since two things
are happening in that function: 1) The proper pitches are being created
(using ly:pitch-transpose) and
2) The given chord steps are being added.
Maybe make-chord-step or just chord-step
(set! complete-chord (map (lambda (x) (chord-step x root))
complete-chord)
seems to be a bit clearer to me. As it is, I wondered why you created
your own transpose function instead of using ly:pitch-transpose. And
then I saw it was because the chord has both pitches and steps now.
https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode309
scm/chord-entry.scm:309: ;; chord modifiers change the pitch list.
maybe change the comment to say chord modifiers change the pitch list
and the chord steps?
https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode349
scm/chord-entry.scm:349: (maj . , maj7-modifier)
I thought you were going to adjust this so maj was not maj7. That way
c:5 could be power chord, and c:maj could be major triad. c:maj7 would
be a major 7.
Is this plan gone now?
https://codereview.appspot.com/321250043/diff/1/scm/define-context-properties.scm
File scm/define-context-properties.scm (right):
https://codereview.appspot.com/321250043/diff/1/scm/define-context-properties.scm#newcode202
scm/define-context-properties.scm:202: (chordSemanticsNameFunction
,procedure? "The function that converts
Should be "A" function, not "The" function, IMO. See the parallel with
chordNoteNamer
https://codereview.appspot.com/321250043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Change all instances of \partcombine to \partCombine in the documentation (issue 326870043 by pkx1...@gmail.com)
On 2017/07/10 14:09:53, dak wrote: https://codereview.appspot.com/326870043/diff/1/Documentation/snippets/changing-partcombine-texts.ly File Documentation/snippets/changing-partcombine-texts.ly (right): https://codereview.appspot.com/326870043/diff/1/Documentation/snippets/changing-partcombine-texts.ly#newcode24 Documentation/snippets/changing-partcombine-texts.ly:24: \partCombine On 2017/07/10 12:50:12, PhilEHolmes wrote: > If files in the snippets directory are changed, the changes will be reverted as > soon as anyone runs makelsr. The best way to change snippets is to edit the LSR > and then follow the CG instructions for transferring those changes over to the > docs. HTH. This is not an option since snippets in the LSR are supposed to be valid for an older version of LilyPond. Instead you have to rely on the convert-ly run that makelsr.py does. I understood that the CG also says you can place an edited version of the snippet in Documentation/snippets/new with a lilypond version of the current development version. Is this incorrect? https://codereview.appspot.com/326870043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various chain-assoc-get -> #:properties (issue 323940043 by d...@gnu.org)
LGTM. One small possible change. https://codereview.appspot.com/323940043/diff/20001/Documentation/snippets/new/three-sided-box.ly File Documentation/snippets/new/three-sided-box.ly (right): https://codereview.appspot.com/323940043/diff/20001/Documentation/snippets/new/three-sided-box.ly#newcode36 Documentation/snippets/new/three-sided-box.ly:36: (let* ((pad (* (magstep font-size) box-padding)) Looks like this could now be a let instead of let* https://codereview.appspot.com/323940043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix typos in \offset documentation (issue 322040043 by david.nales...@gmail.com)
LGTM. THanks for doing this. https://codereview.appspot.com/322040043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-manuals.css: edit color scheme and some spacing (issue 322070043 by paulwmor...@gmail.com)
I don't feel strongly about the old design being bad. The new design looks mostly fine to me. I feel like the table of contents bar at the left is too wide. But I would be fine with it being like this. Carl https://codereview.appspot.com/322070043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add guile-config-1.8 etc. searching (issue 320830043 by truer...@gmail.com)
LGTM. And the reformatting is very nice. Thanks https://codereview.appspot.com/320830043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow 'staff-padding to work with MeasureCounter (issue 312580043 by david.nales...@gmail.com)
On 2017/03/29 23:28:40, david.nalesnik wrote: This is a simple bugfix of something that should have worked since the MeasureCounter was introduced in 2.17.something. As such, I don't think a Changes entry is warranted. I would propose simply that I fix that bug only, nothing else -- no extra snippet, no changes to the existing snippets (which are already dense enough with information). So I am inclined to upload a patch just with the scm/define-grobs.scm alterations. I don't think it needs a Changes entry. But there should be a regression test that shows the proper function of the 'staff-padding property on MeasureCounter. That way, if it were to break again in the future, we would know it. We don't need a documentation snippet, but we do need a regtest. https://codereview.appspot.com/312580043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc CG 6.1: Add caveat on website work (issue 315130043 by gra...@percival-music.ca)
LGTM. Thanks! https://codereview.appspot.com/315130043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
On 2016/09/08 03:28:49, pwm wrote: Thanks for the feedback everyone! On 2016/09/06 16:39:22, Carl wrote: > > I think the only thing remaining is to get the best possible example for the > front page. Any suggestions anyone? I'm fine with the Bach one or whatever people think is best. One of the existing ones would be simplest of course. How about the BWV 861 example from the essay? http://lilypond.org/doc/v2.18/Documentation/essay/engraved-examples-_0028bwv-861_0029 It shows fewer features (e.g. instrument names, score title) but shows a beautiful and complex score. Carl https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
On 2016/09/06 15:49:43, pwm wrote: Screenshot: https://drive.google.com/open?id=0ByNTIEA63_a_ZzMtNTFiZHFqN2c I have not built and tested the webpage, but I think this is exactly the way to go. Show an example, and let the user select any other examples they find interesting. I think the only thing remaining is to get the best possible example for the front page. Thanks, Carl https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Web: home page: add examples/images, reduce news to headlines (issue 306350043 by paulwmor...@gmail.com)
I'm not comfortable with the really long, scrolling, home page. I think we should make the home page so that it is basically a one-screen page on a "typical" computer display, and have the full example page be a link titled "more" or something like that. Carl https://codereview.appspot.com/306350043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Convert a bunch of C++ internals to degrees rather than radians (issue 305380043 by d...@gnu.org)
LGTM. Nicely done. https://codereview.appspot.com/305380043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: CG update Indenting with vim section (issue 302340043 by mark.opu...@googlemail.com)
Thanks for doing this -- it helps me understand better how things work, and gives an example of a more robust set of vim settings. https://codereview.appspot.com/302340043/diff/1/Documentation/contributor/programming-work.itexi File Documentation/contributor/programming-work.itexi (left): https://codereview.appspot.com/302340043/diff/1/Documentation/contributor/programming-work.itexi#oldcode394 Documentation/contributor/programming-work.itexi:394: autocmd BufWritePre * :%s/\s\+$//e I think we should leave the removal of trailing whitespace in our recommended .vimrc This eliminates the problem of whitespace errors (which git will track and complain about). I agree that it doesn't belong in the indenting section, but I think it should still be part of our recommendations for using vim. https://codereview.appspot.com/302340043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 4936: look up "mf" for default initial volume (issue 308890043 by nine.fierce.ball...@gmail.com)
On 2016/08/04 21:23:12, ht wrote: LGTM except for a small question (still) about the process_music logic. Also, I noticed the following definition in scm/midi.scm: ;; 90 == 90/127 == 0.71 is supposed to be the default value ;; urg: we should set this at start of track (define-public dynamic-default-volume 0.71) It would seem to me that the Dynamic_performer should use the scheme variable dynamic-default-volume as the default, rather than a hard-coded lookup of mf. In my opinion, we should not have any constants that a user might want to change hard-coded in C++, because then the user can't change them. I'd also be delighted to have the default value of dynamic-default-volume be set to the lookup of mf in scm/midi.scm https://codereview.appspot.com/308890043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: Add a section about handling MIDI dynamics with multiple voices (issue 302930043 by ht.lilypond.developm...@gmail.com)
Heikki, this is so much better than your first patch. Thanks for
working on it!
As I mention in the comments, I think the current behavior of changing
dynamics in identically-named but distinct voices is flawed, so I think
it should be a known issue, not in the main documentation. But I like
the main documentation entry about the default value; I think it belongs
right where it is and is a great addition to our documentation.
Thanks,
Carl
https://codereview.appspot.com/302930043/diff/20001/Documentation/notation/input.itely
File Documentation/notation/input.itely (right):
https://codereview.appspot.com/302930043/diff/20001/Documentation/notation/input.itely#newcode3028
Documentation/notation/input.itely:3028: value 0.71 in the available
MIDI volume range between 0 and 1.
I think that it's useful to know what the default value is. But I also
think we should have a comment that indicates it is defined in
scm/midi.scm It would probably also be useful to give the name of the
variable that holds the default value, because the user can specify any
desired default value by changing the variable.
And it might be useful to show how to use
default-dynamic-absolute-volume to set the default value to one of the
dynamic levels (it's currently between mf and f).
https://codereview.appspot.com/302930043/diff/20001/Documentation/notation/input.itely#newcode3030
Documentation/notation/input.itely:3030: identically named voices in the
same @code{Staff}.
I hadn't realized this feature of MIDI. I believe this behavior to be a
bug. Dynamic changes should only apply to a specific voice, and as is
made clear in
http://lilypond.org/doc/v2.18/Documentation/notation/creating-and-referencing-contexts
,
\new will always create a new context. The fact that a new context is
created is demonstrated in the first note of the new voice A getting the
default value, rather than the value.
I believe it is a flaw in the MIDI performer to allow any Voice in a
Staff with the same name to share MIDI dynamics.
In your code, if you change the second A voice name to AA, the c1 on
line 3052 would have the dynamic level of .
I appreciate your effort to document the fact that a default dynamic
level exists. I think that is important. I also appreciate your effort
to document the current behavior of LilyPond, but I don't think we
should do it, because I think it's a bug and we shouldn't document bug
behavior in the main body of the manual (I don't think it's intended
behavior; at least I wouldn't be in favor of that behavior).
I think that the behavior indicated in your example should be documented
as a Known Issue, which means we recognize it as a bug. And we should
have an issue as well (MIDI volume levels leak between identically named
voices).
https://codereview.appspot.com/302930043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: Add a section about handling MIDI dynamics with multiple voices (issue 302930043 by ht.lilypond.developm...@gmail.com)
This is too wordy for the notation reference. We need to have a minimum number of words in the NR, and instead show things by examples. It should just have examples that show the problem and the solution. Since we can't see the MIDI output in the manual, there should be a statement of what happens in the MIDI. Thanks, Carl https://codereview.appspot.com/302930043/diff/1/Documentation/notation/input.itely File Documentation/notation/input.itely (right): https://codereview.appspot.com/302930043/diff/1/Documentation/notation/input.itely#newcode3022 Documentation/notation/input.itely:3022: @node MIDI dynamics and voices This section is too tutorial in language for the notation reference. Such a section should be added to the Learning Manual, if it is needed. Otherwise, this section should be greatly reduced, to just show the code that introduces the problem and alternative code that can avoid the problem. https://codereview.appspot.com/302930043/diff/1/Documentation/notation/simultaneous.itely File Documentation/notation/simultaneous.itely (right): https://codereview.appspot.com/302930043/diff/1/Documentation/notation/simultaneous.itely#newcode410 Documentation/notation/simultaneous.itely:410: This section presents the ways to express single-staff This should actually just be a SeeAlso, as you have listed below. We don't need this paragraph in the notation reference. The Notation Reference is not a tutorial; it needs to be as brief as possible. https://codereview.appspot.com/302930043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add halfopenvertical to script.scm (issue 297340043 by carl.d.soren...@gmail.com)
The error that was identified in make doc actually resolves the problem Harm was concerned about. If a script is added to scripts.scm as an articulation, and that articulation is not added to Documentation/included/script-chart.ly, then make doc fails. So I think we are covered for some automated method of verifying that all articulations listed in scripts.scm is in fact tested by the build process (although not in the regression tests). Thanks, Carl https://codereview.appspot.com/297340043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Let define-session-public place variables natively into parser (issue 300110043 by d...@gnu.org)
On 2016/05/27 06:03:44, dak wrote: On 2016/05/27 04:23:29, Carl wrote: > Looks very nice to me. I couldn't have done it, but I love how it works. Can you elaborate on the "couldn't have done it" part? define-session-public copies some ugly code (concerning undead structures or something) from define-session. Is it that, or the heavily underdocumented module system of Guile that's involved here? Because I have a patch reorganizing the session stuff in limbo that would remove/replace the undead machinery with something saner. Possibly giving fewer useful diagnostics, but we haven't had any _useful_ hint from that area for years. It's the module system of Guile. I have no idea how the module system works. I'm vaguely aware that there are modules, and that variables are scoped locally to the modules, but that's about it. To be fair, I haven't spent any time digging in to that aspect of LilyPond. I assume that if I dug into it I could learn it, but that's beyond my current understanding. Thanks, Carl https://codereview.appspot.com/300110043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Let define-session-public place variables natively into parser (issue 300110043 by d...@gnu.org)
Looks very nice to me. I couldn't have done it, but I love how it works. Thanks, Carl https://codereview.appspot.com/300110043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add halfopenvertical to script.scm (issue 297340043 by carl.d.soren...@gmail.com)
On 2016/05/24 23:57:38, Carl wrote: The challenge is that some of the feta font elements in the scripts.* familiy are used as articulations, while others (e.g. caesura) are not. And the ones not used as articulations are not included in script.scm by choice. I guess my wording was bad. It's not a question of use as articulations. It's a question of being part of the Score context property scriptDefinitions. And I'm not sure what the criteria are for including a scripts.* glyph in the scriptDefinitions property. So I think I don't know exactly what the right thing to do here is. I'd welcome any input. Thanks, Carl https://codereview.appspot.com/297340043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
