Re: Let \footnote do the job of \footnote, \footnoteGrob, \autoFootnote and \autoFootnoteGrob (issue 5527058)
Thanks, David! http://codereview.appspot.com/5527058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Let \footnote do the job of \footnote, \footnoteGrob, \autoFootnote and \autoFootnoteGrob (issue 5527058)
http://codereview.appspot.com/5527058/diff/1/python/convertrules.py File python/convertrules.py (right): http://codereview.appspot.com/5527058/diff/1/python/convertrules.py#newcode3362 python/convertrules.py:3362: From an orthogonal point of view, those variables should be either named `matchstring' and `matcharg' or `matchastring' and `matchanarg'... It's not really important, but I prefer the former. http://codereview.appspot.com/5527058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Sketch for DotColumn not triggering vertical alignment. (issue 5538049)
LGTM, but not time to test... http://codereview.appspot.com/5538049/diff/1/lily/staff-symbol-referencer.cc File lily/staff-symbol-referencer.cc (right): http://codereview.appspot.com/5538049/diff/1/lily/staff-symbol-referencer.cc#newcode95 lily/staff-symbol-referencer.cc:95: Real y = (pure The outermost parentheses have no effect. I suggest to remove them. http://codereview.appspot.com/5538049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Undefined references in translated manuals (issue 2220). (issue 5539052)
Julien, please apply such obvious (and trivial) fixes directly to lilypond without creating a Rietveld issue. http://codereview.appspot.com/5539052/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Simplify font building. (issue 5695061)
LGTM. http://codereview.appspot.com/5695061/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Get texidoc translations out of snippets source files (issue 6351083)
http://codereview.appspot.com/6351083/diff/1/Documentation/contributor/lsr-work.itexi
File Documentation/contributor/lsr-work.itexi (right):
http://codereview.appspot.com/6351083/diff/1/Documentation/contributor/lsr-work.itexi#newcode209
Documentation/contributor/lsr-work.itexi:209: e.g. 2011-12-25.
@command{make} is included in this sequence so that
Please use two spaces after a full dot everywhere (and please check your
whole patch for other occurrences).
http://codereview.appspot.com/6351083/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes all black bars in NR (issue 6345088)
LGTM. Thanks a lot! http://codereview.appspot.com/6345088/diff/1/Documentation/snippets/simultaneous-headword.ly File Documentation/snippets/simultaneous-headword.ly (right): http://codereview.appspot.com/6345088/diff/1/Documentation/snippets/simultaneous-headword.ly#newcode16 Documentation/snippets/simultaneous-headword.ly:16: #'((alignment-distances .(12))) Why removing the space after the dot? http://codereview.appspot.com/6345088/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Check in Texinfo 2012-07-03.16 (issue 6374068)
I'm currently in Japan, with bad internet access (and additionally, I can only use the web interface for writing emails, which I heartily dislike, since SMTP doesn't work here in the hotel for yet unknown reasons), so it will take some time until I'm able to test this and communicate with the lilypond community again. Sorry for this delay. http://codereview.appspot.com/6374068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Check in Texinfo 2012-07-03.16 (issue 6374068)
http://codereview.appspot.com/6374068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix Issue 2146 Illegal entry in bfrange block in ToUnicode CMap (issue 6399046)
This looks great! I don't have time right now to test it, but errors will be quite easily be caught :-) Thanks for fixing this. http://codereview.appspot.com/6399046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add support for Cyrillic in texinfo. (issue 6459081)
Reviewers: , Message: It would be nice if a Russian speaking person could fix the Cyrillic index entries. However, since we don't have Cyrillic characters in the index, this is not an urgent issue (and maybe we never need this). Description: Add support for Cyrillic in texinfo. Please review this at http://codereview.appspot.com/6459081/ Affected files: A Documentation/cyrillic.itexi M Documentation/macros.itexi ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add support for Cyrillic in texinfo. (issue 6459081)
http://codereview.appspot.com/6459081/diff/1/Documentation/cyrillic.itexi
File Documentation/cyrillic.itexi (right):
http://codereview.appspot.com/6459081/diff/1/Documentation/cyrillic.itexi#newcode14
Documentation/cyrillic.itexi:14: \gdef\cyrfont{%
I fully agree. However, I'm simply mimicking the EC support in texinfo,
and I'm not going to rewrite font support (which is something urgently
needed, BTW).
http://codereview.appspot.com/6459081/diff/1/Documentation/macros.itexi
File Documentation/macros.itexi (right):
http://codereview.appspot.com/6459081/diff/1/Documentation/macros.itexi#newcode14
Documentation/macros.itexi:14: @include cyrillic.itexi
It's the simplest solution since the Cyrillic strings are in .ly
snippets.
http://codereview.appspot.com/6459081/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add support for Cyrillic in texinfo. (issue 6459081)
Notation, section `Kievan contexts', or `snippets/utf-8.ly'. http://codereview.appspot.com/6459081/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add support for Cyrillic in texinfo. (issue 6459081)
http://codereview.appspot.com/6459081/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add support for Cyrillic in texinfo. (issue 6459081)
For example, `Cyrillic letter V' currently maps to the `V', and `Cyrillic letter G' maps to `G'. [All Cyrillic characters start with prefix `' so that they get sorted after all Latin letters]. But this is certainly not the correct collation order for Russian! AFAIK, `Cyrillic letter V' gets sorted *before* `Cyrillic letter G', thus its mapping should be fixed. Etc., etc. http://codereview.appspot.com/6459081/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add support for Cyrillic in Texinfo/TeX (issue 6492045)
LGTM, thanks! http://codereview.appspot.com/6492045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (left): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#oldcode885 mf/parmesan-clefs.mf:885: Your code is not correct. I'm sending comparison images to the lilypond-devel list. http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode854 mf/parmesan-clefs.mf:854: draw_triple_brevis (exact_center + (0, 1.0 staff_space#), Any reason why you are changing the indentation which is different to all other mf files? http://codereview.appspot.com/6503091/diff/1/mf/parmesan-clefs.mf#newcode866 mf/parmesan-clefs.mf:866: 2.2 half_reduced_il# + staff_space# - 2 ypart exact_center, Please don't make lines longer than 80 lines. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
I'm sorry, but I find this incredibly unhelpful. You've done it wrong is insulting to someone who's spent about 40 hours trying to get it right. The original code was wrong, with any number of incorrect assumtions. If you could point me to what is incorrect, I'll try to fix it. I see nothing wrong with the images you've supplied. Oh! Sorry about that. I thought that the differences between the old and new shape are obvious. If you do a blinking comparison it is easy to see that the lower left and right vertical extensions have become shorter and longer, respectively. Since you don't say anywhere that you are going the change the glyph shape itself, I've assumed (and still assume) that this is an unintentional change. On the other hand, the upper left and right vertical extensions now unnecessarily protrude the clef body, which can be dangerous. From mf/README: If outlines intersect, avoid grazing intersections. In case two outlines intersect in an explicitly defined point, include this point in both intersecting paths to avoid problems due to rounding errors. The mix of tabs and spaces made indentation difficult to follow. Please explain why mixing tabs and spaces is good. It's not about mixing tabs and spaces but about consistency, which I consider extremely important. If you do grep -1 -h 'def .* ([^)]*$' parmesan-custodes.mf you can see that the indentation pattern follows this scheme def foo (expr XXX, ..., YYY, ...) in case the `def' doesn't fit into a single line. The same holds for the other MF files (with three exceptions which are formatting mistakes). Another issue is that your indentation changes actually distract from your real code changes. Your patch has become unnecessarily much longer due to the indentation stuff. If you *really* want to change the indentation, it should be done in a separate patch which should be then tagged as `formatting only' or something like that. BTW, this is true in general: Always try to separate formatting and code changes. I found the line break hindered understanding of the syntax, to no benefit. Most editors extend part 80 characters these days. This is exactly the same issue as above. BTW, I don't share your opinion, and except some well-founded exceptions in parmesan-noteheads.mf, *all* source code lines of the MF files are within the 80-char limit. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
There isn't any indentation standard for mf files, is there? It's implicitly given by the formatting of all MF files. I know that this is far from ideal, but unless someone is stepping forward to write a specification for GNU indent or something similar so that the indentation can be enforced mechanically, I see absolutely no reason to re-format those files. I mean, in this file there's a weird mixture of tabs and spaces Weird? It's always tabs (which is assumed to be represented visually by eight spaces on screen) followed by less than eight spaces. If you are talking about the indentation: I tried hard to be consistent in all files, trying to align vertically where it makes sense, and where it looks nicely. The indentation level itself is a simply a tab. I'm really bewildered that it is apparently so hard for many contributors to format and indent new code in the same manner as the surrounding code. Is this lack of experience? Is this ignorance? Is it arrogance? http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
I was aware of a slight difference, although actually I'm not certain it's that important - these are machine drawn glyphs intending to replicate hand-drawn clefs from the 15th century. How important is 0.1 staff space? This is not the point. Your are changing the shape without documenting this fact. And the problem is not 0.1 staff space but loosing the vertical symmetry for no good reasons. I use Gedit and by default for mf files it puts tabs at 4 spaces, and so the indenting looked completely awry. Good goodness! This is a plain wrong default. If in doubt, *always* use 8 spaces for a tab. This is UNIX standard since the very beginning. Everything else is evil. IMHO mixing tabs and spaces is a really bad idea for that reason - different editors indent different ways. Hmm, the rule `1 physical tab = 8 spaces' is standard on UNIX boxes since 30 years? 40 years? As for the overlaps. Ditto I'll look at that. The original code calculated end points in a very odd way - it allowed, for example, of different amounts of shift between the breves, and then ignored that in the calculation of the decorations. I'll see if I can improve this. Thanks. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add support for Cyrillic in texinfo. (issue 6459081)
Uh, oh, please ignore the new stuff. http://codereview.appspot.com/6459081/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Doc: Improve documentation of \glissando. (issue 6529043)
Reviewers: Graham Percival,
Message:
I agree with your argumentation. However, I don't have time to fix the
patch. Maybe a good soul from the documentation team can improve this.
Description:
Doc: Improve documentation of \glissando.
Based on work from Tiresia GIUNO tires...@googlemail.com.
Please review this at http://codereview.appspot.com/6529043/
Affected files:
M Documentation/notation/expressive.itely
Index: Documentation/notation/expressive.itely
diff --git a/Documentation/notation/expressive.itely
b/Documentation/notation/expressive.itely
index
25dbc81d798c75a3b3b1fef709d6fb2b7b66c2e8..528cdb2047c56e75470673e535f57d7b8a3849c4
100644
--- a/Documentation/notation/expressive.itely
+++ b/Documentation/notation/expressive.itely
@@ -1038,15 +1038,88 @@ g2\glissando g'
c2\glissando c,
@end lilypond
+It is possible to place an expression mark at a certain point within
+the glissando, usually indicated by a stem without a notehead:
+
+@lilypond[verbatim,quote,relative=2]
+f4\glissando\
+\once \override NoteColumn #'glissando-skip = ##t
+\once \override NoteHead #'transparent = ##t
+a4\f\ a8\! r4.
+@end lilypond
+
+@noindent
+Setting @code{glissando-skip} to @code{#t} makes the glissando skip the
+inserted @code{NoteColumn} grob. To hide the notehead, the
+@code{transparent} property is set to @code{#t}. If the stem doesn't
+align well with the glissando, it may need repositioning.
+
+The same works with more than one inserted grob:
+
+@lilypond[verbatim,quote,relative=2]
+r8 f2\glissando a8 r4 |
+r8 f8\glissando
+\override NoteColumn #'glissando-skip = ##t
+\override NoteHead #'transparent = ##t
+g4 a8
+\revert NoteColumn #'glissando-skip
+\revert NoteHead #'transparent
+a8 r4
+@end lilypond
+
+Setting the @code{breakable} property to @code{#t} in combination with
+@code{after-line-breaking} allows to break a glissando if it occurs
+at a line break:
+
+@lilypond[verbatim,quote,relative=2,line-width=4.0\cm]
+\override Glissando #'breakable = ##t
+\override Glissando #'after-line-breaking = ##t
+
+f1\glissando | \break
+a4 r2. |
+f1\glissando \break
+\once \override NoteColumn #'glissando-skip = ##t
+\once \override NoteHead #'transparent = ##t
+a2 a4 r4 |
+@end lilypond
+
+A glissando can connect notes across staves:
+
+@lilypond[verbatim,quote]
+\new PianoStaff
+ \new Staff = right {
+e'''2\glissando
+\change Staff = left
+a,,\glissando
+\change Staff = right
+b''8
+ }
+ \new Staff = left {
+\clef bass s1 s8
+ }
+
+@end lilypond
+
+A glissando can occur between chords:
+
+@lilypond[verbatim,quote,relative=2]
+c1\glissando g'
+c, e1\glissando g' b \break
+
+\set glissandoMap = #'((0 . 1) (1 . 0))
+c, g'1\glissando d a'
+\set glissandoMap = #'((0 . 0) (0 . 1) (0 . 2))
+c1\glissando d f a
+\set glissandoMap = #'((2 . 0) (1 . 0) (0 . 1))
+d f a1\glissando c c'
+@end lilypond
+
Different styles of glissandi can be created. For details, see
@ref{Line styles}.
@snippets
@lilypondfile[verbatim,quote,texidoc,doctitle]
-{glissandi-can-skip-grobs.ly}
-
-@lilypondfile[verbatim,quote,texidoc,doctitle]
{contemporary-glissando.ly}
@seealso
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
parser.yy: remove STRING_IDENTIFIER token (issue 6542057)
LGTM http://codereview.appspot.com/6542057/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
Well done! It's not there yet due to some bugs I believe, but besides this the code looks OK. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode764 mf/parmesan-clefs.mf:764: save reduced_il, vert_thick, hor_thick, blot_rad; Until the great tab removal operation I ask you to use tabs (with 1tab=8columns) to stay in sync with the remaining code. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode787 mf/parmesan-clefs.mf:787: pat = z1l -- z2l..z2+(0, blot_rad)..z2r The remaining MF files format it like this: pat = z1l -- z2l .. z2 + (0, blot_rad) .. z2r etc. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode794 mf/parmesan-clefs.mf:794: unfill pat_mid; The unfill operation leaves no border at the right. Is it really intentional that there is no vertical line at the right of the glyph? At least the clefs in the facsimile scans look differently. http://codereview.appspot.com/6503091/diff/10001/mf/parmesan-clefs.mf#newcode803 mf/parmesan-clefs.mf:803: penlabels (1,2,3,4,5,6); If point X is defined with `penposX', you should use `penlabels(..., X, ...)', otherwise use `labels', but not both at the same time. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
The glyph shape is OK, thanks. Have you tested the appearance on paper
of all available sizes, using a 300 or 600dpi printer?
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf
File mf/parmesan-clefs.mf (left):
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#oldcode902
mf/parmesan-clefs.mf:902:
The dimension of the bounding box is not correct. It's far too large,
as can be seen in the DVI proof.
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf
File mf/parmesan-clefs.mf (right):
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#newcode791
mf/parmesan-clefs.mf:791: ..z2+(0, blot_rad)
This must be `.. {up}z2 + (0, blot_rad)', otherwise the curve doesn't
start with the right direction. This wouldn't be that bad, but it were
different to all other glyph shapes.
Ditto for all other transitions from `--' to `..' (this is, using the
proper `{up}', `{right}', etc.).
http://codereview.appspot.com/6503091/diff/16001/mf/parmesan-clefs.mf#newcode793
mf/parmesan-clefs.mf:793: --quartercircle scaled blot_diameter rotated
180 shifted (z3r + (blot_rad, blot_rad))
Using `quartercircle' gives good results in the type1 output. I only
ask you to make the line fit into 80 columns :-)
On the other hand, why not simply using something like
startcurve_point{down} .. {right}endcurve_point
? This might be shorter, and easier to read.
http://codereview.appspot.com/6503091/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
When you say paper of all available sizes I don't understand
why changing the paper size should have any effect.
Could you explain?
Sorry, bad wording. I mean the appearance of all available sizes
printed out on paper.
Is there guidance on the the correct way to calculate
the parameters to set_char_box()?
Normally, it should be the tightest bounding box. Just look at other
glyphs.
The benefit of quartercurve is that it
takes the place of 2 points, so is actually shorter.
? What do you mean with `takes the place of 2 points'? What I suggest
is to insert
pickup pencircle scaled 0.5 blot_diameter;
at the beginning of the glyph so that `top', `lft', `rt', and `bot' of a
point are defined, then using e.g.
...
z2r
-- top z3r{down}
.. {left}rt z3r
-- etc.
This is much shorter than your `quartercircle' stuff :-)
http://codereview.appspot.com/6503091/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
LGTM, except one small issue. http://codereview.appspot.com/6503091/diff/20001/mf/parmesan-clefs.mf File mf/parmesan-clefs.mf (right): http://codereview.appspot.com/6503091/diff/20001/mf/parmesan-clefs.mf#newcode816 mf/parmesan-clefs.mf:816: 2.2 reduced_il#); The bbox is still too small. I suggest that you define z2 like this: x2l = 0; top y2 = 2.2 reduced_il; This makes the left vertical element a bit shorter which shouldn't be noticeable, however. http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes position of mensural c clef (issue 6503091)
If you do mf '\mode:=proof; input parmesan20' gftodvi parmesan20.2602gf and view the resulting parmesan20.dvi with xdvi, go to your glyph, then press `10 s' to set the shrink factor to 10. This makes the image small enough that you can see even the part of the glyph which is below the lower edge of the paper (indicated by the red box). http://codereview.appspot.com/6503091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds tick mark to scripts (issue 6568055)
LGTM. http://codereview.appspot.com/6568055/diff/1/mf/feta-scripts.mf File mf/feta-scripts.mf (right): http://codereview.appspot.com/6568055/diff/1/mf/feta-scripts.mf#newcode1774 mf/feta-scripts.mf:1774: set_char_box (0, 1.7 staff_space# + epsilon, I suggest to use a tightest bounding box. At least I don't see any reason to not do so. The question is whether it makes sense to give this glyph a depth (this is, to make the glyph go below the baseline), but this is only a matter of taste. http://codereview.appspot.com/6568055/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds tick mark to scripts (issue 6568055)
Please don't use `epsilon' in set_char_box. I think the problem is that you `sharpen' a coordinate distance by doing `define_pixels (y_off)', however, only `black distances' (to use the TrueType vocabulary) like vertical or horizontal stem widths should be handled like that. In general, I would move the tick down, as Janek is suggesting. The exact position in a score should be then handled by default offsets given in Scheme code. http://codereview.appspot.com/6568055/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds tick mark to scripts (issue 6568055)
LGTM. http://codereview.appspot.com/6568055/diff/7001/mf/feta-scripts.mf File mf/feta-scripts.mf (right): http://codereview.appspot.com/6568055/diff/7001/mf/feta-scripts.mf#newcode1781 mf/feta-scripts.mf:1781: penlabels (1,2,3,4); z4 is not defined with penpos4, so you should use `labels' instead of `penlabels'. http://codereview.appspot.com/6568055/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Regularize lyrics lexer mode (issue 6594047)
LGTM. http://codereview.appspot.com/6594047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: lilypond-book: treat iffalse sections in latex as block comments (issue 6584073)
http://codereview.appspot.com/6584073/diff/1/python/book_latex.py
File python/book_latex.py (right):
http://codereview.appspot.com/6584073/diff/1/python/book_latex.py#newcode88
python/book_latex.py:88: \\iffalse.*\\fi))''',
On 2012/10/06 02:12:41, Julien Rioux wrote:
.* should be replaced by the non-greedy .*?
Yes :-)
http://codereview.appspot.com/6584073/diff/1/python/book_latex.py#newcode88
python/book_latex.py:88: \\iffalse.*\\fi))''',
As David has commented on the list, I would prefer if you replaced the
\iffalse ... \fi with a higher-level command, namely \begin{comment} ...
\end{comment} (from the `comment' package). First of all, this syntax
suits LaTeX better since it resembles normal environments. Second, it
is a bad idea in general to expose \ifXXX to the top level; in other
words, users should never do that. Reason is that \ifXXX is handled
very special in TeX since you can define, say, \ifFoo which also pairs
with the next \fi, making correct balancing of nested \ifXXX commands
quite delicate and very hard to debug.
http://codereview.appspot.com/6584073/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: convert-ly (issue 2670) (issue 6610058)
http://codereview.appspot.com/6610058/diff/2001/scripts/convert-ly.py File scripts/convert-ly.py (right): http://codereview.appspot.com/6610058/diff/2001/scripts/convert-ly.py#newcode357 scripts/convert-ly.py:357: sys.exit(errors) Are you going to report the number of errors using the `errors' variable? In case this is true, I would consider this a bad idea, since you abuse the functionality of the exit status. How large can `errors' become? The value returned by `exit' must be in the range 0-255 (with 0 meaning `no error'). http://codereview.appspot.com/6610058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: convert-ly (issue 2670) (issue 6610058)
LGTM now. http://codereview.appspot.com/6610058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Make arguments like Context.GrobName accessible as symbol lists (issue 6635050)
LGTM, without testing, and without really understanding the change. However, simplifications and generalizations are always a good thing. http://codereview.appspot.com/6635050/diff/1/Documentation/de/notation/pitches.itely File Documentation/de/notation/pitches.itely (right): http://codereview.appspot.com/6635050/diff/1/Documentation/de/notation/pitches.itely#newcode1529 Documentation/de/notation/pitches.itely:1529: \accidentalStyle StaffGroup.voice This looks strange. I don't expect double quotes after a dot. Any better representation possibility? http://codereview.appspot.com/6635050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Make arguments like Context.GrobName accessible as symbol lists (issue 6635050)
LGTM. http://codereview.appspot.com/6635050/diff/7002/ly/music-functions-init.ly File ly/music-functions-init.ly (right): http://codereview.appspot.com/6635050/diff/7002/ly/music-functions-init.ly#newcode105 ly/music-functions-init.ly:105: (ly:input-warning location (_ not a spanner name, `~a') name) Wouldn't `~a' is not a spanner name be more appropriate? http://codereview.appspot.com/6635050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix #887: Use ly:string-percent-encode for textedit URIs. (issue193077)
http://codereview.appspot.com/193077/diff/1001/12
File lily/general-scheme.cc (right):
http://codereview.appspot.com/193077/diff/1001/12#newcode230
lily/general-scheme.cc:230: return ((c = 0x2D c = 0x2F) // hyphen,
full-stop, and forward-slash
Wouldn't it be faster to use an array of `0' and `1', indexed by the
character code, instead of the many comparison operations? E.g.
escape_character[128] = { 0, 0, 0, ..., 1, 1, 0, ... }
http://codereview.appspot.com/193077/show
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Optimizations for pure-height approximations. (issue1817045)
http://codereview.appspot.com/1817045/diff/1/4 File scm/define-grob-properties.scm (right): http://codereview.appspot.com/1817045/diff/1/4#newcode1059 scm/define-grob-properties.scm:1059: Since the properties in this file are sorted alphabetically, this is the wrong place... http://codereview.appspot.com/1817045/show ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fix #888: Add ly:stencil-scale. (issue2275042)
LGTM too.
http://codereview.appspot.com/2275042/diff/18001/lily/stencil-scheme.cc
File lily/stencil-scheme.cc (right):
http://codereview.appspot.com/2275042/diff/18001/lily/stencil-scheme.cc#newcode398
lily/stencil-scheme.cc:398: {
Don't say @co...@var{...}} but @var{...}.
http://codereview.appspot.com/2275042/diff/18001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):
http://codereview.appspot.com/2275042/diff/18001/scm/define-markup-commands.scm#newcode3381
scm/define-markup-commands.scm:3381: Scale @co...@var{arg}}.
@co...@var{factor-pair}} is a pair of numbers
Don't say @co...@var{...}} but @var{...}
http://codereview.appspot.com/2275042/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Update docstring for ly:gulp-file (issue2754041)
http://codereview.appspot.com/2754041/diff/1/lily/general-scheme.cc
File lily/general-scheme.cc (right):
http://codereview.appspot.com/2754041/diff/1/lily/general-scheme.cc#newcode84
lily/general-scheme.cc:84: If @var{size} is @code{SCM_UNDEFINED}, the
entire file is read.
To be in sync with the rest of the scheme function documentation, please
just say `undefined' and not `...@code{scm_undefined}'. Besides that, it
looks fine.
http://codereview.appspot.com/2754041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clef support for cue notes (issue2726043)
http://codereview.appspot.com/2726043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Clef support for cue notes (issue2726043)
Oops, somehow I've just created an empty comment. http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly File ly/music-functions-init.ly (right): http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly#newcode238 ly/music-functions-init.ly:238: cleffedCueDuring = I very much dislike this name. What about \cueDuringWithClef? English speaking people are very creative in creating verbs out of nouns, but somehow `cleffed' really hurts to me... http://codereview.appspot.com/2726043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: separating flags from noteheads in font (issue4273119)
LGTM. http://codereview.appspot.com/4273119/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Several fixes for annotate-spacing. (issue4724041)
LGTM. This kind of patch is something I wouldn't discuss on Rietveld but rather push immediately... http://codereview.appspot.com/4724041/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: First pass at avoiding very high slurs (fixes issue 163). (issue4817048)
I don't mind if we have another obscure entry in the detail list currently. If your patches fixes the problem reliably, this would be a great immediate help. IMHO, at some point in the hopefully not too distant future, the whole handling of slurs and ties must be re-examined to make it more user-friendly and robust, and to improve its shapes, both on the macro and micro level. Additionally, due to the very graphical nature of the problem, it deserves a texinfo-like documentation (with many, many images), to be generated from the corresponding comments in the source code files. http://codereview.appspot.com/4817048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Lilypond-book: Implement musicxml support in lilypond-book (issue1659041)
LGTM (without testing).
http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely
File Documentation/usage/lilypond-book.itely (right):
http://codereview.appspot.com/1659041/diff/5001/Documentation/usage/lilypond-book.itely#newcode207
Documentation/usage/lilypond-book.itely:207: @item the
@code{\lilypond@{...@}} command, where you can directly enter short
lilypond code
Please stay within a width of 80 characters.
http://codereview.appspot.com/1659041/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: font: change breve vertical lines (issue4748044)
http://codereview.appspot.com/4748044/diff/1/mf/feta-noteheads.mf File mf/feta-noteheads.mf (right): http://codereview.appspot.com/4748044/diff/1/mf/feta-noteheads.mf#newcode181 mf/feta-noteheads.mf:181: % when there is an interval of fourth. Wouldn't it be simpler to explicitly specify the line length instead of using such a complicated algorithm? I mean a global variable to be added to the various feta-noteheadsXX.mf files. I know that none of the existing size driver files contain such a variable except the standard assignment of `design_size', but this shouldn't stop you to add something since it would be the logical place to add. Cf. similar control files in other MF fonts like TeX's Computer Modern, which are full of such meta-variables. Han-Wen? http://codereview.appspot.com/4748044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: font: change breve vertical lines (issue4748044)
Aww, i was so proud of this code... :-) Frankly, i don't think we will gain anything from defining a global value. The algorithm i wrote is a bit complicated, but i thinks is easier to understand than what you suggest (if i understood your suggestion correctly). It keeps things in one place instead of values flying all around (at least that's how i see it). Just to make it clear: Let's call my hypothetical variable `line-height'. Then we would have in file `feta-noteheads11.mf' the line line-height := 10 in file `feta-noteheads13.mf' the line line-height := 15 etc. (the values are just for demonstration purposes). Not really complicated or confusing IMHO. Just imagine that your algorithm yields the value 13 for size 13, but you want to use 15 since this looks better. How would you deal with that? Add another exception to your algorithm, or redesign it completely? My concern is that the algorithm decides how the value should look like, instead of doing the opposite... http://codereview.appspot.com/4748044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Adds a glyph for tied lyrics. (issue4808074)
Please use tabs in MF files. Besides that, everythings looks fine. http://codereview.appspot.com/4808074/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 905: Gracefully ignore UTF-8 BOM in the middle of a file (issue 4908043)
Could you please tell me what this patch is good for? A BOM not at the beginning of a file is no longer a BOM... I don't oppose to emitting a warning if U+FEFF is encountered, and we subsequently ignore it (since its use as zero width no-break space is deprecated), but only within strings... What am I missing? http://codereview.appspot.com/4908043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 905: Gracefully ignore UTF-8 BOM in the middle of a file (issue 4908043)
OK. Please make the warning message more verbose, though. http://codereview.appspot.com/4908043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add hihat halfopen glyph to font (issue 4714043)
LGTM. http://codereview.appspot.com/4714043/diff/10001/mf/feta-scripts.mf File mf/feta-scripts.mf (right): http://codereview.appspot.com/4714043/diff/10001/mf/feta-scripts.mf#newcode633 mf/feta-scripts.mf:633: height# / 2, height# / 2); Vertical align mismatch. http://codereview.appspot.com/4714043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Lilypond-book: Get rid of lilyquote option, use quote instead (issue 4921050)
LGTM. http://codereview.appspot.com/4921050/diff/2001/python/book_texinfo.py File python/book_texinfo.py (right): http://codereview.appspot.com/4921050/diff/2001/python/book_texinfo.py#newcode250 python/book_texinfo.py:250: if (QUOTE in snippet.option_dict): Why parentheses? Similar lines in the scripts don't use them. http://codereview.appspot.com/4921050/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Fix 1816: Lilypond-book: Give images 1mm less linewidth (issue 4940043)
LGTM. Perhaps you should mention that reducing the line width by 1mm is a temporary hack. http://codereview.appspot.com/4940043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Cleaned up style (issue 4951062)
http://codereview.appspot.com/4951062/diff/1/mf/feta-kievan.mf
File mf/feta-kievan.mf (right):
http://codereview.appspot.com/4951062/diff/1/mf/feta-kievan.mf#newcode78
mf/feta-kievan.mf:78: fill z1{dir -6.9} .. z2 .. z3 z3 .. z4 .. z5
z5 -- z6 z6 .. z7 .. z8 z8{left} .. z9 z9 .. z10 ... {dir
-76.9}cycle;
fill z1{dir -6.9}
.. z2
.. z3
z3
.. z4
.. z5
z5
-- z6
z6
.. z7
.. z8 z8{left}
.. z9 z9
.. z10
... {dir -76.9}cycle;
Well, I generally prefer vertical alignment since it makes the code much
easier to read. However, it shouldn't be a religious thing. For
example, given that the fill command is quite long and clearly separated
into subpaths, the following looks good also:
fill z1{dir -6.9} .. z2 .. z3
z3 .. z4 .. z5
z5 -- z6
z6 .. z7 .. z8
z8{left} .. z9
z9 .. z10... {dir -76.9}cycle;
http://codereview.appspot.com/4951062/diff/6001/mf/feta-kievan.mf
File mf/feta-kievan.mf (right):
http://codereview.appspot.com/4951062/diff/6001/mf/feta-kievan.mf#newcode32
mf/feta-kievan.mf:32: x1 = 0.09 * staff_space;
I'm not really happy with the MF code: It looks like a direct
translation of PostScript points into MF points. This definitely works
but is quite clumsy. If you look at the design of other LilyPond
glyphs, you can see that all coordinates rely on meta-parameters which
control the appearance. For your glyphs, we have to directly shift
coordinates in case of a change, with a great chance to inadvertently
break important relationships within the glyph shape.
I would really like to see a `LilyPond approach' for Kievan notes also,
treating the current approach as a temporary solution.
http://codereview.appspot.com/4951062/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Cleaned up style (issue 4951062)
Simply have a look how other note heads are implemented, and watch how the shape changes for different design sizes. http://codereview.appspot.com/4951062/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Cleaned up style (issue 4951062)
http://codereview.appspot.com/4951062/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Glyphs for Kievan Notation (issue 4951062)
OK. What is noteheight# equal to and how is that determined? A quick grep on the MF files shows that noteheight# is defined in feta-params.mf. http://codereview.appspot.com/4951062/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Improves some parmesan noteheads. (issue 4639065)
LGTM Werner http://codereview.appspot.com/4639065/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Glyphs for Kievan Notation (issue 4951062)
LGTM
http://codereview.appspot.com/4951062/diff/24001/mf/feta-kievan.mf
File mf/feta-kievan.mf (right):
http://codereview.appspot.com/4951062/diff/24001/mf/feta-kievan.mf#newcode52
mf/feta-kievan.mf:52: fill z1{dir 8.6} .. z2 .. z3
A minor thing: Please align this vertically (and at similar places
also); this is, the indentation here consists of a tab and some space
characters:
fill z1 .. z2 ..z3
z4 .. z5 .. z6
...
Maybe you call this weird, but it helps reading the code, at least for
me :-)
http://codereview.appspot.com/4951062/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Remove spurious spaces from music expression display, adapt tests. (issue 5437140)
David, I suggest to apply patches of that kind directly to the repository without setting up a Rietveld issue. http://codereview.appspot.com/5437140/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
lexer.ll: Warn about non-UTF-8 characters (issue 5505090)
LGTM. http://codereview.appspot.com/5505090/diff/1/lily/lexer.ll File lily/lexer.ll (right): http://codereview.appspot.com/5505090/diff/1/lily/lexer.ll#newcode1009 lily/lexer.ll:1009: case 0xc2: Wouldn't it be more effective to create an array of 128 bytes (for 0x80-0xFF) which maps `p[i]' to the value of `more' (or 0 if there is more to do)? http://codereview.appspot.com/5505090/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Prevents Beam vs Flag collisions (issue 5527047)
LGTM. Will test soon. http://codereview.appspot.com/5527047/diff/1/lily/lily-guile.cc File lily/lily-guile.cc (left): http://codereview.appspot.com/5527047/diff/1/lily/lily-guile.cc#oldcode573 lily/lily-guile.cc:573: int i = scm_to_int (k); Please apply this cosmetic patch directly to the repository; I think it doesn't belong to the rest of the patch set. http://codereview.appspot.com/5527047/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Implement session-terminate in lily.scm (issue 6742043)
LGTM. http://codereview.appspot.com/6742043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Revert Archive baselines in LILYPOND_BASELINES directory if specified (issue 6709073)
LGTM. http://codereview.appspot.com/6709073/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 2917: Extend \keepWithTag to allow multiple tags (issue 6744070)
LGTM. Any convert rules necessary? http://codereview.appspot.com/6744070/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Allow quoted identifiers like to be used like \violin1, not just defined. (issue 6778055)
LGTM. http://codereview.appspot.com/6778055/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allow digits in identifiers (issue 6493072)
LGTM. Nice idea. I'm not sure whether this fits into the large picture w.r.t. syntax normalization as envisioned by David, but at least for me it looks reasonable. http://codereview.appspot.com/6493072/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Document symbol list changes. (issue 6813079)
LGTM. http://codereview.appspot.com/6813079/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Allow (closed) scheme function calls as text scripts. (issue 6812088)
Very nice! http://codereview.appspot.com/6812088/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: markup-commands rest-by-number and rest (issue 6850073)
http://codereview.appspot.com/6850073/diff/1/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):
http://codereview.appspot.com/6850073/diff/1/scm/define-markup-commands.scm#newcode3341
scm/define-markup-commands.scm:3341: breve, longa and maxima are valid
input-strings
This should be rather
@code{breve}, @code{longa} and @code{maxima} ...
Ditto for the next lines, which are missing proper texinfo markup. And
please make sure that at least the documentation stays within the 80
character line length limit.
http://codereview.appspot.com/6850073/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add dead-is-alive boolean property to Hara_kiri_group_spanner (issue 6948058)
LGTM. Do we have a regtest (or rather, an example) which demonstrates how to use it? https://codereview.appspot.com/6948058/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 3049: Parser outputs Lyric events for illegal note names (issue 7017044)
LGTM. https://codereview.appspot.com/7017044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Simplify several library functions. (issue 7020044)
Everything looks very nice! I just wonder how it comes that those simplifications haven't been done from the very beginning: Is it possible that functions like `any' or `every' are new in Scheme? https://codereview.appspot.com/7020044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Dual license the files under mf/ using OFL. (issue 6970046)
Sorry for being late. LGTM, with a (not so) minor nit. https://codereview.appspot.com/6970046/diff/1/LICENSE.OFL File LICENSE.OFL (right): https://codereview.appspot.com/6970046/diff/1/LICENSE.OFL#newcode2 LICENSE.OFL:2: Copyright (c) 1996, 1997, ..., 2012, The LilyPond authors (lilypond.org) I strongly suggest to use Copyright (c) 1996-2012 The LilyPond authors (lilypond.org) which is the canonical form w.r.t. a range of copyrighted years. https://codereview.appspot.com/6970046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 3073: Context definitions and context mods ignore \unset (issue 7038044)
LGTM https://codereview.appspot.com/7038044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Let ChordNameVoice use the same performers as Voice (issue 7054043)
LGTM. Thanks for the quick fix. https://codereview.appspot.com/7054043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Add warning message for unknown characters (2889) / Fix assertion (2983) (issue 6625078)
Uh, oh, I failed to use git-cl correctly, so this Rietvield issue is used for two tracker items... Sorry. https://codereview.appspot.com/6625078/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Dual license the files under mf/ using OFL. (issue 6970046)
LGTM https://codereview.appspot.com/6970046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Create a \tuplet function to complement \times and tupletSpannerDuration (issue 7058068)
LGTM. https://codereview.appspot.com/7058068/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Better staggering of accidental placements. (issue 7101045)
https://codereview.appspot.com/7101045/diff/5001/scm/define-context-properties.scm File scm/define-context-properties.scm (right): https://codereview.appspot.com/7101045/diff/5001/scm/define-context-properties.scm#newcode310 scm/define-context-properties.scm:310: (horizontallyStaggerDifferentVoiceOctaveAccidentals ,boolean? This name is a joke, isn't it? It has a length of bloody 50 characters! By all means, please replace it with some *much* shorter, say, \set Staff . accidentalGrouping = #'voice and \set Staff . accidentalGrouping = #'staff https://codereview.appspot.com/7101045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Updates ancient clefs (issue 7180043)
LGTM. https://codereview.appspot.com/7180043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Remove complete aborts in the lexer (issue 7202048)
LGTM. https://codereview.appspot.com/7202048/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
let beam thickness depend on line thickness (fix 3173) (issue 7312091)
LGTM, inspite of the compilation error which I can't explain :-) https://codereview.appspot.com/7312091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Avoid excessive number of dots in chords (#3179). (issue 7319049)
Thanks for the review.
https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc
File lily/dot-column.cc (right):
https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode63
lily/dot-column.cc:63: vectorInterval allowed_y_positions;
On 2013/02/17 08:20:39, Keith wrote:
It would be simpler to have just one interval, describing all the dots
handled
by one dot_engraver, even if coming from two voices.
I've started with a single interval but this doesn't work well. Just
imagine
{ e f g a b2. } \\
{ c, d e f g2. }
There is a gap between the two clusters, and it looks really strange if
this is filled with dots.
https://codereview.appspot.com/7319049/diff/2001/lily/dot-column.cc#newcode80
lily/dot-column.cc:80: Interval hp = Stem::head_positions (stem);
On 2013/02/17 08:20:39, Keith wrote:
It looks like head_positions are shifted in the test 'grid-lines.ly'
but dot
positions are not, at this stage in the code.
This is addressed in the newer patchset.
https://codereview.appspot.com/7319049/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Avoid excessive number of dots in chords (#3179). (issue 7319049)
... next patch set will follow soon. https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc File lily/dot-column.cc (right): https://codereview.appspot.com/7319049/diff/7001/lily/dot-column.cc#newcode206 lily/dot-column.cc:206: p += (int) robust_scm2double (dp.dot_-get_property (staff-position), 0.0); On 2013/02/18 08:06:45, Keith wrote: That is why I suggested building allowed_positions using p: allowed_positions.add_point(p) You can extend this to have a separate allowed_positions for each chord, finding the Stem of the Dots as you did above. But the `desired position' is not what I need! Following Gould, I really need the extrema of the chord's note head positions, slightly extended at the top and the bottom in case those notes are sitting on a line. https://codereview.appspot.com/7319049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Avoid excessive number of dots in chords (#3179). (issue 7319049)
We do lose the double-dot on f'4.. \\ f'2. Yes. As shown in the scanned Beethoven example in #3179, what lilypond currently does with `chord-dots' off is not correct either... Well, Gould is showing the usual case, where notes have Dots.staff-position = 0. You were asking for opinions on the optimal way to behave for users who override Dots.staff-position. You could generalize by using the requested Dots position 'p' (which is exactly the NoteHead position in the simple case) in place of the note-head position. Thanks for the suggestion. In case there is ever the need to improve my solution, you give a recipe to do that :-) https://codereview.appspot.com/7319049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Issue 2985: lilypond: Umlauts and other special characters incorrectly exported to PDF meta data (issue 7393055)
LGTM. https://codereview.appspot.com/7393055/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Make test-output-distance regtest contain span bars (issue 7400057)
LGTM. https://codereview.appspot.com/7400057/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows slurs to break at barlines. (issue 7424049)
Very nice, and thanks a lot! From visual inspection only, LGTM. Just curious: You apparently like the word `junk', however, I find it not optimal. Can you replace this with `discard' or `cut' within the command names? https://codereview.appspot.com/7424049/diff/3002/scm/define-music-types.scm File scm/define-music-types.scm (right): https://codereview.appspot.com/7424049/diff/3002/scm/define-music-types.scm#newcode122 scm/define-music-types.scm:122: . ((description . End a slur here.) - End a phrasing slur here. https://codereview.appspot.com/7424049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows slurs to break at barlines. (issue 7424049)
Much better, thanks! https://codereview.appspot.com/7424049/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Allows minimum-length to work for end-of-line spanners. (issue 7453046)
LGTM. Thanks for the good comment :-) https://codereview.appspot.com/7453046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Adds Ferneyhough hairpins to LilyPond. (issue 7615043)
From visual inspection, LGTM.
https://codereview.appspot.com/7615043/diff/1/scm/output-lib.scm
File scm/output-lib.scm (right):
https://codereview.appspot.com/7615043/diff/1/scm/output-lib.scm#newcode929
scm/output-lib.scm:929: form. @code{x} is the portion of the width
consumed for a given line
Please use two spaces after a full stop.
https://codereview.appspot.com/7615043/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Remove -d old-relative compatibility option (issue 7764046)
LGTM https://codereview.appspot.com/7764046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Prohibit non-postevents from music functions to be used as post-event (issue 7866043)
LGTM. https://codereview.appspot.com/7866043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Remove obscure GNOME-backend related commands from Emacs LilyPond-mode (issue 7583044)
LGTM. https://codereview.appspot.com/7583044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Make a PostEvents container class for packaging several postevents (issue 7742044)
LGTM. The combination `\' `\n' `\(' is probably worth a comment in the
contributors' manual.
https://codereview.appspot.com/7742044/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Script for running pixel-based regtest comparisons (issue 7578046)
LGTM. My only (very minor) concern are overlong lines in the script, something which I consider hard to read. If possible, stay within the 80 character per line limit. https://codereview.appspot.com/7578046/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Allows slurs to break at barlines. (issue 7424049)
https://codereview.appspot.com/7424049/diff/41001/input/regression/repeat-slur.ly
File input/regression/repeat-slur.ly (right):
https://codereview.appspot.com/7424049/diff/41001/input/regression/repeat-slur.ly#newcode10
input/regression/repeat-slur.ly:10:
This should be rather @code{\\breakSlur}, @code{\\startBrokenSlur} and
@code{\\stopBrokenSlur}.
And what exactly does \breakSlur? It's missing in the regtest.
Finally, you should mention phrasing slurs also.
https://codereview.appspot.com/7424049/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
