Re: Clean up and fix glyph contour generation nits. (issue 566080043 by hanw...@gmail.com)
*Much* better to read and understand, thanks! LGTM now. https://codereview.appspot.com/566080043/
Re: Clean up and fix glyph contour generation nits. (issue 566080043 by hanw...@gmail.com)
> > I don't fully understand this code. > > that is an ominous comment to come from the Freetype > author. I had assumed you reviewed the existing code > as well. I have two difficulties: I'm not good at C++ and Guile stuff, and it's not completely clear to me what the code wants to achieve. It's not your recent changes that I'm struggling with but the overall design. What's definitely missing is handling the case where the first point of an outline is a (second-order) off-curve point, making it necessary to construct an on-curve point if the point before (i.e., the last point of the current contour) is a (second order) off-curve point, too. On the other hand, we talked about approximating the outlines by using the hull polygon that is spanned by the control points: If you do this the construction of the on-curve point wouldn't be necessary, right? Does your code change in commit 79ec62e6 do that already? The comment only says 'Substitute a line segment instead' but doesn't explain whether this is a legitimate approximation. > > A quite well documented routine to walk over a contour is function > > `FT_Outline_Decompose` in file `src/base/ftoutln.c` of the FreeType source > code. > > I'll have a look. Thanks. Note that the first point of a contour can never be a third-order off-curve point, which means that you don't need to handle wrapping in this case. https://codereview.appspot.com/566080043/
Remove deprecated context properties (issue 560030044 by v.villen...@gmail.com)
LGTM. This issue also adds some scheme code and a conversion rule – they are different commits, right? https://codereview.appspot.com/560030044/diff/557800043/scm/define-context-properties.scm File scm/define-context-properties.scm (right): https://codereview.appspot.com/560030044/diff/557800043/scm/define-context-properties.scm#newcode609 scm/define-context-properties.scm:609: merge rests when this is set to true.") While you are at it, please fix indentation (i.e., no indentation for the continuation line). https://codereview.appspot.com/560030044/
Re: Clean up and fix glyph contour generation nits. (issue 566080043 by hanw...@gmail.com)
I don't fully understand this code. Please add more comments. A quite well documented routine to walk over a contour is function `FT_Outline_Decompose` in file `src/base/ftoutln.c` of the FreeType source code. https://codereview.appspot.com/566080043/diff/560030043/lily/freetype.cc File lily/freetype.cc (right): https://codereview.appspot.com/566080043/diff/560030043/lily/freetype.cc#newcode124 lily/freetype.cc:124: else if (ctags[j] & 1) s/1/FT_CURVE_TAG_ON/ https://codereview.appspot.com/566080043/diff/560030043/lily/freetype.cc#newcode132 lily/freetype.cc:132: else if (ctags[j] & 2) s/2/FT_CURVE_TAG_CUBIC/ https://codereview.appspot.com/566080043/
Prevent race condition in `-dfont-ps-resdir` (issue 561810045 by truer...@gmail.com)
LGTM, with minor nits. https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm File scm/backend-library.scm (right): https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm#newcode143 scm/backend-library.scm:143: ;; When the directory already exists, it raises system-error. s/When/If/ https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm#newcode161 scm/backend-library.scm:161: ;; When the file already exists, it raises system-error. s/When/If/ https://codereview.appspot.com/561810045/diff/551840043/scm/backend-library.scm#newcode188 scm/backend-library.scm:188: ;; When the file already exists, it raises system-error. s/When/If/ https://codereview.appspot.com/561810045/diff/551840043/scm/framework-ps.scm File scm/framework-ps.scm (right): https://codereview.appspot.com/561810045/diff/551840043/scm/framework-ps.scm#newcode284 scm/framework-ps.scm:284: from file `~a' index ~a...") Maybe for subfont ~a of file ~a") ? https://codereview.appspot.com/561810045/diff/551840043/scm/framework-ps.scm#newcode544 scm/framework-ps.scm:544: (_ "Font file `~a' already exists, skipping.") s/\./.../ to be in sync with other, similar error messages. Alternatively, you might do s/\.\.\././ in the other error messages :-) https://codereview.appspot.com/561810045/
Re: Split glyph contours in up/down segments for skylines (issue 569700043 by hanw...@gmail.com)
[forgot to send it previously] https://codereview.appspot.com/569700043/diff/582060043/lily/freetype.cc File lily/freetype.cc (right): https://codereview.appspot.com/569700043/diff/582060043/lily/freetype.cc#newcode126 lily/freetype.cc:126: else if (outline->tags[j] & 1) On 2020/05/08 08:15:58, hahnjo wrote: > is there a define for this magic number? Yep: FT_CURVE_TAG_* (in file `ftimage.h`). https://codereview.appspot.com/569700043/
Re: Split glyph contours in up/down segments for skylines (issue 569700043 by hanw...@gmail.com)
https://codereview.appspot.com/569700043/diff/573820043/lily/freetype.cc
File lily/freetype.cc (right):
https://codereview.appspot.com/569700043/diff/573820043/lily/freetype.cc#newcode129
lily/freetype.cc:129: else if (ctags[j] & 1)
1 → FT_CURVE_TAG_ON
https://codereview.appspot.com/569700043/diff/573820043/lily/freetype.cc#newcode137
lily/freetype.cc:137: else if (ctags[j] & 2)
2 → FT_CURVE_TAG_CUBIC
https://codereview.appspot.com/569700043/diff/573820043/lily/freetype.cc#newcode165
lily/freetype.cc:165: else
0 → FT_CURVE_TAG_ON
Maybe make this an `else if` ... followed by `else { ... must never
happen ... }
to make it easier to follow
https://codereview.appspot.com/569700043/
Issue 4182: avoid checking the offset of cross-staff stems too early (issue 554030043 by barr...@gmail.com)
LGTM
https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly
File input/regression/cross-staff-stem-offset.ly (right):
https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly#newcode5
input/regression/cross-staff-stem-offset.ly:5: Cross-staff stems should
not have their @code{'X-offset} calculated too
We normally don't use the "'" symbol in documentation.
https://codereview.appspot.com/554030043/diff/582040043/input/regression/cross-staff-stem-offset.ly#newcode6
input/regression/cross-staff-stem-offset.ly:6: early because
@code{'direction} may not be known. The last stem in this
Please use two spaces after a full stop.
https://codereview.appspot.com/554030043/
Remove define causing a warning (issue 561790043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/561790043/
Re: Add OTC font support for `-dfont-ps-resdir` (issue 548080043 by truer...@gmail.com)
Thanks for the changes. Here the next round of comments :-)
https://codereview.appspot.com/548080043/diff/567530058/lily/open-type-font-scheme.cc
File lily/open-type-font-scheme.cc (right):
https://codereview.appspot.com/548080043/diff/567530058/lily/open-type-font-scheme.cc#newcode467
lily/open-type-font-scheme.cc:467: warning (_f ("font index %d too large
for font `%s', using index 0",
Maybe it makes sense to use a format like
font `%s': ...
here, too.
https://codereview.appspot.com/548080043/diff/567530058/lily/open-type-font-scheme.cc#newcode495
lily/open-type-font-scheme.cc:495: warning (_f ("font `%s': cannot read
`%s' field of subfont %d",
Shall we do
s/`%s' field/field `%s'/
here and in the following messages? Ditto
s/`%s' table/table `%s'/
later on. We already have `font xxx' and `subfont yyy'...
https://codereview.appspot.com/548080043/diff/567530058/lily/open-type-font-scheme.cc#newcode513
lily/open-type-font-scheme.cc:513: warning (_f ("font `%s': cannot open
for writing",
Shall we replace `font' with `subfont' here and in succeeding write
messages?
https://codereview.appspot.com/548080043/diff/567530058/lily/open-type-font-scheme.cc#newcode594
lily/open-type-font-scheme.cc:594: " table no. %u of subfont %d",
s/table no. %u/table %u/
https://codereview.appspot.com/548080043/
Add OTC font support for `-dfont-ps-resdir` (issue 548080043 by truer...@gmail.com)
LGTM, with some nits
https://codereview.appspot.com/548080043/diff/573790043/Documentation/usage/running.itely
File Documentation/usage/running.itely (right):
https://codereview.appspot.com/548080043/diff/573790043/Documentation/usage/running.itely#newcode663
Documentation/usage/running.itely:663: this method cannnot embed CID
fonts with Ghostscript 9.26 and later.
s/cannnot/cannot/
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc
File lily/open-type-font-scheme.cc (right):
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode421
lily/open-type-font-scheme.cc:421: "ttcTag", collection.c_str ()));
Why "ttcTag"? I would rather call this "TTC tag".
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode428
lily/open-type-font-scheme.cc:428: warning (_f ("font file `%s' is not
collection font",
s/not collection font/not a font collection/
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode437
lily/open-type-font-scheme.cc:437: warning (_f ("cannot read %s of
`%s'",
font `%s': cannot read `Version' field
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode446
lily/open-type-font-scheme.cc:446: warning (_f ("font file `%s' TTC
header version is unknown",
→ ... `%s': invalid TTC header version
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode455
lily/open-type-font-scheme.cc:455: warning (_f ("cannot read %s of
`%s'",
font `%s': cannot read `numFonts' field
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode478
lily/open-type-font-scheme.cc:478: warning (_f ("cannot read %s of
`%s'",
font `%s': cannot read offset of subfont %d
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode495
lily/open-type-font-scheme.cc:495: warning (_f ("cannot read %s of
`%s'",
→ font `%s': cannot read `sfntVersion' field of subfont %d
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode504
lily/open-type-font-scheme.cc:504: warning (_f ("font file `%s' index %d
does not have valid sfntVersion",
font `%s': invalid `sfntVersion' field in subfont %d
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode513
lily/open-type-font-scheme.cc:513: warning (_f ("cannot open font
filename `%s'",
→ cannot open font `%s' for writing
https://codereview.appspot.com/548080043/diff/573790043/lily/open-type-font-scheme.cc#newcode523
lily/open-type-font-scheme.cc:523: warning (_f ("cannot write %s of
`%s'",
font `%s': cannot write `sfntVersion' field
etc., etc. for remaining messages
https://codereview.appspot.com/548080043/
Remove ly:lexer-keywords command (issue 559960060 by d...@gnu.org)
LGTM https://codereview.appspot.com/559960060/
Re: Split glyph contours in up/down segments for skylines (issue 569700043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/569700043/
Calculate skylines only once. (issue 553980043 by hanw...@gmail.com)
LGTM. The figured bass stuff is a separate commit, I guess... https://codereview.appspot.com/553980043/
Re: Use GhostScript API instead of forking (issue 548030043 by jonas.hahnf...@gmail.com)
LGTM, thanks! Looks very good, indeed. https://codereview.appspot.com/548030043/
Doc: mention tuplet-slur in Changes and in NR 1.2.1.2 "Tuplets" (issue 559930043 by v.villen...@gmail.com)
LGTM, thanks!
https://codereview.appspot.com/559930043/diff/549940043/Documentation/notation/rhythms.itely
File Documentation/notation/rhythms.itely (right):
https://codereview.appspot.com/559930043/diff/549940043/Documentation/notation/rhythms.itely#newcode275
Documentation/notation/rhythms.itely:275: @cindex tuplet slur
Add:
@cindex slur, for tuplets
https://codereview.appspot.com/559930043/diff/549940043/Documentation/notation/rhythms.itely#newcode299
Documentation/notation/rhythms.itely:299: @cindex tuplet, visibility
Maybe modify this to
@cindex tuplet bracket, visibility
since we already have this index entry.
Add:
@cindex visibility of tuplet brackets
https://codereview.appspot.com/559930043/diff/549940043/Documentation/snippets/controlling-tuplet-bracket-visibility.ly
File Documentation/snippets/controlling-tuplet-bracket-visibility.ly
(right):
https://codereview.appspot.com/559930043/diff/549940043/Documentation/snippets/controlling-tuplet-bracket-visibility.ly#newcode17
Documentation/snippets/controlling-tuplet-bracket-visibility.ly:17:
bracket), @code{#'if-no-beam} (only print a bracket if there is no beam,
s/#'if-no-beam/'if-no-beam/
(for synchronization with the rest of the documentation)
https://codereview.appspot.com/559930043/
Issue 3778: Use bounding box as skylines for markup in svg backend (issue 582010043 by barr...@gmail.com)
LGTM https://codereview.appspot.com/582010043/
Add `-dfont-ps-resdir` option to embed fonts to PDFs later (issue 577900043 by truer...@gmail.com)
Very nice, thanks! There are some grammar glitches in the documentation – I hope that a native English speaker can improve that. https://codereview.appspot.com/577900043/diff/577910043/Documentation/usage/running.itely File Documentation/usage/running.itely (right): https://codereview.appspot.com/577900043/diff/577910043/Documentation/usage/running.itely#newcode652 Documentation/usage/running.itely:652: This is useful when you want to create a font-non-embedded PDFs first Replace this and the next line with This is useful if you want to create PDFs without embedded fonts first, letting Ghostscript embed the fonts in a follow-up step as shown below. https://codereview.appspot.com/577900043/diff/577910043/scm/framework-ps.scm File scm/framework-ps.scm (right): https://codereview.appspot.com/577900043/diff/577910043/scm/framework-ps.scm#newcode291 scm/framework-ps.scm:291: (ly:warning (_ "Font ~a cannot be used in PostScript resource directory because it is an OpenType/CFF Collection (OTC) font.") name)) Please fold lines longer than 80 characters. https://codereview.appspot.com/577900043/
Re: Use Scheme_hash_table for keyword handling (issue 577840053 by d...@gnu.org)
LGTM https://codereview.appspot.com/577840053/
Transform: add print_smob to aid debugging (issue 561680043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/561680043/
Fix warnings related to po (issue 551830043 by jonas.hahnf...@gmail.com)
LGTM, thanks a lot! https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile File po/GNUmakefile (right): https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode14 po/GNUmakefile:14: sed-makestuff = -e 's/[a-zA-Z_/]*make\[[0-9]*\].*//' shouldn't this be rather '[0-9]\+'? https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode15 po/GNUmakefile:15: sed-edstuff = -e 's/[ \.,adic0-9]*//' -e 's/---//' | sort -u I think that within '[...]' you don't have to escape '.'. And again I would use '\+' instead of '*'. https://codereview.appspot.com/551830043/
Re: Make dblatex an optional dependency (issue 567480043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/567480043/
stepmake: Remove defunct test template (issue 565960043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/565960043/
Don't use C++ operator synonym "not" (issue 551820043 by d...@gnu.org)
LGTM https://codereview.appspot.com/551820043/
Use binary search in Axis_group_interface::combine_pure_heights. (issue 573730043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/573730043/
Re: Documentation: Add dependency notation.texi -> internals.texi (issue 557720043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/557720043/diff/581940043/Documentation/GNUmakefile File Documentation/GNUmakefile (right): https://codereview.appspot.com/557720043/diff/581940043/Documentation/GNUmakefile#newcode304 Documentation/GNUmakefile:304: $(outdir)/contributor.texi: $(outdir)/ly-grammar.txt Please add an empty line here. https://codereview.appspot.com/557720043/
Remove unnecessary includes of dispatcher.hh (issue 555700043 by d...@gnu.org)
LGTM https://codereview.appspot.com/555700043/
Use a hash table for the lexer keywords (issue 549920043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/549920043/
Obviate method_finder methods (issue 551780043 by d...@gnu.org)
Nice! I don't really understand the C++ wizardry, but having to type less macros in the normal code is certainly beneficial. https://codereview.appspot.com/551780043/
stepmake: Remove unused tex template (issue 563870043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/563870043/
Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)
> I agree with the sentiment, but I fail to come up > with a naming choice that does not make the cure > worse than the problem. I would simply use GET_LISTENERX (or GET_LISTENER1) for the longer call. https://codereview.appspot.com/549890043/
Issue 5917: ly:performance-headers and \midi { after-writing } (issue 567450043 by nine.fierce.ball...@gmail.com)
LGTM https://codereview.appspot.com/567450043/diff/555680043/lily/performance-scheme.cc File lily/performance-scheme.cc (left): https://codereview.appspot.com/567450043/diff/555680043/lily/performance-scheme.cc#oldcode31 lily/performance-scheme.cc:31: LY_DEFINE (ly_performance_set_header_x, "ly:performance-set-header!", Will this removal need a `convert-ly` warning? https://codereview.appspot.com/567450043/
Re: Make GET_LISTENER take a pointer arg and deduce its type (issue 549890043 by d...@gnu.org)
LGTM - but I don't know the internals well enough to really decide that. What about using two macros instead of one? The first would take a context as a first argument (as it does now), the second one uses 'this' in the macro body, omitting that as a macro argument. The former seems to be a rarer case than the latter. https://codereview.appspot.com/549890043/
Fix comment for Box::unite (issue 559870043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/559870043/diff/553910043/lily/include/box.hh File lily/include/box.hh (right): https://codereview.appspot.com/559870043/diff/553910043/lily/include/box.hh#newcode38 lily/include/box.hh:38: /// smallest box enclosing `this` and `b` Why '///' instead of '//'? https://codereview.appspot.com/559870043/
Re: midi: convert data to bigendian encoding directly (issue 565920043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/565920043/
stencil-integral: factor out repeated scm_car(expr) calls (issue 579590044 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/579590044/
python: Fix compile for in-tree builds (issue 581910043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/581910043/
Execute build scripts directly from source (issue 545870043 by jonas.hahnf...@gmail.com)
LGTM, with some nits. https://codereview.appspot.com/545870043/diff/549860043/make/doc-i18n-root-rules.make File make/doc-i18n-root-rules.make (right): https://codereview.appspot.com/545870043/diff/549860043/make/doc-i18n-root-rules.make#newcode10 make/doc-i18n-root-rules.make:10: find $(outdir)/$* -name '*.html' | xargs grep -L 'UNTRANSLATED NODE: IGNORE ME' | sed 's!$(outdir)/!!g' | xargs $(PYTHON) $(buildscript-dir)/mass-link.py --prepend-suffix .$(ISOLANG) hard $(outdir) $(top-build-dir)/Documentation/$(outdir) If you are going to change that please shorten the lines to 80 chars if possible. https://codereview.appspot.com/545870043/diff/549860043/scripts/build/help2man.pl File scripts/build/help2man.pl (left): https://codereview.appspot.com/545870043/diff/549860043/scripts/build/help2man.pl#oldcode1 scripts/build/help2man.pl:1: #!@PERL@ -w Not sure whether this is useful – help2man.pl is not part of LilyPond but gets imported, so I think it's best if it stays unchanged. Please check! https://codereview.appspot.com/545870043/diff/549860043/scripts/build/mf2pt1.pl File scripts/build/mf2pt1.pl (left): https://codereview.appspot.com/545870043/diff/549860043/scripts/build/mf2pt1.pl#oldcode1 scripts/build/mf2pt1.pl:1: #!@PERL@ Again an imported script, which should ideally stay unmodified for easy import of newer versions. https://codereview.appspot.com/545870043/
Re: Abstract Grob property storage into Mutable_properties. (issue 561640043 by hanw...@gmail.com)
>From visual inspection, LGTM.
I only wonder whether we should use
if
{
foo(...);
}
or
if
foo(...);
for single statements – right now, the source code contains both
variants...
https://codereview.appspot.com/561640043/
Re: Revert "Load only the default font for System_start_delimiter" (issue 557670043 by thomasmorle...@gmail.com)
some nits
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly
File input/regression/system-start-brace-style.ly (right):
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode4
input/regression/system-start-brace-style.ly:4: SystemStartGrob's style
of @code{StaffGroup} to @code{'brace}, always prints a
@code{SystemStart@var{Grob}} – and no comma after 'brace
https://codereview.appspot.com/557670043/diff/569610043/input/regression/system-start-brace-style.ly#newcode6
input/regression/system-start-brace-style.ly:6: Every @code{StaffGroup}
should start with a @code{SystemStartBrace}.
If you want a new paragraph, please add an empty line before the
sentence.
https://codereview.appspot.com/557670043/diff/569610043/lily/system-start-delimiter.cc
File lily/system-start-delimiter.cc (right):
https://codereview.appspot.com/557670043/diff/569610043/lily/system-start-delimiter.cc#newcode151
lily/system-start-delimiter.cc:151: esp. because that triggers mktextfm
for non-existent
mktextfm is no longer relevant to LilyPond since we don't have a TeX
back-end anymore. Maybe the comment should be just
We use the style sheet to look up the font file
name. This is better than using 'find_font' directly.
https://codereview.appspot.com/557670043/
Reduce number of calls to Bezier::dir_at_point by 2x (issue 583750044 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/583750044/
Shortcut Rational addition if either operand is zero (issue 551690046 by hanw...@gmail.com)
LGTM. Maybe you can add a comment w.r.t. speed improvement. https://codereview.appspot.com/551690046/
Re: Use python3 in website.make (issue 581880043 by hanw...@gmail.com)
Maybe this can be short-tracked to quickly get rid of the faulty webpage? https://codereview.appspot.com/581880043/
Re: Simplify mf-to-table (issue 549840043 by hanw...@gmail.com)
LGTM (without testing) https://codereview.appspot.com/549840043/
slur-configuration: fix size_t conversion warning (issue 545860043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/545860043/
Re: Cleanup many unused global variables (issue 547890043 by jonas.hahnf...@gmail.com)
LGTM, thanks. BTW, it's 'clean up' if you use it as a verb'. https://codereview.appspot.com/547890043/
Re: Fix line comments for new snippets (issue 549820043 by d...@gnu.org)
> I'd rather not have 2.21.0 in a state inconsistent > with its makelsr program. So if I were to push, > I'd need to push the results of a new makelsr run > as well. OK, I misunderstood. https://codereview.appspot.com/549820043/
Fix line comments for new snippets (issue 549820043 by d...@gnu.org)
LGTM, and please push directly. https://codereview.appspot.com/549820043/
Regtests cleanup: fix texidoc strings (issue 571950043 by v.villen...@gmail.com)
LGTM, thanks!
https://codereview.appspot.com/571950043/diff/545810043/input/regression/auto-beam-exceptions.ly
File input/regression/auto-beam-exceptions.ly (right):
https://codereview.appspot.com/571950043/diff/545810043/input/regression/auto-beam-exceptions.ly#newcode7
input/regression/auto-beam-exceptions.ly:7: texidoc = "beamExceptions is
used to modify the automatic beaming for certain durations;
@code{beamExceptions}
https://codereview.appspot.com/571950043/
Issue #1204: Document, and add regtest for, external fonts. (issue 557640051 by v.villen...@gmail.com)
Nice idea, thanks. Some nits and questions. https://codereview.appspot.com/557640051/diff/571940043/Documentation/notation/text.itely File Documentation/notation/text.itely (right): https://codereview.appspot.com/557640051/diff/571940043/Documentation/notation/text.itely#newcode1552 Documentation/notation/text.itely:1552: (and thus available in LilyPond scores), through the following commands: no comma https://codereview.appspot.com/557640051/diff/571940043/Documentation/notation/text.itely#newcode1558 Documentation/notation/text.itely:1558: I would remove this empty line in the @example environment. https://codereview.appspot.com/557640051/diff/571940043/input/regression/font-name-add-files.ly File input/regression/font-name-add-files.ly (right): https://codereview.appspot.com/557640051/diff/571940043/input/regression/font-name-add-files.ly#newcode14 input/regression/font-name-add-files.ly:14: dummyfontfile = #(string-append (tmpnam) "-dummyfont.otf") s/ / / https://codereview.appspot.com/557640051/diff/571940043/input/regression/font-name-add-files.ly#newcode210 input/regression/font-name-add-files.ly:210: #(mkdir dummyfontdir) Is it guaranteed that we can create this directory? What about srcdir != builddir? https://codereview.appspot.com/557640051/
Re: add property label-alignments to OttavaBracket (issue 575330043 by lilyp...@maltemeyn.de)
Minor doc nits.
https://codereview.appspot.com/575330043/diff/549780043/Documentation/changes.tely
File Documentation/changes.tely (right):
https://codereview.appspot.com/575330043/diff/549780043/Documentation/changes.tely#newcode67
Documentation/changes.tely:67: @item The labels can be changed by
setting the context property @code{ottavationMarkups}. The new default
is numbers only.
Please stay within 78 characters per line. And please use two spaces
after a full stop.
https://codereview.appspot.com/575330043/diff/549780043/Documentation/changes.tely#newcode70
Documentation/changes.tely:70: @item The labels can be vertically
aligned differently for brackets below/above the staff by setting the
grob property @code{label-alignments}.
If you insist on using '/' (instead of simply using the word 'or'), you
should add '@/' after the slash to allow a line break.
https://codereview.appspot.com/575330043/
Re: mf: use python scripting for generating Emmentaler fonts (issue 553580043 by hanw...@gmail.com)
https://codereview.appspot.com/553580043/diff/571810043/mf/gen-emmentaler-brace.fontforge.py File mf/gen-emmentaler-brace.fontforge.py (right): https://codereview.appspot.com/553580043/diff/571810043/mf/gen-emmentaler-brace.fontforge.py#newcode40 mf/gen-emmentaler-brace.fontforge.py:40: # mergeFonts takes a font, but this is a recent innovation fo s/fo/of/ https://codereview.appspot.com/553580043/
Re: Add dynamic-interface to keepAliveInterfaces (issue 553760043 by j...@abou-samra.fr)
LGTM https://codereview.appspot.com/553760043/diff/565830044/Documentation/notation/staff.itely File Documentation/notation/staff.itely (right): https://codereview.appspot.com/553760043/diff/565830044/Documentation/notation/staff.itely#newcode801 Documentation/notation/staff.itely:801: rests, skips, or a combination of these elements. This behavior can be Please use two spaces after a full stop. https://codereview.appspot.com/553760043/diff/565830044/Documentation/notation/staff.itely#newcode807 Documentation/notation/staff.itely:807: solely skips will then be removed. Maybe s/solely skips/skips only/ https://codereview.appspot.com/553760043/
Re: Issue 5864: improve Rational infinity initialization (issue 577710043 by nine.fierce.ball...@gmail.com)
LGTM https://codereview.appspot.com/577710043/
Re: Inline executable-* stepmake templates in lily/GNUmakefile (issue 577690043 by hanw...@gmail.com)
> > What's the reason this isn't called `LOADLIBS`? > > you should ask the GNU project. Aah, indeed, thanks. Well, `LOADLIBES` is deprecated, and the new name is `LDLIBS`. So maybe use `LDLIBS`? https://codereview.appspot.com/577690043/
Re: Issue 5036: 128 beaming output not producing output as expected (?) (issue 559700043 by torsten.haemme...@web.de)
LGTM https://codereview.appspot.com/559700043/diff/545760043/input/regression/beaming-more-than-4-beams-normal-size.ly File input/regression/beaming-more-than-4-beams-normal-size.ly (right): https://codereview.appspot.com/559700043/diff/545760043/input/regression/beaming-more-than-4-beams-normal-size.ly#newcode7 input/regression/beaming-more-than-4-beams-normal-size.ly:7: smoothly as possible (standard-sized beams). The outide-staff beams will s/outide/outside/, and please avoid future tense. https://codereview.appspot.com/559700043/diff/545760043/input/regression/beaming-more-than-4-beams-normal-size.ly#newcode9 input/regression/beaming-more-than-4-beams-normal-size.ly:9: when it comes to beam quanting/scoring/positioning." Please add `/@` after the `/`s to allow line breaks (in the PDF output). https://codereview.appspot.com/559700043/
Cleanup flower/ makefile (issue 577700045 by hanw...@gmail.com)
LGTM, with nits. https://codereview.appspot.com/577700045/diff/547810068/flower/GNUmakefile File flower/GNUmakefile (right): https://codereview.appspot.com/577700045/diff/547810068/flower/GNUmakefile#newcode17 flower/GNUmakefile:17: TEST_LOADLIBES = $(LIBRARY) $(CXXABI_LIBS) I suggest s/TEST_LOADLIBES/TEST_LOADLIBS/. https://codereview.appspot.com/577700045/diff/547810068/flower/GNUmakefile#newcode33 flower/GNUmakefile:33: AR=ar s/AR=ar/AR = ar/ for consistency. Irrespective of that I wonder why setting `AR` is necessary at all since the configure script sets this variable (via the `STEPMAKE_LIB` macro) – it seems to make that it won't work with cross-compiling. https://codereview.appspot.com/577700045/
Trim unused toplevel targets. (issue 547810069 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/547810069/diff/575870045/GNUmakefile.in File GNUmakefile.in (right): https://codereview.appspot.com/547810069/diff/575870045/GNUmakefile.in#newcode26 GNUmakefile.in:26: RELEASE_FILES = RELEASE-COMMIT Many GNU packages auto-generate a ChangeLog file from the git commit messages. Shall we do something similar? https://codereview.appspot.com/547810069/
Inline scm stepmake templates (issue 575870044 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/575870044/
Inline elisp stepmake templates. (issue 563780044 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/563780044/
Stop installing TFM and Type1 fonts. (issue 577700043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/577700043/
\compressFullBarRests should be renamed (issue 553750044 by v.villen...@gmail.com)
LGTM, thanks! I have some nits here and there, though.
https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely
File Documentation/changes.tely (right):
https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode66
Documentation/changes.tely:66: @code{\compressFullBarRests} has been
renamed
s/renamed/renamed to/
https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode67
Documentation/changes.tely:67: @code{\compressEmptyMeasures}, to clear
up
s/, to clear up/to avoid/
https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode70
Documentation/changes.tely:70:
Shouldn't \expandEmptyMeasures be mentioned as well?
https://codereview.appspot.com/553750044/diff/561590043/Documentation/de/notation/rhythms.itely
File Documentation/de/notation/rhythms.itely (right):
https://codereview.appspot.com/553750044/diff/561590043/Documentation/de/notation/rhythms.itely#newcode891
Documentation/de/notation/rhythms.itely:891: @funindex
expandEmptyMeasures
I suggest to fix the wrong index entries here: Only retain the ones with
a leading backslash.
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/rhythms.itely
File Documentation/notation/rhythms.itely (right):
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/rhythms.itely#newcode868
Documentation/notation/rhythms.itely:868: the note name uppercase
@code{R}. Their duration is entered
For single-letter stuff I suggest to use `@example` instead of `@code`.
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/rhythms.itely#newcode888
Documentation/notation/rhythms.itely:888: number of measure-lengths;
therefore, some time signatures
I guess you refer to the `measure-length` property, right? If this is
correct, then you should write `@code{measure-length}` instead.
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely
File Documentation/notation/staff.itely (right):
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode864
Documentation/notation/staff.itely:864: Methods to quote other voices
and format cue notes
Maybe s/format/to format/.
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1586
Documentation/notation/staff.itely:1586: @code{\compressMMRests} will
only apply to rests, and leave
no comma
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1588
Documentation/notation/staff.itely:1588: a property setting, its syntax
differs slightly in that
no comma
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1603
Documentation/notation/staff.itely:1603: rely on the
@code{@var{skipBars}} internal property, which is
why @var?
https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1605
Documentation/notation/staff.itely:1605: @ref{The set command}.
ditto
https://codereview.appspot.com/553750044/
Inline executable-* stepmake templates in lily/GNUmakefile (issue 577690043 by hanw...@gmail.com)
>From visual checking I'm not sure whether your changes work as expected... https://codereview.appspot.com/577690043/diff/581870043/lily/GNUmakefile File lily/GNUmakefile (right): https://codereview.appspot.com/577690043/diff/581870043/lily/GNUmakefile#newcode15 lily/GNUmakefile:15: LOADLIBES = $(FLOWER_LIB) $(CONFIG_LIBS) What's the reason this isn't called `LOADLIBS`? https://codereview.appspot.com/577690043/diff/581870043/lily/GNUmakefile#newcode18 lily/GNUmakefile:18: EXECUTABLES = $(notdir $(EXECUTABLE)) This is a funny name, too. AFAICS, this is just a single name, right? https://codereview.appspot.com/577690043/diff/581870043/lily/GNUmakefile#newcode28 lily/GNUmakefile:28: $(foreach a, $(EXECUTABLES), \ Since `EXECUTABLES` is a single name, we don't need a loop. And what happened with `SEXECUTABLES`? https://codereview.appspot.com/577690043/
Re: aclocal.m4: Support GUILE_CONFIG, document GUILE_FLAVOR (issue 575860044 by d...@gnu.org)
LGTM, and some minor nits. https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4 File aclocal.m4 (right): https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode625 aclocal.m4:625: AC_ARG_VAR(GUILE_FLAVOR, AS_HELP_STRING([], What about breaking the line here to get shorter lines? https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode630 aclocal.m4:630: AC_ARG_VAR(GUILE_CONFIG, [guile-config executable, obsoleted by pkgconfig/GUILE_FLAVOR])dnl Ditto. https://codereview.appspot.com/575860044/diff/547790043/aclocal.m4#newcode640 aclocal.m4:640: AC_MSG_ERROR([\$GUILE_CONFIG info guileversion failed: $tmp_GUILE_FLAVOR]) For consistency I suggest to add `;;` here, too. https://codereview.appspot.com/575860044/
Re: flower: Get rid of libc-extension (issue 553740043 by jonas.hahnf...@gmail.com)
https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly File input/regression/tie-single-manual.ly (right): https://codereview.appspot.com/553740043/diff/547780043/input/regression/tie-single-manual.ly#newcode19 input/regression/tie-single-manual.ly:19: \override Tie.staff-position = #-7.4 > I'm not really happy about tweaking this test like this. > It relies on rounding halfway always up (my_round) > instead of away from zero (std::round). I think this is > a coincidence, but if we want to retain this I need to > go through all replacements of my_round -> std::round > and check if the argument could be negative... Hmm. Are you talking about rounding to integers? If `staff-position` is a floating point value, LilyPond takes it as an exact value, see `has_manual_delta_y`. So there shouldn't be any rounding here... I've accidentally added this to the documentation today, see issue #4955. https://codereview.appspot.com/553740043/
Re: scripts/build/scan-mf-deps: script to generate MF dependencies (issue 553700043 by hanw...@gmail.com)
https://codereview.appspot.com/553700043/diff/555460043/mf/invoke-mf2pt.sh File mf/invoke-mf2pt.sh (right): https://codereview.appspot.com/553700043/diff/555460043/mf/invoke-mf2pt.sh#newcode18 mf/invoke-mf2pt.sh:18: # which no longer dump a .mem file > > We could probably get rid of the .mem file support, > > which is obsolete since about 10 years. > > sure. Followup change? ok – no hurry :-) https://codereview.appspot.com/553700043/diff/555460043/mf/invoke-mf2pt.sh#newcode31 mf/invoke-mf2pt.sh:31: --fullname=$name \ > > spaces vs. tabs? > > no idea. Whatever? Bad formulation, sorry. I meant that there might be a tabs-vs-spaces issue, and that tabs should be replaced with spaces (and/or the indentation should be fixed). https://codereview.appspot.com/553700043/
Re: scripts/build/scan-mf-deps: script to generate MF dependencies (issue 553700043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/553700043/diff/555460043/mf/invoke-mf2pt.sh File mf/invoke-mf2pt.sh (right): https://codereview.appspot.com/553700043/diff/555460043/mf/invoke-mf2pt.sh#newcode18 mf/invoke-mf2pt.sh:18: # which no longer dump a .mem file We could probably get rid of the .mem file support, which is obsolete since about 10 years. https://codereview.appspot.com/553700043/diff/555460043/mf/invoke-mf2pt.sh#newcode31 mf/invoke-mf2pt.sh:31: --fullname=$name \ spaces vs. tabs? https://codereview.appspot.com/553700043/
Remove compat hack for Solaris7 and HP-UX (issue 579480047 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/579480047/
Re: Fix output-distance tests (issue 569540043 by hanw...@gmail.com)
> Are there still systems that run without bash that we care about? I guess all BSD variants might miss bash since it is evil, evil GNU software. https://codereview.appspot.com/569540043/
Re: scripts/build/scan-mf-deps: script to generate MF dependencies (issue 553700043 by hanw...@gmail.com)
https://codereview.appspot.com/553700043/diff/545710043/mf/invoke-mf2pt.sh File mf/invoke-mf2pt.sh (right): https://codereview.appspot.com/553700043/diff/545710043/mf/invoke-mf2pt.sh#newcode4 mf/invoke-mf2pt.sh:4: mf2pt1=$(realpath $1) Second try. According to https://unix.stackexchange.com/questions/136494/whats-the-difference-between-realpath-and-readlink-f https://unix.stackexchange.com/questions/101080/realpath-command-not-found it seems that today a `realpath` binary is available on most systems. However, it is not mentioned in the suite of programs that autoconf uses universally. I thus recommend that we add a `configure` test for it – or at least mention it in the list of LilyPond build requisites. https://codereview.appspot.com/553700043/
Re: Fix output-distance tests (issue 569540043 by hanw...@gmail.com)
Just skimming the code: LGTM https://codereview.appspot.com/569540043/diff/555440061/scripts/build/output-distance.py File scripts/build/output-distance.py (right): https://codereview.appspot.com/569540043/diff/555440061/scripts/build/output-distance.py#newcode66 scripts/build/output-distance.py:66: # explicitly use bash, so we don't get dash on Ubuntu. This needs an update of `STEPMAKE_INIT` in `aclocal.m4`. Right now, AFAICS, `bash` is optional, and `configure` falls back to whatever is given in the `SHELL` environment variable. https://codereview.appspot.com/569540043/
Re: scripts/build/scan-mf-deps: script to generate MF dependencies (issue 553700043 by hanw...@gmail.com)
this looks like a good idea, thanks! https://codereview.appspot.com/553700043/diff/547740044/mf/invoke-mf2pt.sh File mf/invoke-mf2pt.sh (right): https://codereview.appspot.com/553700043/diff/547740044/mf/invoke-mf2pt.sh#newcode4 mf/invoke-mf2pt.sh:4: mf2pt1=$(realpath $1) I only know `realpath` as a GNU make command. Ditto for other commands below in this script. Is this script work in progress? https://codereview.appspot.com/553700043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
LGTM now, thanks! https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm File scm/define-music-types.scm (right): https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm#newcode685 scm/define-music-types.scm:685: . ((description . "A transition between lyric syllables.") Maybe s/transition/vowel transition/ ? https://codereview.appspot.com/565750043/
Re: configure.ac: Restore check for 'libguile18.h'. (issue 557580043 by lemzw...@googlemail.com)
> configure.ac:195: save_CPPFLAGS="$CXXFLAGS" > Took me some time to find this two-character typo :-( > fixed in staging with commit 7fddbc0ff1 Thanks a lot! https://codereview.appspot.com/557580043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
> Gould talks specifically about vowels, but I don't see > any reason why it shouldn't apply to sh->ss, or even > from vowel to closed mouth. How about > gradual-syllable-change-event? Mhmm, what about simply `vowel-transition-event`? IMHO it's not necessary to invent new names. https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
Some more nits :-)
https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-glossary.tely
File Documentation/music-glossary.tely (right):
https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-glossary.tely#newcode415
Documentation/music-glossary.tely:415: * transition arrow::
I think it would be better to replace 'transition arrow' in the glossary
with 'vowel transition'. How a vowel transition gets represented is a
technical detail.
https://codereview.appspot.com/565750043/diff/557610044/Documentation/music-glossary.tely#newcode7983
Documentation/music-glossary.tely:7983: D: ?,
A proper German translation of 'vowel transition' is 'Vokalwechsel'.
https://codereview.appspot.com/565750043/diff/557610044/input/regression/lyric-transition-padding.ly
File input/regression/lyric-transition-padding.ly (right):
https://codereview.appspot.com/565750043/diff/557610044/input/regression/lyric-transition-padding.ly#newcode4
input/regression/lyric-transition-padding.ly:4: shorter than
minimum-length. Instead, space is added if necessary
@code{minimum-length}
https://codereview.appspot.com/565750043/diff/557610044/lily/spanner.cc
File lily/spanner.cc (right):
https://codereview.appspot.com/565750043/diff/557610044/lily/spanner.cc#newcode380
lily/spanner.cc:380: SCM add_bounds = me->get_property
("minimum-length-add-bounds");
Are this and the next property internal ones? If yes, please document
them as such. Otherwise, please add a regression test to demonstrate
how they are used. This ensures that your code gets covered as much as
possible.
https://codereview.appspot.com/565750043/
Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)
Looks very nice, thanks!
I must admit that I've never seen such a feature before, so I can't
really comment on the actual implementation; my nits are just to improve
the documentation.
However, I wonder why you call this transition *line* and not transition
*arrow* ...
https://codereview.appspot.com/565750043/diff/553650043/Documentation/notation/vocal.itely
File Documentation/notation/vocal.itely (right):
https://codereview.appspot.com/565750043/diff/553650043/Documentation/notation/vocal.itely#newcode871
Documentation/notation/vocal.itely:871: (drawn as an arrows), which are
entered as @samp{ -> } between
Please use @samp{->}. The additional white space doesn't make sense in
a paragraph.
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-broken.ly
File input/regression/lyric-transition-broken.ly (right):
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-broken.ly#newcode5
input/regression/lyric-transition-broken.ly:5: texidoc = "Lyric
transitions run to the end of the line if it
s/Lyric transitions run/A lyric transition runs/
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-broken.ly#newcode7
input/regression/lyric-transition-broken.ly:7: the note on the next
line. Transition lines are printed at the
Two spaces after the full stop, please.
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-broken.ly#newcode17
input/regression/lyric-transition-broken.ly:17: \new Voice =A {
s/=A/= "A"/
I think it's better to always put identifiers into double quotes.
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-broken.ly#newcode22
input/regression/lyric-transition-broken.ly:22: \context Lyrics
\lyricsto A { a -> a -> ha }
s/A/"A"/
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-padding.ly
File input/regression/lyric-transition-padding.ly (right):
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-padding.ly#newcode3
input/regression/lyric-transition-padding.ly:3: texidoc = "Padding does
not cause LyricTransitions to become
@code{LyricTransition}s
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-padding.ly#newcode5
input/regression/lyric-transition-padding.ly:5: leaving the transition
line at minimum-length."
@code{minimum-length}
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-right-margin.ly
File input/regression/lyric-transition-right-margin.ly (right):
https://codereview.appspot.com/565750043/diff/553650043/input/regression/lyric-transition-right-margin.ly#newcode5
input/regression/lyric-transition-right-margin.ly:5: that the transition
can be drawn at minimum-length."
@code{minimum-length}
https://codereview.appspot.com/565750043/
Re: Issue 5829: Re-indent all mf files (issue 573610048 by torsten.haemme...@web.de)
Too lazy to check every file, but according to the few samples I looked at: LGTM, thanks! https://codereview.appspot.com/573610048/
Fix Windows resource to enable manifest (issue 551580044 by truer...@gmail.com)
LGTM https://codereview.appspot.com/551580044/
Re: aclocal.m4: Update PKG_* macros to pkg-config 0.29.2 (issue 551590044 by lemzw...@googlemail.com)
Reviewers: hanwenn, hahnjo, Message: > > can we include this rather than copy & paste? > > This would require generating aclocal.m4 by aclocal as > part of generating configure. I'm all for doing this, > but I'd avoid doing this level of tooling changes > before 2.21.0 Seconded. Description: aclocal.m4: Update PKG_* macros to pkg-config 0.29.2 This commit removes the old definition of PKG_CHECK_MODULES (from 2004) and simply appends a recent version of pkg-config's file `pkg.m4` at the end of `aclocal.m4`. This immediately improves documentation of pkg-config stuff. Now the list of environment variables shown in `configure --help` ends with PKG_CONFIG path to pkg-config utility PKG_CONFIG_PATH directories to add to pkg-config's search path PKG_CONFIG_LIBDIR path overriding pkg-config's built-in search path GUILE_CFLAGS C compiler flags for GUILE, overriding pkg-config GUILE_LIBS linker flags for GUILE, overriding pkg-config BDWGC_CFLAGS C compiler flags for BDWGC, overriding pkg-config BDWGC_LIBS linker flags for BDWGC, overriding pkg-config PANGO_FT2_CFLAGS C compiler flags for PANGO_FT2, overriding pkg-config PANGO_FT2_LIBS linker flags for PANGO_FT2, overriding pkg-config FONTCONFIG_CFLAGS C compiler flags for FONTCONFIG, overriding pkg-config FONTCONFIG_LIBS linker flags for FONTCONFIG, overriding pkg-config FREETYPE2_CFLAGS C compiler flags for FREETYPE2, overriding pkg-config FREETYPE2_LIBS linker flags for FREETYPE2, overriding pkg-config GLIB_CFLAGS C compiler flags for GLIB, overriding pkg-config GLIB_LIBS linker flags for GLIB, overriding pkg-config GOBJECT_CFLAGS C compiler flags for GOBJECT, overriding pkg-config GOBJECT_LIBS linker flags for GOBJECT, overriding pkg-config Please review this at https://codereview.appspot.com/551590044/ Affected files (+277, -54 lines): M aclocal.m4
Move metafont stepmake templates into mf/GNUmakefile (issue 577610043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/577610043/
make: remove unused mutopia templates (issue 559600043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/559600043/
Calculate download sizes rather than hardcoding them (issue 567340043 by d...@gnu.org)
Very nice, thanks! From visual inspection, LGTM, with only minor nits.
https://codereview.appspot.com/567340043/diff/565730043/scripts/build/fix-docsize.sh
File scripts/build/fix-docsize.sh (right):
https://codereview.appspot.com/567340043/diff/565730043/scripts/build/fix-docsize.sh#newcode37
scripts/build/fix-docsize.sh:37: sed -n
's|^\./||;1h;1!H;${x;/\n/{s/^/{/;s/\n/,/g;s/$/}/};p}')" >&2
I think you can increase the readablity of the sed command if you use
more than a single line for the series of commands.
Additionally, according to the autoconf info manual, you shouldn't
combine '!' and ';' – I guess that modern sed implementations are not
affected, but...
https://codereview.appspot.com/567340043/diff/565730043/scripts/build/fix-docsize.sh#newcode50
scripts/build/fix-docsize.sh:50: sed 's/^[ ]*\([^ ]*\)[ ]*\([^ ]*\)[
]*$/\/|\\1\1\\2|/')
What about putting the 's|...|...|' into a separate line?
https://codereview.appspot.com/567340043/
Add some const declarations to page breaking code (issue 569490044 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/569490044/
Use $(MAKE) instead of 'make' throughout (issue 565720043 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/565720043/
Don't add . to PATH in Make (issue 563650043 by d...@gnu.org)
LGTM https://codereview.appspot.com/563650043/
Remove more unused bits from aclocal.m4 (issue 549680043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/549680043/
Re: Issue 5806: Tweak mf files to avoid FontForge internal overlap error (issue 571780043 by torsten.haemme...@web.de)
LGTM, thanks! https://codereview.appspot.com/571780043/
Re: Issue 5806: Tweak mf files to avoid FontForge internal overlap error (issue 571780043 by torsten.haemme...@web.de)
not happy with indentation yet... https://codereview.appspot.com/571780043/diff/549620043/mf/feta-scripts.mf File mf/feta-scripts.mf (left): https://codereview.appspot.com/571780043/diff/549620043/mf/feta-scripts.mf#oldcode999 mf/feta-scripts.mf:999: .. z2l > Nevertheless, I've changed the indentation to 8 characters. Thanks. I now see that the word 'indentation' is misleading. What I actually mean is to vertically align the continuation lines as done everywhere else in the code. https://codereview.appspot.com/571780043/diff/579360043/mf/feta-arrowheads.mf File mf/feta-arrowheads.mf (right): https://codereview.appspot.com/571780043/diff/579360043/mf/feta-arrowheads.mf#newcode83 mf/feta-arrowheads.mf:83: .. reverse pat yscaled -1 Hmm. This is not the vertical alignment used in other `.mf` files. For example, at this very spot it should be arrow_path := pat .. reverse pat yscaled -1 .. cycle; this is, the `..` operator has the same indentation as `pat`. Similar indentation is often used in C code, for example. Any reason for doing it differently? https://codereview.appspot.com/571780043/
Re: mf: use python scripting for generating Emmentaler fonts (issue 553580043 by hanw...@gmail.com)
> > Can we assume that FontForge's python support and is > > always enabled? Shall we check this? > > the FF page doesn't say that python is optional. It's a build option in both the old (configure) and new (cmake) builds... https://codereview.appspot.com/553580043/
Re: build cleanups. (issue 547690053 by hanw...@gmail.com)
LGTM https://codereview.appspot.com/547690053/
Re: mf: use python scripting for generating Emmentaler fonts (issue 553580043 by hanw...@gmail.com)
LGTM; some minor nits https://codereview.appspot.com/553580043/diff/571780045/mf/GNUmakefile File mf/GNUmakefile (right): https://codereview.appspot.com/553580043/diff/571780045/mf/GNUmakefile#newcode130 mf/GNUmakefile:130: $(outdir)/emmentaler-brace.otf: emmentaler-brace.fontforge.py $(outdir)/emmentaler-brace.otf-table \ maybe break line to make it shorter... https://codereview.appspot.com/553580043/diff/571780045/mf/emmentaler-brace.fontforge.py File mf/emmentaler-brace.fontforge.py (right): https://codereview.appspot.com/553580043/diff/571780045/mf/emmentaler-brace.fontforge.py#newcode1 mf/emmentaler-brace.fontforge.py:1: #!@FONTFORGE@ Can we assume that FontForge's python support and is always enabled? Shall we check this? https://codereview.appspot.com/553580043/diff/571780045/mf/emmentaler-brace.fontforge.py#newcode6 mf/emmentaler-brace.fontforge.py:6: import fontforge importing 'fontforge' twice is correct? If yes, please add a comment. https://codereview.appspot.com/553580043/diff/571780045/mf/gen-emmentaler.fontforge.py File mf/gen-emmentaler.fontforge.py (right): https://codereview.appspot.com/553580043/diff/571780045/mf/gen-emmentaler.fontforge.py#newcode63 mf/gen-emmentaler.fontforge.py:63: alphabet ="feta-alphabet%(design_size)d" % vars() whitespace https://codereview.appspot.com/553580043/
Re: Issue 5806: Tweak mf files to avoid FontForge internal overlap error (issue 571780043 by torsten.haemme...@web.de)
Very nice, thanks! LGTM.
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-arrowheads.mf
File mf/feta-arrowheads.mf (right):
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-arrowheads.mf#newcode58
mf/feta-arrowheads.mf:58: y5 = 0;
Please use an indentation of 8 spaces for orthogonality.
We could later decide to reduce the indentation to two (or four) spaces
– which I like much better. However, this would be a lot of work and is
probably not worth the trouble.
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf
File mf/feta-numbers.mf (left):
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf#oldcode467
mf/feta-numbers.mf:467: ..tension 0.9.. {dir (alpha + 10)}z2r
indentation
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf
File mf/feta-numbers.mf (right):
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf#newcode467
mf/feta-numbers.mf:467: pata:= subpath (0.1, 5) of z1r{dir (beta)}
s/pata:=/pata :=/
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf#newcode493
mf/feta-numbers.mf:493: fill subpath (ta,length pata) of pata
space after comma
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf#newcode494
mf/feta-numbers.mf:494: .. reverse subpath (tb,length patb) of patb
indentation
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf#newcode750
mf/feta-numbers.mf:750: .. {dir (delta - 90)}z3r
indentation
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-numbers.mf#newcode793
mf/feta-numbers.mf:793: patb := z13l{dir (180 + beta)}
indentation
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-scripts.mf
File mf/feta-scripts.mf (left):
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-scripts.mf#oldcode999
mf/feta-scripts.mf:999: .. z2l
indentation
https://codereview.appspot.com/571780043/
Add comments to code related to page breaking/layout (issue 563630043 by hanw...@gmail.com)
LGTM. Please feel free to ignore (most of) my remarks if you consider such nitpicking as unnecessary :-) https://codereview.appspot.com/563630043/diff/571770043/lily/include/page-breaking.hh File lily/include/page-breaking.hh (right): https://codereview.appspot.com/563630043/diff/571770043/lily/include/page-breaking.hh#newcode107 lily/include/page-breaking.hh:107: Read the large commennt at the top of page-breaking.cc for context. s/commennt/comment/ https://codereview.appspot.com/563630043/diff/571770043/lily/include/page-spacing.hh File lily/include/page-spacing.hh (right): https://codereview.appspot.com/563630043/diff/571770043/lily/include/page-spacing.hh#newcode101 lily/include/page-spacing.hh:101: we add lines. details details what? Looks like something is missing accidentally. https://codereview.appspot.com/563630043/diff/571770043/scm/page.scm File scm/page.scm (right): https://codereview.appspot.com/563630043/diff/571770043/scm/page.scm#newcode51 scm/page.scm:51: of layout settings just like markups inside the music" Final stop missing. https://codereview.appspot.com/563630043/diff/571770043/scm/page.scm#newcode96 scm/page.scm:96: "Add a annotation at the top to STENCIL and return new stencil." s/a/an/ https://codereview.appspot.com/563630043/diff/571770043/scm/page.scm#newcode119 scm/page.scm:119: "add annotations to a stencil, and return result" Add ... result. https://codereview.appspot.com/563630043/diff/571770043/scm/paper-system.scm File scm/paper-system.scm (right): https://codereview.appspot.com/563630043/diff/571770043/scm/paper-system.scm#newcode38 scm/paper-system.scm:38: "add stencils for notes to the main stencil, returning the result." s/add/Add/ https://codereview.appspot.com/563630043/diff/571770043/scm/paper-system.scm#newcode89 scm/paper-system.scm:89: "Y-ext and next-Y-ext are either skyline-pairs or extents" Final stop missing. https://codereview.appspot.com/563630043/
Fixes for cross-compilation to x86_64-w64-mingw32 (issue 579340043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/579340043/
aclocal.m4: also recognize guile2.2 (issue 555370043 by hanw...@gmail.com)
LGTM. https://codereview.appspot.com/555370043/
Re: Issue 5788: New French Beamimg Approach (issue 557500043 by torsten.haemme...@web.de)
> How about > * beamed-stem-french-adjustment > * french-beaming-stem-adjustment > …? I like the second one. https://codereview.appspot.com/557500043/
Re: aclocal.m4 (STEPMAKE_GUILE_DEVEL): Fix logic and improve diagnostics. (issue 573570044 by lemzw...@googlemail.com)
Reviewers: hahnjo, Message: > Do we really want that many "no"s in the output Currently, you get this (in one long line): checking for guile-1.8 >= 1.8.2... checking for guile-2.2 >= 2.2.0... checking for guile-2.0 >= 2.0.7... Package guile-2.0 was not found in the pkg-config search path. Perhaps you should add the directory containing `guile-2.0.pc' to the PKG_CONFIG_PATH environment variable No package 'guile-2.0' found With the above patch, you get checking for guile-1.8 >= 1.8.2... no checking for guile-2.2 >= 2.2.0... no checking for guile-2.0 >= 2.0.7... no ... ERROR: Please install required programs: guile-devel >= 1.8 So I think the many 'no' strings are the right thing to do. Description: aclocal.m4 (STEPMAKE_GUILE_DEVEL): Fix logic and improve diagnostics. Add quotes around string argument that contains a logical operator to avoid unexpected results. Also use $PKG_CONFIG everywhere. Please review this at https://codereview.appspot.com/573570044/ Affected files (+20, -13 lines): M aclocal.m4 Index: aclocal.m4 diff --git a/aclocal.m4 b/aclocal.m4 index 19345394857c85a6d6f0e8a8cb8796d9bb928245..0d63a178aa16e12802a17fb265cf7fc1584d0d7b 100644 --- a/aclocal.m4 +++ b/aclocal.m4 @@ -669,13 +669,20 @@ AC_DEFUN(STEPMAKE_GUILE, [ AC_DEFUN(STEPMAKE_GUILE_DEVEL, [ if test -n "$GUILE_FLAVOR"; then -PKG_CHECK_MODULES([GUILE], [$GUILE_FLAVOR], true, [GUILE_FLAVOR=""]) +PKG_CHECK_MODULES([GUILE], [$GUILE_FLAVOR], +[true], [GUILE_FLAVOR="missing"]) else -PKG_CHECK_MODULES([GUILE], [guile-1.8 >= 1.8.2], [GUILE_FLAVOR="guile-1.8"], [ -PKG_CHECK_MODULES( -[GUILE], [guile-2.2 >= 2.2.0], [GUILE_FLAVOR="guile-2.2"], [ -PKG_CHECK_MODULES([GUILE], [guile-2.0 >= 2.0.7], [GUILE_FLAVOR="guile-2.0"]) -]) +PKG_CHECK_MODULES([GUILE], [guile-1.8 >= 1.8.2], +[GUILE_FLAVOR="guile-1.8"], [ +AC_MSG_RESULT([no]) +PKG_CHECK_MODULES([GUILE], [guile-2.2 >= 2.2.0], +[GUILE_FLAVOR="guile-2.2"], [ +AC_MSG_RESULT([no]) +PKG_CHECK_MODULES([GUILE], [guile-2.0 >= 2.0.7], +[GUILE_FLAVOR="guile-2.0"], [ +AC_MSG_RESULT([no]) +GUILE_FLAVOR="missing"]) +]) ]) fi @@ -686,7 +693,7 @@ AC_DEFUN(STEPMAKE_GUILE_DEVEL, [ guile-1.8) ;; *) -STEPMAKE_ADD_ENTRY(REQUIRED, [guile-devel >= 1.8]) +STEPMAKE_ADD_ENTRY(REQUIRED, ["guile-devel >= 1.8"]) ;; esac ]) @@ -1093,7 +1100,7 @@ AC_DEFUN(STEPMAKE_GLIB, [ LIBS="$save_LIBS" else r="libglib-dev or glib?-devel" -ver="`pkg-config --modversion $1`" +ver="`$PKG_CONFIG --modversion $1`" STEPMAKE_ADD_ENTRY($2, ["$r >= $3 (installed: $ver)"]) fi ]) @@ -1112,7 +1119,7 @@ AC_DEFUN(STEPMAKE_GOBJECT, [ LIBS="$save_LIBS" else r="libgobject-dev or gobject?-devel" -ver="`pkg-config --modversion $1`" +ver="`$PKG_CONFIG --modversion $1`" STEPMAKE_ADD_ENTRY($2, ["$r >= $3 (installed: $ver)"]) fi ]) @@ -1134,7 +1141,7 @@ AC_DEFUN(STEPMAKE_FREETYPE2, [ # URG #r="lib$1-dev or $1-devel" r="libfreetype6-dev or freetype?-devel" -ver="`pkg-config --modversion $1`" +ver="`$PKG_CONFIG --modversion $1`" STEPMAKE_ADD_ENTRY($2, ["$r >= $3 (installed: $ver)"]) fi ]) @@ -1157,7 +1164,7 @@ AC_DEFUN(STEPMAKE_PANGO_FT2, [ # URG #r="lib$1-dev or $1-devel"e r="libpango1.0-dev or pango?-devel" -ver="`pkg-config --modversion $1`" +ver="`$PKG_CONFIG --modversion $1`" STEPMAKE_ADD_ENTRY($2, ["$r >= $3 (installed: $ver)"]) fi ]) @@ -1183,7 +1190,7 @@ AC_DEFUN(STEPMAKE_PANGO_FT2_WITH_OTF_FEATURE, [ # URG #r="lib$1-dev or $1-devel"e r="libpango1.0-dev or pango?-devel" -ver="`pkg-config --modversion $1`" +ver="`$PKG_CONFIG --modversion $1`" STEPMAKE_ADD_ENTRY($2, ["$r >= $3 (It is required if you'd like "]) STEPMAKE_ADD_ENTRY($2, ["to use OpenType font feature. "]) STEPMAKE_ADD_ENTRY($2, ["installed: $ver)"]) @@ -1206,7 +1213,7 @@ AC_DEFUN(STEPMAKE_FONTCONFIG, [ LIBS="$save_LIBS" else r="lib$1-dev or $1-devel" -ver="`pkg-config --modversion $1`" +ver="`$PKG_CONFIG --modversion $1`" STEPMAKE_ADD_ENTRY($2, ["$r >= $3 (installed: $ver)"]) fi ])
Re: Issue 5788: New French Beamimg Approach (issue 557500043 by torsten.haemme...@web.de)
> It sounds like actual manipulation of that property > by the user would interfere with the implementation > of french-beaming. So maybe just mark/sort it as an > internal property and only regtest french-beaming as > such? If this is the intention, yes. https://codereview.appspot.com/557500043/
Remove outdated compiler checks in configure (issue 551510043 by jonas.hahnf...@gmail.com)
LGTM https://codereview.appspot.com/551510043/
