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) ||
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):
> >
> >
>
LGTM
Makes the code much easier to read.
Carl
https://codereview.appspot.com/565920043/
LGTM
Carl
https://codereview.appspot.com/583750043/
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
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):
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/
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#.
>
> >
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
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
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/
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
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
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
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/
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/
Very nice.
I think there should be a changes.tely entry.
Carl
https://codereview.appspot.com/577130043/
Very nice.
I think there should be a changes.tely entry.
Carl
https://codereview.appspot.com/577130043/
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):
LGTM.
Thanks for working on these annoying details!
Carl
https://codereview.appspot.com/559260043/
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
LGTM. Nice work!
Carl
https://codereview.appspot.com/554960043/
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
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
LGTM, although I haven't tested it.
Thanks, for working on this!
Carl
https://codereview.appspot.com/554930043/
LGTM
https://codereview.appspot.com/566930043/
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
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
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
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/
___
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
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
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.
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
LGTM
https://codereview.appspot.com/562550043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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
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
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
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
LGTM.
Carl
https://codereview.appspot.com/369840043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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
LGTM
https://codereview.appspot.com/349740043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
LGTM
https://codereview.appspot.com/357740043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
LGTM.
https://codereview.appspot.com/343270043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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
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
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
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
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
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
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/
LGTM
https://codereview.appspot.com/344910043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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,
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
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
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)
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
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
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
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
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
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
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
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
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
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):
LGTM
https://codereview.appspot.com/334430043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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?
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
LGTM
https://codereview.appspot.com/326510043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
LGTM
https://codereview.appspot.com/325640043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
LGTM
https://codereview.appspot.com/327470043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
LGTM
https://codereview.appspot.com/327480043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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,
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
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/
___
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
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):
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
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
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
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):
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):
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
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/
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
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
LGTM.
Thanks!
https://codereview.appspot.com/315130043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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.
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
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
LGTM. Nicely done.
https://codereview.appspot.com/305380043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
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):
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
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
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
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
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)
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
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.
1 - 100 of 1173 matches
Mail list logo
