Re: Fixes for cross-compilation to x86_64-w64-mingw32 (issue 579340043 by jonas.hahnf...@gmail.com)

[benko . pal]

On 2020/02/28 09:32:57, hahnjo wrote:
> On 2020/02/28 09:23:44, benko.pal wrote:
> > it may not matter, but actually long is 32 bit, void * is 64 bit on
current
> > native 64 bit platforms too, I believe.
> 
> Only mingw, on Linux long is 64 bit AFAICT. See also
> https://stackoverflow.com/a/384672/10606944

I stand corrected, thanks.

https://codereview.appspot.com/579340043/

Re: Fixes for cross-compilation to x86_64-w64-mingw32 (issue 579340043 by jonas.hahnf...@gmail.com)

[benko . pal]

it may not matter, but actually long is 32 bit, void * is 64 bit on
current native 64 bit platforms too, I believe.

https://codereview.appspot.com/579340043/

Re: Issue 5740: Add \post to defer context actions to end of time step (issue 581600043 by nine.fierce.ball...@gmail.com)

[benko . pal]

On 2020/02/06 14:29:55, Dan Eble wrote:
> The reviewers are turning the first question I asked around and asking
it back
> to me.  I don't know if this is useful without other stuff I've been
working on.
>  That's why I've posted it for review.  I thought that you (well,
mainly I
> thought that David K.) might know of some corner of LilyPond where
delaying a
> \set (or similar thing) until the end of the current time step would
be useful.

I remember very faintly (incorrectly???) that many years ago I had a
problem of
doing something at the last point in time, as points of time could be
deduced only
from the beginning of durations, not from the end of those; if that is
so (or if
there are cases when determining a point of time is more natural as the
end of a
duration than as the beginning of another one), \post is definitely
useful.

Another conceivable meaning is, used together with \once:
"tweak something, and when it finished, tweak something else"
(may perhaps be useful when pasting music from tiny fragments or using a
complex
hierarchy of music functions).
This meaning would perhaps be
- expressed better with \next (or \then? is it reserved?) than \post;
- supported better if \once and \post could be iterated like if .. else
if .. else,
i.e. if

\once \set  \post \once \set  \post \set 
  ...

meant

\set 

\set 

\set 
...


but as the new set-once-post.ly regtest shows, this is not how it works
with this patch.

https://codereview.appspot.com/581600043/

Re: Use a pointer for the output parameter of Lily_lexer::scan_word (issue 577440044 by hanw...@gmail.com)

[benko . pal]

On 2020/01/29 06:43:19, hanwenn wrote:
> score performer

removed unused: looks good.

changing references to pointers: looks BD.

https://codereview.appspot.com/577440044/

Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)

[benko . pal]

https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc
File lily/parse-scm.cc (right):

https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#newcode77
lily/parse-scm.cc:77: const Input *hi = >start_;
I understand (and like) adding the const, but can't understand changing
the reference to pointer even after reading the whole discussion.

https://codereview.appspot.com/577410045/

Re: lily: fix some type conversion warnings (issue 557190043 by hanw...@gmail.com)

[benko . pal]

On 2020/01/22 10:15:03, hanwenn wrote:
> Why do we need to have the distinction between size_t and int? I know
the
> standard library returns size_t in some places, but is there any
reason for
> LilyPond to used unsigned integers anywhere? 

It's not just the sign (btw I like unsigned integers):
they also differ in size, at least on 64 bit architectures
int is int32_t, size_t is uint64_t.

p

https://codereview.appspot.com/557190043/

Remove spurious '% begin verbatim' in Documentation/snippets/new (issue 583000043 by d...@gnu.org)

[benko . pal]

LGTM


https://codereview.appspot.com/58343/diff/568950043/Documentation/snippets/figured-bass-headword.ly
File Documentation/snippets/figured-bass-headword.ly (right):

https://codereview.appspot.com/58343/diff/568950043/Documentation/snippets/figured-bass-headword.ly#newcode35
Documentation/snippets/figured-bass-headword.ly:35:
I guess there should be no empty line at all here

https://codereview.appspot.com/58343/diff/568950043/Documentation/snippets/heavily-customized-polymetric-time-signatures.ly
File
Documentation/snippets/heavily-customized-polymetric-time-signatures.ly
(right):

https://codereview.appspot.com/58343/diff/568950043/Documentation/snippets/heavily-customized-polymetric-time-signatures.ly#newcode39
Documentation/snippets/heavily-customized-polymetric-time-signatures.ly:39:
\new Staff \with {
indentation unneeded in this block

https://codereview.appspot.com/58343/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Avoid some crashes for bad "control-points" property (issue 576610043 by d...@gnu.org)

[benko . pal]

LGTM

https://codereview.appspot.com/576610043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Spacing_spanner::prune_loose_columns: prune in-place (issue 351720043 by d...@gnu.org)

[benko . pal]

On 2018/07/10 18:09:19, dak wrote:

On 2018/07/10 17:49:33, dak wrote:
> On 2018/07/10 17:10:53, benko.pal wrote:
> > LGTM; just by looking I can't see how it can make make fail.
> > using rp[-2] and rp[-1] instead of lastcol and c would be cleaner

to me, but

> > YMMV.
>
> Since the whole point is to do the operation in-place, rp[-2] may

already have

> been overwritten by rp[-1] in the last iteration.  It may seem

cleaner to you

to
> use rp[-2] but it would be a rather ugly bug.  Doing things in-place

is more

> efficient but you have to keep track of what you are doing.


ah yes, thanks for pointing this out!
actually this may worth some comments; I mean, documenting why looseness
decision is based only on original neighbours, not on after-pruning
neighbours (I know rp[-2] may not be the after-pruning neighbour
either).  this may be obvious, but I feel uneasy by just reading this
file.


> I consider it possible that mixing a const iterator with a non-const

one might

> be what caused the compilation error but I'd want to see the error

message to

> be sure.



Just seen  which indicates that

as of

C++11, you can use a const_iterator for a call to erase (since the

modifiability

is ensured by the object erase is called on and the const-ness of the

iterator

is irrelevant) but apparently not previously.  So I guess it is my

newer

compiler that made the erase call work without complaint.  I'll see

how I can

make this C++08(?)-save.


that was my guess too, but while I messed with git and make, you beat me
:)
looks still good to me.

https://codereview.appspot.com/351720043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Spacing_spanner::prune_loose_columns: prune in-place (issue 351720043 by d...@gnu.org)

[benko . pal]

LGTM; just by looking I can't see how it can make make fail.
using rp[-2] and rp[-1] instead of lastcol and c would be cleaner to me,
but YMMV.

https://codereview.appspot.com/351720043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: issue 3208: MMRs for > 1 m. only count m. (issue 333340043 by lilyp...@maltemeyn.de)

[benko . pal]

On 2018/01/03 20:37:33, Malte Meyn wrote:

I think you’re right. (I found the German translation at our

university

library.)



For single measure rests she writes (section II.6)
• semibrevis rest for any time signature
• exception: brevis rest for 4/2, 8/4 and longer
→ LilyPond does this but adds
• exception: longa rest for 4/1 and longer, maxima rest for 8/1 and

longer


For compressed multi measure rests she writes (section III.18)
• \override MultiMeasureRest.expand-limit = [one of 1 “modern”, 9

“classic”, or

2 “many editions”]
• how 2, 3, 4, 5, 6, 7, 8, 9 measure rests are written if expand-limit

= 9; she

doesn’t mention time signature/measure length at all here
→ LilyPond does this with the following exceptions
• default expand-limit is 10, not 9
• the forms for 2 to 9 bars rest are only the ones shown by her for

measure

lengths < 2/1 until this patch comes and fixes that



In section III.18 she writes something like “put a 1 above a single

measure

rest” and shows only an example with a semibrevis rest (without

mentioning

measure length) but doesn’t do that in II.6 …


is there an example where a single measure rest looks different standing
alone than one in an expanded multi-measure rest of odd measures?

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

Re: Let repeatTie work inside of event-chord (issue 335910043 by thomasmorle...@gmail.com)

[benko . pal]

if it's really the same, can't it be used without copying?  or if
separate classes are absolutely needed, can't the common part be
gathered in a common base class?

https://codereview.appspot.com/335910043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: add some rarely used mensural clefs (issue 330120043 by benko....@gmail.com)

[benko . pal]

https://codereview.appspot.com/330120043/diff/1/scm/parser-clef.scm
File scm/parser-clef.scm (right):

https://codereview.appspot.com/330120043/diff/1/scm/parser-clef.scm#newcode96
scm/parser-clef.scm:96: ("petrucci-g2" . ("clefs.petrucci.g" -2 0))
On 2017/09/10 13:02:25, pkx166h wrote:

Is this correct?



I know nothing about ancient notation, but it seems that "petrucci-g2"

is

identical "petrucci-g" (below) apart from using 'g2' instead of 'g1'.



On a similar note I can see that petrucci-f and f4 also are identical

(and

produce the same output).


yes, this is all correct and intended.
we could have petrucci-french instead of the new g1, but we don't have
petrucci-violin (petrucci-alto, etc.) either -- so long we went with
petrucci-c1 etc.
now as I added the g1 variant, for consistency reasons seemed worthwile
to have g2 as alias of g, just like petrucci-f4 is an alias of
petrucci-f since long (I don't know which one was first, f or f4).
I thought about introducing a petrucci-c alias to one of the c-clefs,
but since all five are used frequently (and together -- in different
parts), any choice would be rather confusing than logical.

https://codereview.appspot.com/330120043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

add some rarely used mensural clefs (issue 330120043 by benko....@gmail.com)

[benko . pal]

Reviewers: ,

Message:
g1 clef occurs in English manuscripts;
f2 clef occurs in Ockeghem's Missa prolationum (Chigi codex)

Description:
add some rarely used mensural clefs
and an alias petrucci-g2 to petrucci-g

Please review this at https://codereview.appspot.com/330120043/

Affected files (+9, -0 lines):
  M scm/parser-clef.scm


Index: scm/parser-clef.scm
diff --git a/scm/parser-clef.scm b/scm/parser-clef.scm
index  
ea9f67fade20959cfb49300c21e52aba51770d2b..9832753445166c55b0dc1986c60db29b39d039ad  
100644

--- a/scm/parser-clef.scm
+++ b/scm/parser-clef.scm
@@ -87,10 +87,13 @@
 ("petrucci-c3" . ("clefs.petrucci.c3" 0 0))
 ("petrucci-c4" . ("clefs.petrucci.c4" 2 0))
 ("petrucci-c5" . ("clefs.petrucci.c5" 4 0))
+("petrucci-f2" . ("clefs.petrucci.f" -2 0))
 ("petrucci-f3" . ("clefs.petrucci.f" 0 0))
 ("petrucci-f4" . ("clefs.petrucci.f" 2 0))
 ("petrucci-f5" . ("clefs.petrucci.f" 4 0))
 ("petrucci-f" . ("clefs.petrucci.f" 2 0))
+("petrucci-g1" . ("clefs.petrucci.g" -4 0))
+("petrucci-g2" . ("clefs.petrucci.g" -2 0))
 ("petrucci-g" . ("clefs.petrucci.g" -2 0))
 ("kievan-do" . ("clefs.kievan.do" 0 0

@@ -123,7 +126,13 @@
 ("clefs.petrucci.c3" . 0)
 ("clefs.petrucci.c4" . 0)
 ("clefs.petrucci.c5" . 0)
+("clefs.petrucci.f2" . 4)
+("clefs.petrucci.f3" . 4)
+("clefs.petrucci.f4" . 4)
+("clefs.petrucci.f5" . 4)
 ("clefs.petrucci.f" . 4)
+("clefs.petrucci.g1" . -4)
+("clefs.petrucci.g2" . -4)
 ("clefs.petrucci.g" . -4)
 ("clefs.kievan.do" . 0)))




___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: NR: Mention standalone accidentals in figuremode (issue 299510043 by g...@ursliska.de)

[benko . pal]

LGTM

https://codereview.appspot.com/299510043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: CG: push to staging with rebase not merge (issue 292370043 by paulwmor...@gmail.com)

[benko . pal]

https://codereview.appspot.com/292370043/diff/1/Documentation/contributor/source-code.itexi
File Documentation/contributor/source-code.itexi (right):

https://codereview.appspot.com/292370043/diff/1/Documentation/contributor/source-code.itexi#newcode2296
Documentation/contributor/source-code.itexi:2296: on savannah.
or by running
$ gitk
after
$ git fetch
or
$ git pull -r
as suggested later, but then check for origin/master being the same as
origin/staging (actually there's no need for a local master or staging
branch).
or simply whether
$ git log origin/master..origin/staging
lists any commit.

https://codereview.appspot.com/292370043/diff/1/Documentation/contributor/source-code.itexi#newcode2325
Documentation/contributor/source-code.itexi:2325: git pull -r origin
staging
$ git pull -r
suffices if staging is not ahead (and branch_name is based on
origin/master)

https://codereview.appspot.com/292370043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Issue 4767: Crash with Completion_heads_engraver and bare durations (issue 288100043 by d...@gnu.org)

[benko . pal]

LGTM

https://codereview.appspot.com/288100043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Change several instances of "git cl" in the Contributors Guide to "git-cl". (issue 282960043 by j...@weathervanefarm.net)

[benko . pal]

Is this needed in some environments/with some git versions?  git cl with
a space always worked for me (just as all the regular git commands did).
It doesn't hurt though, so LGTM.

https://codereview.appspot.com/282960043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: modify coord-rotate to get exact values for (sin PI) etc (issue 269530043 by thomasmorle...@gmail.com)

[benko . pal]

sorry to chime in that late, but:
am I right that the problem is that we get the rotation matrix
cos a   -sin a
sin acos a
inexact?  and if so, how the inexactness is present?  one of the
diagonals is exactly +/-1 while the other is not exactly 0?
in that case I'd suggest (would have suggested) to perform a paranoid
normalization of the (cos a, sin a) vector, which e.g. normalizes (1,
1e-9) into (1, 0).


https://codereview.appspot.com/269530043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

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

[benko . pal]

https://codereview.appspot.com/264950043/diff/40001/lily/include/grob-array.hh
File lily/include/grob-array.hh (right):

https://codereview.appspot.com/264950043/diff/40001/lily/include/grob-array.hh#newcode63
lily/include/grob-array.hh:63: // Note: This method may reorder the
array without affecting the result of
what does "reorder" mean in the current context?  the function replaces
some grobs, deletes others.

https://codereview.appspot.com/264950043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: Issue 4528/3 Document ssaattbb.ly built-in template (issue 256340043 by tdanielsmu...@googlemail.com)

[benko . pal]

https://codereview.appspot.com/256340043/diff/1/input/regression/ssaattbb-template-with-changed-instrument-names.ly
File input/regression/ssaattbb-template-with-changed-instrument-names.ly
(left):

https://codereview.appspot.com/256340043/diff/1/input/regression/ssaattbb-template-with-changed-instrument-names.ly#oldcode5
input/regression/ssaattbb-template-with-changed-instrument-names.ly:5:
can be changed when using the satb built-in template.
On 2015/08/01 22:14:13, Trevor Daniels wrote:

git-cl is showing the wrong base file here.  While it's true I used

this file as

the starting point, the ssaattbb file should be new, not shown as an

update of

this one - and in my repository it is new and the satb file is still

present and

correct.  ??


git tracks complete trees; it does NOT track individual files directly.
by the indirect mechanisms it found the new file as a slightly modified
_copy_ of an old one.

https://codereview.appspot.com/256340043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Provide music functions property-{override,revert,set,unset} (issue 251130043 by d...@gnu.org)

[benko . pal]

that's fabulous, thanks!

https://codereview.appspot.com/251130043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: freetype.cc: solve algebra puzzle for converting quadratics to cubics (issue 252810043 by d...@gnu.org)

[benko . pal]

LGTM

https://codereview.appspot.com/252810043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Redesign listeners using templates (issue 235790043 by d...@gnu.org)

[benko . pal]

LGTM

https://codereview.appspot.com/235790043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Redesign listeners using templates (issue 235790043 by d...@gnu.org)

[benko . pal]

looks exciting to me; my only comment is about a comment


https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh
File lily/include/listener.hh (right):

https://codereview.appspot.com/235790043/diff/1/lily/include/listener.hh#newcode27
lily/include/listener.hh:27: register a method as an event handler in a
dispatcher, then you
this whole comment is probably to be deleted; IIUC the my_listen example
below replaces it.

https://codereview.appspot.com/235790043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 4212: fix out-of-bounds index in division_maior() (issue 189420043 by nine.fierce.ball...@gmail.com)

[benko . pal]

On 2015/01/02 08:56:03, dak wrote:

On 2015/01/02 07:21:19, benko.pal wrote:
 On 2015/01/01 23:08:56, Dan Eble wrote:
  On 2015/01/01 22:57:31, benko.pal wrote:
  

https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc

   File lily/breathing-sign.cc (right):
  
  
 



https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcode122

 lily/breathing-sign.cc:122: if (ydim[DOWN]  val  line_pos.begin

()  it - 1)

 I'd rather write
 line_pos.begin () + 1  it
 but good catch anyway, thanks!



Actually, all of those checks (existing and proposed) look like

undefined

behavior since they calculate a possibly non-existing iterator and

compare with

it.  With most compilers and implementations things will probably

work, but one

can't really rely on it.  In particular, the compiler is allowed to

make

line_pos.begin ()  whatever the same as line_pos.begin () !=

whatever.

it - 1 was used above as it[-1]; it exists, because ydim is a positive
range
(last if) strictly smaller than the whole staff (widening by negative
amount),
so it must be larger than begin, as asserted.

https://codereview.appspot.com/189420043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 4212: fix out-of-bounds index in division_maior() (issue 189420043 by nine.fierce.ball...@gmail.com)

[benko . pal]

On 2015/01/01 23:08:56, Dan Eble wrote:

On 2015/01/01 22:57:31, benko.pal wrote:


https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc

 File lily/breathing-sign.cc (right):




https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcode122

 lily/breathing-sign.cc:122: if (ydim[DOWN]  val  line_pos.begin

()  it -

1)
 I'd rather write
 line_pos.begin () + 1  it
 but good catch anyway, thanks!



I also find the whole surrounding code difficult to read, but I don't

want to

spend any more time on this than I have to.  Thanks for the feedback.


your code is fine, I withdraw my suggestion (I should never hurry a
review).
thanks again!

https://codereview.appspot.com/189420043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 4212: fix out-of-bounds index in division_maior() (issue 189420043 by nine.fierce.ball...@gmail.com)

[benko . pal]

https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc
File lily/breathing-sign.cc (right):

https://codereview.appspot.com/189420043/diff/1/lily/breathing-sign.cc#newcode122
lily/breathing-sign.cc:122: if (ydim[DOWN]  val  line_pos.begin () 
it - 1)
I'd rather write
line_pos.begin () + 1  it
but good catch anyway, thanks!

https://codereview.appspot.com/189420043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3286: add single-C time signature style (issue 164830043 by nine.fierce.ball...@gmail.com)

[benko . pal]

I think we don't want 4/8 displayed as C by default, neither 2/4 as cut
C.
(I also feel uncomfortable to enter \time 2/1 for the Bach Kyrie or
Gratias.)

https://codereview.appspot.com/164830043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3286: add single-C time signature style (issue 164830043 by nine.fierce.ball...@gmail.com)

[benko . pal]

On 2014/10/26 17:24:01, Dan Eble wrote:

On 2014/10/26 07:40:58, benko.pal wrote:
 I think we don't want 4/8 displayed as C by default



Then this change should not bother you because this new style is not

the

default.


ah, indeed, sorry for the noise.

https://codereview.appspot.com/164830043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Issue 3957: Add `make-countdown-announcement.sh'. (issue 109000046 by markpole...@gmail.com)

[benko . pal]

https://codereview.appspot.com/10946/diff/150001/scripts/auxiliar/make-countdown-announcement.sh
File scripts/auxiliar/make-countdown-announcement.sh (right):

https://codereview.appspot.com/10946/diff/150001/scripts/auxiliar/make-countdown-announcement.sh#newcode22
scripts/auxiliar/make-countdown-announcement.sh:22: [ $2 ] 
PATCH_MEISTER=$2
PATCH_MEISTER=${2:-The Patch Meister}

https://codereview.appspot.com/10946/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3783: track default tremolo type in the parser (issue 50400044)

[benko . pal]

LGTM

https://codereview.appspot.com/50400044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

splitting file movement and whitespace-only changes into separate commits (issue 42330044)

[benko . pal]

Reviewers: ,

Message:
continuation of
http://codereview.appspot.com/38530043
belonging to
http://code.google.com/p/lilypond/issues/detail?id=3705

if requested, I can either update dev/janek/metafont-cleanup or push as
a new branch.

Description:
splitting file movement and whitespace-only changes into separate
commits

the commits now read (in reverse chronological order):

unify formatting and indentation


-8
include subfonts unconditionally


-8
remove spurious else


-8
make comments more uniform


-8
factor out common inclusions and initializations


-8
factor out autometric_parameter declarations


-8
move brace-drawing procedure to separate file (and rename files)


-8
rename feta-alphabet to feta-alphabet-generic

to conform to the convention

-8
rename feta-generic to feta-other-generic; similarly parmesan-generic

these files are not for the whole font,
but just some glyphs that aren't in their own subfonts

-8
remove feta-test-generic.mf and other testing files

they were just polluting the code

-8
font: clean up staffline-display in testing mode

Previous testing code was a mix of hideous copypaste with
non-obvious passing glyph outlines around, which attempted
to produce additional glyphs in testing mode, so that there
would be both on-staffline and on-staffspace variations.

That code was completely unreadable and unmaintainable.

Instead i've introduced a global variable that determines
how the stafflines will be printed relative to the glyphs.
To see alternative configuration, just change that value.

-8
font: rename draw_staff to draw_staff_if_debugging

Previous name was confusing: the stafflines are actually
not drawn unless 'test' is set to non-zero value.

-8
feta accidentals: split into several files


-8
omit filename extension as in the rest of the file

Please review this at https://codereview.appspot.com/42330044/

Affected files (+1473, -2485 lines):
  A mf/common-modules-and-initialization.mf
  A mf/debugging-settings.mf
  A mf/declare-autometric-parameters.mf
  M mf/feta-accidentals.mf
  A mf/feta-alphabet-generic.mf
  D mf/feta-alphabet.mf
  M mf/feta-alphabet11.mf
  M mf/feta-alphabet13.mf
  M mf/feta-alphabet14.mf
  M mf/feta-alphabet16.mf
  M mf/feta-alphabet18.mf
  M mf/feta-alphabet20.mf
  M mf/feta-alphabet23.mf
  M mf/feta-alphabet26.mf
  A mf/feta-arrow.mf
  M mf/feta-braces-a.mf
  M mf/feta-braces-b.mf
  M mf/feta-braces-c.mf
  M mf/feta-braces-d.mf
  M mf/feta-braces-e.mf
  M mf/feta-braces-f.mf
  M mf/feta-braces-g.mf
  A mf/feta-braces-generic.mf
  M mf/feta-braces-h.mf
  M mf/feta-braces-i.mf
  M mf/feta-braces.mf
  M mf/feta-clefs.mf
  M mf/feta-flags-generic.mf
  M mf/feta-flags11.mf
  M mf/feta-flags13.mf
  M mf/feta-flags14.mf
  M mf/feta-flags16.mf
  M mf/feta-flags18.mf
  M mf/feta-flags20.mf
  M mf/feta-flags23.mf
  M mf/feta-flags26.mf
  A mf/feta-flats.mf
  D mf/feta-generic.mf
  M mf/feta-macros.mf
  A mf/feta-naturals.mf
  M mf/feta-noteheads-generic.mf
  M mf/feta-noteheads.mf
  M mf/feta-noteheads11.mf
  M mf/feta-noteheads13.mf
  M mf/feta-noteheads14.mf
  M mf/feta-noteheads16.mf
  M mf/feta-noteheads18.mf
  M mf/feta-noteheads20.mf
  M mf/feta-noteheads23.mf
  M mf/feta-noteheads26.mf
  A mf/feta-other-generic.mf
  A mf/feta-parenthesis.mf
  M mf/feta-rests.mf
  M mf/feta-scripts.mf
  A mf/feta-sharps.mf
  D mf/feta-test-generic.mf
  D mf/feta-test11.mf
  D mf/feta-test13.mf
  D mf/feta-test16.mf
  D mf/feta-test20.mf
  D mf/feta-test23.mf
  D mf/feta-test26.mf
  M mf/feta-timesignatures.mf
  M mf/feta11.mf
  M mf/feta13.mf
  M mf/feta14.mf
  M mf/feta16.mf
  M mf/feta18.mf
  M mf/feta20.mf
  M mf/feta23.mf
  M mf/feta26.mf
  M mf/parmesan-clefs.mf
  D mf/parmesan-generic.mf
  M mf/parmesan-noteheads-generic.mf
  M mf/parmesan-noteheads11.mf
  M mf/parmesan-noteheads13.mf
  M mf/parmesan-noteheads14.mf
  M mf/parmesan-noteheads16.mf
  M mf/parmesan-noteheads18.mf
  M mf/parmesan-noteheads20.mf
  M mf/parmesan-noteheads23.mf
  M mf/parmesan-noteheads26.mf
  A mf/parmesan-other-generic.mf
  M mf/parmesan11.mf
  M mf/parmesan13.mf
  M mf/parmesan14.mf
  M mf/parmesan16.mf
  M mf/parmesan18.mf
  M mf/parmesan20.mf
  M mf/parmesan23.mf
  M mf/parmesan26.mf



___
lilypond-devel mailing list
lilypond-devel@gnu.org

Re: Metafont code cleanup (issue 38530043)

[benko . pal]

 However, if you don't mind, i'd prefer to leave it as is - i have
 _already_ spent about 4 hours cleaning up and rebasing commits to

make

 them somewhat ordered for review, and i'm quite tired.


 I do mind.  this is not the sort of thing that can be done in a
 follow up patch.  If you don't want to do that, I can do the
 only-move-files-in-the-first-commit part, but I can't promise
 I'll finish before the weekend.


continued in
https://codereview.appspot.com/42330044/

https://codereview.appspot.com/38530043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Metafont code cleanup (issue 38530043)

[benko . pal]

However, if you don't mind, i'd prefer to leave it as is - i have
_already_ spent about 4 hours cleaning up and rebasing commits to make
them somewhat ordered for review, and i'm quite tired.


I do mind.  this is not the sort of thing that can be done in a
follow up patch.  If you don't want to do that, I can do the
only-move-files-in-the-first-commit part, but I can't promise
I'll finish before the weekend.

p

https://codereview.appspot.com/38530043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Completion_*_engraver: add means to preserve scale factor; issue 3650 (issue 35370043)

[benko . pal]

LSTGM

https://codereview.appspot.com/35370043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Completion_*_engraver: add means to preserve scale factor; issue 3650 (issue 35370043)

[benko . pal]

LGTM

https://codereview.appspot.com/35370043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Support articulations, slurs and breaths in MIDI (issue 26470047)

[benko . pal]

I would need, however, an appropriate new name for the property, and

the best I

can think of now is 'strength'.


'attack'?

https://codereview.appspot.com/26470047/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Isolated durations in music sequences now stand for unpitched notes (issue 22120043)

[benko . pal]

On 2013/11/12 10:23:56, dak wrote:

a) we don't commit ourselves either way: it is undefined what a pure

duration

after an intervening chord will do.



b) just single pitches, no chords: that's what this patch proposes.



c) also chords: this patch needs more work, supporting code must be

written,

Pitch_squash_engraver has to be changed etc etc.



The problem is that with both options b) and c), we'll soonish have

music

depending on this behavior.



I'm leaning towards a): tell people not to rely on particular behavior

after a

chord for now.


how does a) differ from b) patch-wise?

https://codereview.appspot.com/22120043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Isolated durations in music sequences now stand for unpitched notes (issue 22120043)

[benko . pal]

https://codereview.appspot.com/22120043/diff/40001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/22120043/diff/40001/scm/music-functions.scm#newcode780
scm/music-functions.scm:780: ;;; that don't interpret them is harmless.
does that mean that

c e2. 4.

is turned into

c e2. e4.

?  while I actually like not copying articulation, not copying chords
make the new feature (which I'm enthusiastic about) a bit less useful,
e.g. writing tied chords as

c e2. ~ 4.

would really be neat, making it more similar to the shorthand when
duration is omitted.

https://codereview.appspot.com/22120043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3560: Completion_heads_engraver with \scaleDurations (issue 14268043)

[benko . pal]

basically LGTM


https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc
File lily/completion-note-heads-engraver.cc (right):

https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc#newcode200
lily/completion-note-heads-engraver.cc:200: factor_ =
robust_scm2rational (factor, 0);
1 would be a more robust default

https://codereview.appspot.com/14268043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3560: Completion_heads_engraver with \scaleDurations (issue 14268043)

[benko . pal]

LGTM

https://codereview.appspot.com/14268043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Let Skyline's copy constructor use whole-sale copy construction of members (issue 12747043)

[benko . pal]

LGTM, but then why not let the compiler generate the copy constructor?

https://codereview.appspot.com/12747043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: follow up issue 2470: make Completion_rest_engraver aware of completionUnit (issue 10100043)

[benko . pal]

On 2013/06/12 07:48:50, dak wrote:

On 2013/06/06 21:19:04, benko.pal wrote:
 follow up issue 3402 too



The delta from patch set 1 to patch set 2 looks decidedly strange.  It

looks

like deleting most of the file.


um, yes.  strange.  the unified diff is as intended.

https://codereview.appspot.com/10100043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Change \transpose to \relative in ancient.itely (issue 7538043)

[benko . pal]

some white mensural examples change, but that doesn't matter (if anybody
prefers not changing an example, I can fix those).  I hope Greogrian
examples don't change - I don't know whether it would matter.


https://codereview.appspot.com/7538043/diff/5001/Documentation/notation/ancient.itely
File Documentation/notation/ancient.itely (right):

https://codereview.appspot.com/7538043/diff/5001/Documentation/notation/ancient.itely#newcode952
Documentation/notation/ancient.itely:952: @c @end example
please make the two examples above and below match

https://codereview.appspot.com/7538043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Change \transpose to \relative in ancient.itely (issue 7538043)

[benko . pal]

https://codereview.appspot.com/7538043/diff/5001/Documentation/notation/ancient.itely
File Documentation/notation/ancient.itely (right):

https://codereview.appspot.com/7538043/diff/5001/Documentation/notation/ancient.itely#newcode952
Documentation/notation/ancient.itely:952: @c @end example
On 2013/03/07 21:16:18, aleksandr.andreev wrote:

On 2013/03/07 19:04:52, benko.pal wrote:
 please make the two examples above and below match



Sorry, I am confused which examples you are referring to. The above

example is

commented out. Should we eliminate it completely?


sorry, you are right.  I mistook @c for code.  please ignore me.

https://codereview.appspot.com/7538043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Web: Easier Editing - added bwwtolily (issue 7098055)

[benko . pal]

bwwtolilly is a typo, isn't it?
(diff view doesn't work for some reason.)

https://codereview.appspot.com/7098055/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Updates ancient clefs (issue 7180043)

[benko . pal]

LGTM

https://codereview.appspot.com/7180043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

CG: explicitly detail the correct values for git cl config (issue 7096052)

[benko . pal]

LGTM; not used by LilyPond is really superior to you don't need to
understand!

https://codereview.appspot.com/7096052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Better staggering of accidental placements. (issue 7101045)

[benko . pal]

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):

https://codereview.appspot.com/7101045/diff/1/lily/accidental-placement.cc#newcode210
lily/accidental-placement.cc:210: int parity = 1;
could you use bool (or in an improved version more values will be used)?

https://codereview.appspot.com/7101045/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Create a \tuplet function to complement \times and tupletSpannerDuration (issue 7058068)

[benko . pal]

good for a start but I have some problems with the duration parameter:

1.
{
  \tuplet 3/2 { c8 c c c c c }
  \tuplet 3/2 4 { c8 c c c c c }
  \tuplet 3/2 { c8 c c c c c }
}

I found it ugly that the first and last input line are engraved
differently (i.e. duration has an effect outside of its tuplet);

2. it works funny in scaled music (e.g. nested tuplets):

  \tuplet 3/2 { c4 \tuplet 3/2 4 { c8 c c c c c } }


https://codereview.appspot.com/7058068/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: PO: remove duplicates entries for hh and cc from ALL_PO_SOURCES (issue 7029043)

[benko . pal]

LGTM

https://codereview.appspot.com/7029043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Rewrite of Midi.c in Python (issue 7016046)

[benko . pal]

SGTM

python/midi.c should be removed (this may involve some make changes)
midi files are seen neither in the web interface, nor in the downloaded
patch.

https://codereview.appspot.com/7016046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: canonicalise notional octave of tonic (issue 7019045)

[benko . pal]

LGTM

https://codereview.appspot.com/7019045/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 3049: Parser outputs Lyric events for illegal note names (issue 7017044)

[benko . pal]

LGTM

https://codereview.appspot.com/7017044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Changes lilypond-example for rest-by-number (issue 6924053)

[benko . pal]

LGTM

https://codereview.appspot.com/6924053/

___
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)

[benko . pal]

LGTM


https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-rest-styles.ly
File input/regression/markup-rest-styles.ly (right):

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-rest-styles.ly#newcode18
input/regression/markup-rest-styles.ly:18: (symbol-string style))
sorry for nitpicking, but please don't use tabs.  if nothing else is to
be done, I'll be happy to do the formatting.

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-rest.ly
File input/regression/markup-rest.ly (right):

https://codereview.appspot.com/6850073/diff/17001/input/regression/markup-rest.ly#newcode30
input/regression/markup-rest.ly:30: (number-string (expt 2 duration))
more formatting nitpicking: do we have a line length limit?

https://codereview.appspot.com/6850073/

___
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)

[benko . pal]

LGTM, thanks!

https://codereview.appspot.com/6850073/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Use define-void-function rather than define-music-function in several places (issue 6854104)

[benko . pal]

LGTM

https://codereview.appspot.com/6854104/

___
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)

[benko . pal]

On 2012/11/29 00:41:14, thomasmorley65 wrote:

On 2012/11/27 20:03:06, benko.pal wrote:



  Did you had a look on the compiled output of the new reg-tests?


I did now, it's OK with me.


https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-styles.ly
File input/regression/markup-rest-styles.ly (right):

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-styles.ly#newcode25
input/regression/markup-rest-styles.ly:25: '(-3 -2 -1 0 1 2 3 4 5 6
7
please fix indentation, this and the next list are at quite different
levels.  similarly in the other .ly file.

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly
File input/regression/markup-rest.ly (right):

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly#newcode27
input/regression/markup-rest.ly:27: (if (= duration 0) dots )
this if looks superfluous

https://codereview.appspot.com/6850073/

___
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)

[benko . pal]

On 2012/11/26 21:11:53, thomasmorley65 wrote:

On 2012/11/26 15:09:58, benko.pal wrote:


http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm

 File scm/define-markup-commands.scm (right):




http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm#newcode3251

 scm/define-markup-commands.scm:3251: (or multi-measure-rest ( log

1)))

 there are no Petrucci rest glyphs at all; use mensural without any

further

 condition.  or don't even allow petrucci as style for these markups.



Well, writing this conditions I tried to orientate myself on

`select-head-glyph´

from output-lib.scm
`select-head-glyph´ returns note-head-glyphs for some styles without

defined

note-head-glyphs as well. Mostly if ( log 1)
So I tried to transfer this behaviour to rests.


actually if ( log 0).  all this is driven by the actual layout of the
Feta font (see NR A.8): petrucci has its own notehead glyphs if ( log
-1), but it has no rest glyphs at all.


Did you had a look on the compiled output of the new reg-tests?


no.  could you push to a dev/ branch?


This output is what I tried to achieve.
You'll see that for some styles mensural- or neomensural-glyphs are

taken,

others are defaulting.



The main question is:
If there are no defined glyphs for a specific style,
should I try to select others (currently tried)
or
should I return default-glyphs (I suspect this would be much easier)?



Opinions?


I don't exactly know what is style and how is it set.  I thought it's a
local property of your new command and must be set explicitly by the
user; in this case you can either declare that petrucci is an invalid
style or make sure that setting petrucci is read as setting mensural.
in other words, translate petrucci to mensural once and for all in the
beginning.



http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm#newcode3299

 scm/define-markup-commands.scm:3299: ;; If there is a ledger, move

the dots in

 X-direction to avoid collision.
 is this comment true?  I can't see how the condition below checks

for ledger.


You're right, the comment is misleading.
I selected ledgered glyphs for whole and half rests for all styles,

apart from

the listed ones.
Depending on the decision which glyphs are taken (see above), I'll

change the

comment and/or the code.


I don't know whether it's relevant or not, but (in the default style)
there's a ledgered variant for breve rest too (though not for longa).

http://codereview.appspot.com/6850073/
___
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)

[benko . pal]

On 2012/11/25 23:03:43, thomasmorley65 wrote:

On 2012/11/21 08:05:09, benko.pal wrote:
[...]
 there are no separate glyphs for rests and multi measure rests: the

M in glyph

 names stands not for MultiMeasure, but for Minus.



Didn't know that.



 that makes for rewriting the
 cond below, I'm afraid, but it will be simpler.



Well, I want the markup-commands to produce different output depending

on the

used style. I can't see an other way, than to write detailed

conditions.

OTOH, I tried to consider your hint and to write them concise.


I just meant that instead of checking several times whether dealing with
multi-measure rest or not, you may convert duration log at a single
place (with the caveat of turning a negative sign to 'M' instead of
'-').

http://codereview.appspot.com/6850073/

___
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)

[benko . pal]

So I'd suggest not treating the '-' at all (to make things not more
confusing than they already are) and letting the font backend sort
things out.


I concur.


http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm#newcode3251
scm/define-markup-commands.scm:3251: (or multi-measure-rest ( log 1)))
there are no Petrucci rest glyphs at all; use mensural without any
further condition.  or don't even allow petrucci as style for these
markups.

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm#newcode3299
scm/define-markup-commands.scm:3299: ;; If there is a ledger, move the
dots in X-direction to avoid collision.
is this comment true?  I can't see how the condition below checks for
ledger.

http://codereview.appspot.com/6850073/diff/6004/scm/define-markup-commands.scm#newcode3305
scm/define-markup-commands.scm:3305: (and (= log 1) (equal? style
'petrucci
this check looks fishy, as there are no Petrucci-style rest glyphs.

http://codereview.appspot.com/6850073/

___
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)

[benko . pal]

http://codereview.appspot.com/6850073/diff/9001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/6850073/diff/9001/scm/define-markup-commands.scm#newcode3247
scm/define-markup-commands.scm:3247: M
there are no separate glyphs for rests and multi measure rests: the M in
glyph names stands not for MultiMeasure, but for Minus.  that makes for
rewriting the cond below, I'm afraid, but it will be simpler.

http://codereview.appspot.com/6850073/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Fix and document usage of `convert-ly - test.ly'. (issue 6846083)

[benko . pal]

LGTM

http://codereview.appspot.com/6846083/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Allow music of nominally zero duration to be typeset. (issue 6810087)

[benko . pal]

On 2012/11/16 22:57:29, dak wrote:

On 2012/11/16 22:13:59, benko.pal wrote:
 LGTM in the sense that it won't make things worse; I've tried to

understand

 the code but failed, see below.



I have not actually tried to understand the code.  I just added checks

for

existing array elements before access until I could no longer make

LilyPond

segfault or produce programming errors.



So this is, indeed, strictly a patch on the won't make things worse

basis,

except for the one, initial removal of a zero-duration check,

necessary for sane

behavior in things like incipits without notes.


yes, the commit message made me guess so; I hoped someone (Joe?) would
chime in.

L still GTM

http://codereview.appspot.com/6810087/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Allow music of nominally zero duration to be typeset. (issue 6810087)

[benko . pal]

LGTM in the sense that it won't make things worse; I've tried to
understand the code but failed, see below.


http://codereview.appspot.com/6810087/diff/2001/lily/simple-spacer.cc
File lily/simple-spacer.cc (right):

http://codereview.appspot.com/6810087/diff/2001/lily/simple-spacer.cc#newcode375
lily/simple-spacer.cc:375: Grob *end_col = dynamic_castItem *
(cols[col_index + 1])-find_prebroken_piece (LEFT);
is there anybody out there understanding this code?  shouldn't next_col
be used instead of cols[col_index + 1] (and this whole block put under
the if (next_col)?)

http://codereview.appspot.com/6810087/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: new syntax for \tweak, \override (2936) (issue 6852052)

[benko . pal]

LGTM


http://codereview.appspot.com/6852052/diff/1/Documentation/notation/vocal.itely
File Documentation/notation/vocal.itely (left):

http://codereview.appspot.com/6852052/diff/1/Documentation/notation/vocal.itely#oldcode170
Documentation/notation/vocal.itely:170:
I'm sort of sorry to see this funny paragraph disappear :)
earnest: this (and some other examples) may well be worth mentioning in
Changes.

http://codereview.appspot.com/6852052/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: Add example of extending glissandi over repeats (2591) (issue 6814115)

[benko . pal]

LGTM by reading only

http://codereview.appspot.com/6814115/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: fix representation switching from line-position to staff-space (issue 6778050)

[benko . pal]

Reviewers: Keith, dak,


http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc
File lily/breathing-sign.cc (left):

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc#oldcode88
lily/breathing-sign.cc:88: int const int_dim = (int) ydim[i];
On 2012/10/27 19:36:21, Keith wrote:

If you want to avoid ending the divisi at a fractional position, maybe

just

ydim[i] = int_dim;


this juggling was done only because Symbol_referencer::on_staff_line
takes an int, whereas the staff positions are Real's.

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc
File lily/breathing-sign.cc (right):

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc#newcode99
lily/breathing-sign.cc:99: standard algorithms are suitable to find the
upper line of
On 2012/10/27 19:51:49, dak wrote:

Do we really need a complicated variable line arrangement algorithm

here?

I want staves with line-positions like (-2 0 2 4) work.  I could arrange
for this a bit simpler, but the moral I drew from the line-count saga is
that from time to time comes a user with tablature or line-positions (-4
4) or (-4 0 4)  (I know, they are not using \divisioMaior, only repeat
signs, bar lines, slurs, time signatures, etc.), and if I can cater for
them, then I should do it.


Nobody complained before this was fixed 2.15-ish.


I apologise, my original fix is plain silly.


It really looks like an overly complex solution to a self-made

problem.

of course I wouldn't mind anybody coming up with a more elegant
solution, but the original code relying on the line-count property is
unacceptable for me (not here, but at enough other places to make me
hunt these down).


I think that for most of those problems, the very old code looking

only at the

line-count property (even when it was overruled via line-positions for

special

effects) was producing exactly what people expected and wanted.


I started this series about a year ago because I couldn't make the old
code produce what I wanted.  see also your comment in
http://code.google.com/p/lilypond/issues/detail?id=2783#c35
(BTW let me note that I'm working on rests as hinted at in comment 34.)


All that magic and second-guessing does not appear to have resulted in

an

improvement for actual scores.  Not in results, and most certainly not

in

predictability.


if common staves are not predictable, it's a bug and I'm willing to
work on it.

having said all this, I don't mind if this particular patch is rejected.

http://codereview.appspot.com/6778050/diff/1001/lily/breathing-sign.cc#newcode112
lily/breathing-sign.cc:112: assert (line_pos.begin ()  it);
On 2012/10/27 19:36:21, Keith wrote:

Can you assert this here, and similarly a few lines down?   If someone

uses a

collapsed staff with all line-positions equal, I would have thought

that

lower_bound() would return a pointer to the first line-position, and

similarly

upper_bound() a pointer to one beyond the last line-position.


you are right; what's more, assertions are futile in such cases, this
condition must be checked (and perhaps I'll just do nothing).

Description:
fix representation switching from line-position to staff-space

Please review this at http://codereview.appspot.com/6778050/

Affected files:
  M input/regression/breathing-sign-ancient.ly
  M lily/breathing-sign.cc



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: prevent collision of ligatures and next note (issue 6740046)

[benko . pal]

hi Janek,


Pal, could you add some comments and a more descriptive commit

message?  I see

the code, and it seems to make sense, but i don't understand the

why's.  For

example, why the parameters should be passed in a different way?


in C++ there should be a good reason to pass complex structures like
std::vectorGrob_info by value, not by reference to const; in this case
there's no such reason, pass-by-reference works perfectly.


I also think that it'd be a good idea to add a regtest, even if it's

imperfect

(just write a comment inside it, so that everyone envestigating it

will be

warned that some unexpected results may occur).


the problem is that I really can't predict _when_ unexpected results can
and when can't occur.  I tried to come up with the Tiniest example and
I'm sure my example in the attachment to
http://code.google.com/p/lilypond/issues/detail?id=2914
is not the tiniest possible that demonstrates the problem.  put
otherwise, working all right with previous versions is just as
unexpected for me as producing a collision.


http://codereview.appspot.com/6740046/diff/1/lily/mensural-ligature-engraver.cc

File lily/mensural-ligature-engraver.cc (right):



http://codereview.appspot.com/6740046/diff/1/lily/mensural-ligature-engraver.cc#newcode428

lily/mensural-ligature-engraver.cc:428: if (size_t const dot_count =
Rhythmic_head::dot_count (current))
Is this related to the ligature collision problem or just a by-the-way

fix?

it _was_ related (see usage of dot_count lower); I hope the new
organization is cleaner.

p

http://codereview.appspot.com/6740046/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

prevent collision of ligatures and next note (issue 6740046)

[benko . pal]

Reviewers: ,

Message:
mensural-ligatures.ly is listed as changed - that is not surprising,
although I couldn't tell in advance _how_ it will change.  what is a bit
more surprising that I can't tell even now, having seen the regtest
comparison.

Description:
prevent collision of ligatures and next note

Please review this at http://codereview.appspot.com/6740046/

Affected files:
  M lily/mensural-ligature-engraver.cc
  M scm/define-grobs.scm


Index: lily/mensural-ligature-engraver.cc
diff --git a/lily/mensural-ligature-engraver.cc  
b/lily/mensural-ligature-engraver.cc
index  
35b8614b0d62fa2a657510c68b9ac3fc35e37144..848ca0cb4b003a11a0e3cc63cbfe78f1065d7a0a  
100644

--- a/lily/mensural-ligature-engraver.cc
+++ b/lily/mensural-ligature-engraver.cc
@@ -64,9 +64,11 @@ public:
   TRANSLATOR_DECLARATIONS (Mensural_ligature_engraver);

 private:
-  void transform_heads (vectorGrob_info primitives);
-  void propagate_properties (Spanner *ligature, vectorGrob_info  
primitives);

-  void fold_up_primitives (vectorGrob_info primitives);
+  void transform_heads (vectorGrob_info const primitives);
+  void propagate_properties (Spanner *ligature,
+ vectorGrob_info const primitives);
+  void fold_up_primitives (Spanner *ligature,
+   vectorGrob_info const primitives);
 };

 IMPLEMENT_TRANSLATOR_LISTENER (Mensural_ligature_engraver, ligature);
@@ -89,7 +91,7 @@ Mensural_ligature_engraver::create_ligature_spanner ()
 }

 void
-Mensural_ligature_engraver::transform_heads (vectorGrob_info primitives)
+Mensural_ligature_engraver::transform_heads (vectorGrob_info const  
primitives)

 {
   if (primitives.size ()  2)
 {
@@ -336,7 +338,7 @@ Mensural_ligature_engraver::transform_heads  
(vectorGrob_info primitives)

  */
 void
 Mensural_ligature_engraver::propagate_properties (Spanner *ligature,
-  vectorGrob_info  
primitives)
+  vectorGrob_info const  
primitives)

 {
   Real thickness
 = robust_scm2double (ligature-get_property (thickness), 1.3);
@@ -349,6 +351,7 @@ Mensural_ligature_engraver::propagate_properties  
(Spanner *ligature,

   Real maxima_head_width
 = Font_interface::get_default_font (ligature)-
   find_by_name (noteheads.sM3ligmensural).extent (X_AXIS).length ();
+  Real min_length = 0.0;

   Item *prev_primitive = NULL;
   for (vsize i = 0; i  primitives.size (); i++)
@@ -362,9 +365,11 @@ Mensural_ligature_engraver::propagate_properties  
(Spanner *ligature,

 {
 case MLP_BREVIS:
 case MLP_LONGA:
+  min_length += head_width;
   primitive-set_property (head-width, scm_from_double  
(head_width));

   break;
 case MLP_MAXIMA:
+  min_length += maxima_head_width;
   primitive-set_property (head-width,
scm_from_double (maxima_head_width));
   break;
@@ -377,6 +382,7 @@ Mensural_ligature_engraver::propagate_properties  
(Spanner *ligature,

   {
 SCM flexa_scm = primitive-get_property (flexa-width);
 Real const flexa_width = robust_scm2double (flexa_scm, 2.0);
+min_length += flexa_width + thickness;
 SCM head_width = scm_from_double (0.5 * (flexa_width +  
thickness));

 primitive-set_property (head-width, head_width);
 prev_primitive-set_property (head-width, head_width);
@@ -390,10 +396,13 @@ Mensural_ligature_engraver::propagate_properties  
(Spanner *ligature,


   prev_primitive = primitive;
 }
+
+  ligature-set_property (minimum-length, scm_from_double (min_length));
 }

 void
-Mensural_ligature_engraver::fold_up_primitives (vectorGrob_info  
primitives)

+Mensural_ligature_engraver::fold_up_primitives (Spanner *ligature,
+vectorGrob_info const  
primitives)

 {
   Item *first = 0;
   Real distance = 0.0;
@@ -416,7 +425,7 @@ Mensural_ligature_engraver::fold_up_primitives  
(vectorGrob_info primitives)
   Real head_width = scm_to_double (current-get_property  
(head-width));

   distance += head_width - thickness;

-  if (Rhythmic_head::dot_count (current)  0)
+  if (size_t const dot_count = Rhythmic_head::dot_count (current))
 /*
   Move dots above/behind the ligature.
   dots should also avoid staff lines.
@@ -452,6 +461,12 @@ Mensural_ligature_engraver::fold_up_primitives  
(vectorGrob_info primitives)

   else if (delta == 1 || delta == -1)
 vert_shift -= delta * staff_space;
 }
+  else
+ligature-set_property
+  (minimum-length,
+   scm_from_double
+   (head_width*dot_count + robust_scm2double
+(ligature-get_property (minimum-length), 0.0)));

   dot_gr-translate_axis (vert_shift, Y_AXIS);

@@ -470,7 +485,7 @@ 

Re: Fix extra spacing in Kievan notation (issue 6684051)

[benko . pal]

I don't like that: the patch manipulates a staff based on note head.  it
also increments a hack.

have you tried something like
\override SpacingSpanner #'shortest-duration-space = #0
\override SpacingSpanner #'spacing-increment = #0
?

http://codereview.appspot.com/6684051/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Added \clef treble_8 for guitar harmonics (issue 6588049)

[benko . pal]

LGTM

https://codereview.appspot.com/6588049/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: extend description of glissandi (2844) (issue 6567059)

[benko . pal]

having identical files in several directories looked suspicious to me,
but if that's the way to have it, then LGTM.

https://codereview.appspot.com/6567059/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Doc: extend description of glissandi (2844) (issue 6567059)

[benko . pal]

what about an example like

\afterGrace f1\glissando f'16

?

http://codereview.appspot.com/6567059/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: Improve documentation of \glissando. (issue 6529043)

[benko . pal]

http://codereview.appspot.com/6529043/diff/1/Documentation/notation/expressive.itely
File Documentation/notation/expressive.itely (right):

http://codereview.appspot.com/6529043/diff/1/Documentation/notation/expressive.itely#newcode1074
Documentation/notation/expressive.itely:1074:
@lilypond[verbatim,quote,relative=2,line-width=4.0\cm]
On 2012/09/25 15:31:17, Trevor Daniels wrote:

In this example and the previous one the skipped notes
have durations, so the visible notes in a bar don't
add up to the measure length.  Is this the usual practice?


I'm just singing Xenakis' Nuits, and it's notated this way.


And a second point: it would be better to use \hideNotes
and then make the stem visible, otherwise ledger lines
and dots remain visible.


ugh, I don't have the score at hand; I think I haven't seen ledger
lines; I'm absolutely unsure about dots.  will check tonight.

http://codereview.appspot.com/6529043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: Improve documentation of \glissando. (issue 6529043)

[benko . pal]

http://codereview.appspot.com/6529043/diff/1/Documentation/notation/expressive.itely
File Documentation/notation/expressive.itely (right):

http://codereview.appspot.com/6529043/diff/1/Documentation/notation/expressive.itely#newcode1074
Documentation/notation/expressive.itely:1074:
@lilypond[verbatim,quote,relative=2,line-width=4.0\cm]
http://www.iannis-xenakis.org/partitions/Syrmos.jpg

http://codereview.appspot.com/6529043/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Doc: Improve documentation of \glissando. (issue 6529043)

[benko . pal]

http://codereview.appspot.com/6529043/diff/1/Documentation/notation/expressive.itely
File Documentation/notation/expressive.itely (right):

http://codereview.appspot.com/6529043/diff/1/Documentation/notation/expressive.itely#newcode1059
Documentation/notation/expressive.itely:1059:
@lilypond[verbatim,quote,relative=2]
On 2012/09/25 21:07:06, Trevor Daniels wrote:

This example shows a beam extending from a stem to the
final note of the glissando.  Is this correct, or would
it be better to suppress the beam?


just like ordinary duration display: either a beam or a flag.  Bartók's
String Quartet no. 4. has examples for both.
I find your example fine.  is it mentioned that glissando is not
standardised?  e.g. Bartók wanted all glissandi start immediately, while
others use the headless stem notation, reserving full headed notes to
mean fixed pitch through the whole duration.

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

Re: simplify previous patch-set by special casing 2-line staves (issue 6506090)

[benko . pal]

On 2012/09/14 06:53:47, Keith wrote:

On 2012/09/14 06:16:57, benko.pal wrote:
 main argument: I can't guess why the user chose a
 specific way of manipulating the staff and how (s)he
 interprets it,



If the user writes
 \override #'staff-space = #0.5
he wants to scale things.



If he writes
 \override #'line-positions = #'(-2 -1 0 1 2)
he wants to re-arrange things


perhaps yes, perhaps not (using different weird microtonal thingies).
in this comparison the relevant thing is whether he wants different
repeat signs.  note that, unfortunately, LilyPond has several scaling
possibilities, enabling the user to scale some and not other things.  if
everything (dots included) is scaled, the code under review will do the
right thing; if some is not, then divining the intent is not so easy.

to me the repeat sign is a purely engraving matter.  making it depend on
the musical meaning of the staff is just making it more fragile.
if we want repeat signs in a TabStaff look as requested, then I want
every repeat sign (with identical dot size) in an identically looking
staff (regardless how it's achieved by mixing line-positions,
set-global-staff-size, layout-set-staff-size, line-count, staff-space)
look identical.

http://codereview.appspot.com/6506090/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: change defaults for dot spacing in repeat sign to accommodate tab staves (issue 6488097)

[benko . pal]

I messed up something, the new patchset is at
http://codereview.appspot.com/6506090

http://codereview.appspot.com/6488097/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: change defaults for dot spacing in repeat sign to accommodate tab staves (issue 6488097)

[benko . pal]

sorry, I'm confused.
do we want to support
- NR 2.5.1 style 2-line percussion staves (setting both line-count and
staff-space to 2 instead of setting just line-positions to (-2 2))?
- default TabStaff's (even line-count, 1.5 staff-space)?

if we want to support both of those without changing dot size,
calculations cannot be based on line-positions only, staff-space must be
considered.  if we consider staff-space, issue 2648 must be taken care
of.

http://codereview.appspot.com/6488097/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

remove top-level const's from declarations (issue 6501096)

[benko . pal]

Reviewers: ,

Message:
following up
http://codereview.appspot.com/6477062/#msg5

Description:
remove top-level const's from declarations

Please review this at http://codereview.appspot.com/6501096/

Affected files:
  M flower/include/file-name.hh
  M lily/include/constrained-breaking.hh
  M lily/include/skyline.hh


Index: flower/include/file-name.hh
diff --git a/flower/include/file-name.hh b/flower/include/file-name.hh
index  
3e9c6d5294082689709f2d4dbca6675e02b514c0..b30dd9cd5b6d8f89ccb1e586a5ca3f3a8e595d1c  
100644

--- a/flower/include/file-name.hh
+++ b/flower/include/file-name.hh
@@ -23,7 +23,7 @@
 #include std-vector.hh
 #include std-string.hh

-std::string dir_name (std::string const file_name);
+std::string dir_name (std::string file_name);
 std::string get_working_directory ();

 class File_name
Index: lily/include/constrained-breaking.hh
diff --git a/lily/include/constrained-breaking.hh  
b/lily/include/constrained-breaking.hh
index  
731e20c1e3dca00feab28d8e4ce9ed3af1ffc0fe..cb4c4f70be28a7ac56e72e8e98291b6bf7d62b14  
100644

--- a/lily/include/constrained-breaking.hh
+++ b/lily/include/constrained-breaking.hh
@@ -203,6 +203,6 @@ private:
   Real combine_demerits (Real force, Real prev_force);

   bool calc_subproblem (vsize start, vsize systems, vsize max_break_index);
-  void fill_line_details (Line_details *const, vsize, vsize);
+  void fill_line_details (Line_details *, vsize, vsize);
 };
 #endif /* CONSTRAINED_BREAKING_HH */
Index: lily/include/skyline.hh
diff --git a/lily/include/skyline.hh b/lily/include/skyline.hh
index  
e6cd030627dd746dfcdfee8739590441d768bca9..0cb9fdcc81f5f23474f28c0ac2fbd16b713180aa  
100644

--- a/lily/include/skyline.hh
+++ b/lily/include/skyline.hh
@@ -56,7 +56,7 @@ private:
   Direction sky_;

   void internal_merge_skyline (listBuilding *, listBuilding *,
-   listBuilding *const result) const;
+   listBuilding *result) const;
   listBuilding internal_build_skyline (listBuilding *) const;
   Real internal_distance (Skyline const , Real horizon_padding, Real  
*touch_point) const;

   Real internal_distance (Skyline const , Real *touch_point) const;



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: remove top-level const's from declarations (issue 6501096)

[benko . pal]

results of the following searches were checked manually:
grep -rE -e 'const( +[a-zA-Z0-9_]*)? *, *$' .
grep -rE -e 'const( +[a-zA-Z0-9_]*)?( *,[^;]+)?\) *(const|= *0)? *;' .


http://codereview.appspot.com/6501096/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Removes echoed information from make doc (issue 6496074)

[benko . pal]

LGTM

http://codereview.appspot.com/6496074/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Run fixcc + astyle2.02.1 (issue 6477062)

[benko . pal]

LGTM

http://codereview.appspot.com/6477062/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Run fixcc + astyle2.02.1 (issue 6477062)

[benko . pal]

http://codereview.appspot.com/6477062/diff/1/lily/include/skyline.hh
File lily/include/skyline.hh (right):

http://codereview.appspot.com/6477062/diff/1/lily/include/skyline.hh#newcode58
lily/include/skyline.hh:58: listBuilding *const result);
I'd like to see the const removed (top-level const on function parameter
types are ignored), but that may be the target of another patch.

http://codereview.appspot.com/6477062/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: line_count related patches in a single commit for review (issue 6419064)

[benko . pal]

I have now four commits: regtest changes plus one change
in three (sort of unrelated) functions each in bar-line.scm
(colon, extent and line-span computation).
what review process do you prefer/suggest?
one review per commit or review in one, push in four?


uploaded in a single commit for review:
http://codereview.appspot.com/6441166
http://code.google.com/p/lilypond/issues/detail?id=2762

http://codereview.appspot.com/6419064/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

issue 2533 continued: fix problematic uses of line-count in bar-line (issue 6441166)

[benko . pal]

Reviewers: marc, Keith, dak,

Message:
all changes in a single commit; I'm happy to resubmit it in four; I can
also push (after review and coutdown) in one or four or as you wish.

Description:
issue 2533 continued: fix problematic uses of line-count in bar-line

- modify regtests to show when to put the dots of a repeat sign
  inside or outside the staff
- make same dots
  - be centred on the staff centre,
  - avoid staff lines,
  - be far enough when there are no staff lines
- extend or shrink bar line if it's too short or long

Please review this at http://codereview.appspot.com/6441166/

Affected files:
  M input/regression/repeat-sign-global-size-10.ly
  M input/regression/repeat-sign-global-size-30.ly
  M input/regression/repeat-sign-layout-size.ly
  M scm/bar-line.scm



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: line_count related patches in a single commit for review (issue 6419064)

[benko . pal]

the post-1320 version.  Marc, please consider patch set 2 for bar-line
related changes.

http://codereview.appspot.com/6419064/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Issue 1320: Scheme bar line interface (issue 6305115)

[benko . pal]

Marc, please don't throw the whole 2533 issue stuff out; look at the
latest version at
http://codereview.appspot.com/6431044
in particular bar-line.cc and repeat-sign.ly.

I really don't mind if your patch goes before mine (it would even be
better - why push a change to a file that is to be thrown out by the
next commit), but I do mind that (some of) those cases work right, and
unfortunately I'm far less fluent in Scheme than in C++.

http://codereview.appspot.com/6305115/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: line_count related patches in a single commit for review (issue 6419064)

[benko . pal]

regtests reorganized and explanations added

http://codereview.appspot.com/6419064/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: fix repeat-sign problems introduced with issue 2533 (issue 6431044)

[benko . pal]

sorry, after juggling with git branches the new version is at
http://codereview.appspot.com/6419064

http://codereview.appspot.com/6431044/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

line_count related patches in a single commit for review (issue 6419064)

[benko . pal]

Reviewers: Keith, dak, marc,

Message:
when splitting the previous patch into smaller commits I added new
regtests and found further errors with bar-line.
to fix those there are some changes from the previous version:
- calc_bar_extent now not only shrinks the bar line, but can expand it
for narrow (e.g. single-line) staves
- compound_barline is restructured

Description:
line_count related patches in a single commit for review

in decreasing order of importance:
- make slurs symmetric to staff
- improve time signature and bar line (in particular dots of a repeat
sign)
  positioning under non-standard circumstances
- enhance regtests
- etc.

Please review this at http://codereview.appspot.com/6419064/

Affected files:
  M input/regression/ledger-lines-varying-staves.ly
  M input/regression/non-centered-bar-lines.ly
  A input/regression/repeat-sign-global-size-10.ly
  A input/regression/repeat-sign-global-size-30.ly
  A input/regression/repeat-sign-layout-size.ly
  A input/regression/repeat-sign.ly
  M input/regression/staff-ledger-positions.ly
  M input/regression/staff-line-positions.ly
  M lily/bar-line.cc
  M lily/beam.cc
  M lily/breathing-sign.cc
  M lily/custos.cc
  M lily/rest-collision.cc
  M lily/rest.cc
  M lily/slur-scoring.cc
  M lily/time-signature.cc
  M lily/vaticana-ligature.cc



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

fix repeat-sign problems introduced with issue 2533 (issue 6431044)

[benko . pal]

Reviewers: ,

Message:
patch enhanced to fix
http://code.google.com/p/lilypond/issues/detail?id=2648

if anyone wants to run regtests, please run both versions on the new
regtests.

I'd be happy to split the patch into several commits.

Description:
fix repeat-sign problems introduced with issue 2533

This reverts commit d05afb4c0bf29cbd6a98691c9c62afa1604fa4b2,
i.e. applies (a modified version of)
2a872121379fffe7b1cd5d23048b7ea04b4d1f68.
modifications relative to the latter are
- dot size is queried (not hardcoded) when printing a repeat sign
- bar line extent is shrunk not by scaling but cutting
- new regtests for repeat sign in exotic staves
  (non-default staff size, line count, line positions)

Please review this at http://codereview.appspot.com/6431044/

Affected files:
  M input/regression/ledger-lines-varying-staves.ly
  M input/regression/non-centered-bar-lines.ly
  A input/regression/repeat-sign-global-size-10.ly
  A input/regression/repeat-sign-global-size-30.ly
  A input/regression/repeat-sign-layout-size.ly
  A input/regression/repeat-sign.ly
  M input/regression/staff-ledger-positions.ly
  M input/regression/staff-line-positions.ly
  M input/regression/zero-staff-space.ly
  M lily/bar-line.cc
  M lily/beam.cc
  M lily/breathing-sign.cc
  M lily/custos.cc
  M lily/rest-collision.cc
  M lily/rest.cc
  M lily/slur-scoring.cc
  M lily/time-signature.cc
  M lily/vaticana-ligature.cc



___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

make beams in 6/4, 9/4 and 12/4 behave like those in 3/4 (issue 6389044)

[benko . pal]

Reviewers: ,

Message:
note that the newly added regtest shows some cases I'm not really sure
about: e.g. beaming/breaking of six eigth (or a4 a8 a8 a4) may have to
depend on whether it's a triplet or sextuplet.

Description:
make beams in 6/4, 9/4 and 12/4 behave like those in 3/4

Please review this at http://codereview.appspot.com/6389044/

Affected files:
  A input/regression/autobeam-default-tuplets.ly
  M input/regression/autobeam-show-defaults.ly
  M scm/time-signature-settings.scm


Index: input/regression/autobeam-default-tuplets.ly
diff --git a/input/regression/autobeam-default-tuplets.ly  
b/input/regression/autobeam-default-tuplets.ly

new file mode 100644
index  
..44a77794281262e167842290a5a8949e1ff72e8b

--- /dev/null
+++ b/input/regression/autobeam-default-tuplets.ly
@@ -0,0 +1,40 @@
+\version 2.15.40
+
+\header {
+
+  texidoc = 
+Default autobeam settings for tuplets
+
+
+}
+
+{
+  \time 2/2
+  \times 4/5 { \repeat unfold 5 a8 }
+  \times 8/10 { \repeat unfold 10 a16 } |
+  \times 4/5 { \repeat unfold 10 a16 }
+  \times 4/5 { \repeat unfold 5 a16 }
+  \times 4/5 { \repeat unfold 5 a16 } |
+  \times 4/5 { \repeat unfold 10 a32 }
+  \times 4/5 { a4 a16 }
+  \set tupletSpannerDuration = #(ly:make-moment 1 4)
+  \times 4/5 { a8. a8 a8 a8. } |
+
+  \times 2/3 { a8 a a }
+  \times 2/3 { a4 a8 }
+  \times 2/3 { \repeat unfold 6 a8 } |
+  \times 4/6 { \repeat unfold 6 a8 }
+  \unset tupletSpannerDuration
+  \times 4/6 { \repeat unfold 6 a8 } |
+  \times 2/3 { a4 a8 a8 a4 }
+  \times 4/6 { a4 a8 a8 a4 } |
+}
+{
+  \time 3/4
+  \times 6/8 { \repeat unfold 8 a8 } |
+  \times 6/9 { \repeat unfold 9 a8 } |
+  \times 6/10 { \repeat unfold 10 a8 } |
+  \times 3/4 { \repeat unfold 8 a8 } |
+  \times 3/5 { \repeat unfold 10 a8 } |
+  \times 6/9 { a4 a8 a8 a4 a8 a a } |
+}
Index: input/regression/autobeam-show-defaults.ly
diff --git a/input/regression/autobeam-show-defaults.ly  
b/input/regression/autobeam-show-defaults.ly
index  
59a948ce59acb06ae54ce0b5f91deece92bfe19f..52c71baf062c5421a822908f02c265a3209bfd83  
100644

--- a/input/regression/autobeam-show-defaults.ly
+++ b/input/regression/autobeam-show-defaults.ly
@@ -34,11 +34,12 @@ Each score shows the desired beaming
 {
   \time 3/2
   a8^\markup Beams should end at 4/8, 8/8, 10/8 and 12/8
-  \repeat unfold 10 a8 a16 a32 a64 a128 a }
+  \repeat unfold 3 a8 \repeat unfold 8 a16 a8 a8 a8 a16 a32 a64 a128 a }
 {
   \time 3/4
   a8^\markup 1/8 beams should end at 3/4; smaller beams should end at  
1/4, 2/4, and 3/4

   \repeat unfold 5 a8
+  \repeat unfold 12 a16
   \repeat unfold 11 a16 a32 a64 a128 a
 }
 {
@@ -55,9 +56,9 @@ Each score shows the desired beaming
 }
 {
   \time 4/2
-  a8^\markup Beams should end at 4/8, 8/8, 12/8, 14/8, and 16/8
-  \repeat unfold 14 a8
-  a16 a32 a64 a128 a
+  a8^\markup Beams should end at 4/8, 8/8, 10/8, 12/8, 14/8, and 16/8
+  \repeat unfold 3 a8 \repeat unfold 8 a16 \repeat unfold 16 a32
+  a8 a8 a8 a16 a32 a64 a128 a
 }
 {
   \time 4/4
@@ -91,9 +92,10 @@ Each score shows the desired beaming
 }
 {
   \time 9/4
-  a8^\markup Beams should end at 6/8, 12/8, 14/8, 16/8, and 18/8
-  \repeat unfold 16
-  a8 a16 a32 a64 a128 a
+  a8^\markup Beams should end at 6/8, 8/8, 10/8, 12/8, 14/8, 16/8, and  
18/8

+  \repeat unfold 5 a8
+  \repeat unfold 12 a16
+  \repeat unfold 5 a8 a16 a32 a64 a128 a
 }
 {
   \time 9/8
@@ -110,9 +112,11 @@ Each score shows the desired beaming
 }
 {
   \time 12/4
-  a8^\markup Beams should end at 6/8, 12/8, 18/8, 20/8, 22/8, and 24/8
-  \repeat unfold 22 a8
-  a16 a32 a64 a128 a
+  a8^\markup Beams should end at 6/8, 8/8, 10/8, 12/8, 14/8, 16/8, 18/8,  
20/8, 22/8, and 24/8

+  \repeat unfold 5 a8
+  \repeat unfold 12 a16
+  \repeat unfold 24 a32
+  \repeat unfold 5 a8 a16 a32 a64 a128 a
 }
 {
   \time 12/8
Index: scm/time-signature-settings.scm
diff --git a/scm/time-signature-settings.scm  
b/scm/time-signature-settings.scm
index  
af852575dcedeb521c56b44be4672d29e1b21210..22f483ea6174f195f7c595a7f78736325bff2de5  
100644

--- a/scm/time-signature-settings.scm
+++ b/scm/time-signature-settings.scm
@@ -121,9 +121,9 @@
 ;;   use defaults, so no entries necessary

 ;; in 6 4 time:
-;;   use defaults, but end beams with 32nd or finer each 1/4 beat
+;;   use defaults, but end beams with 8th triplets or finer each 1/4  
beat

 ((6 . 4) .
- ((beamExceptions . ((end .  (((1 . 16) . (4 4 4 4 4 4
+ ((beamExceptions . ((end .  (((1 . 12) . (3 3 3 3 3 3

 ;; in 6 8 time:
 ;;   use defaults, so no entries necessary
@@ -132,9 +132,9 @@
 ;;   use defaults, so no entries necessary

 ;; in 9 4 time:
-;;   use defaults, but end beams with 32nd or finer each 1 4 beat
+;;   use defaults, but end beams with 8th triplets or finer each 1/4  
beat

 ((9 . 4) .
- ((beamExceptions . ((end . (((1 . 32) . (8 8 8 8 8 8 8  
8
+ ((beamExceptions . ((end . (((1 . 12) . 

Re: Issue 1320: Scheme bar line interface (issue 6305115)

[benko . pal]

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83
scm/bar-line.scm:83: (define (make-colon-bar-line grob)
I'm afraid this defun doesn't match the relevant part of current
Bar_line::compound_barline.

try

\new Staff {
  \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
  s1 \bar :|
}

http://codereview.appspot.com/6305115/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: line_count fixes (issue 6211047)

[benko . pal]

http://codereview.appspot.com/6211047/diff/1/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/6211047/diff/1/lily/beam.cc#newcode1282
lily/beam.cc:1282: staff_span *= 0.5 / staff_space;
division is silly and works only because staff_space is generally 1.

http://codereview.appspot.com/6211047/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Add mail aliases for Carl, Colin, Janek, and Pál, for the sake of git shortlog (issue 6206057)

[benko . pal]

http://codereview.appspot.com/6206057/diff/1/.mailmap
File .mailmap (right):

http://codereview.appspot.com/6206057/diff/1/.mailmap#newcode5
.mailmap:5: Benkő Pál benko@gmail.com
On 2012/05/13 07:45:51, dak wrote:

On 2012/05/13 07:13:08, janek wrote:
 It seems that this document is in the format given name surname,

so here

it
 should be Pál Benkő (unless i'm heavily mistaken).



I'd rather say it is in the format



 full name mail address



and Hungarians happen to put the given name last in the full name.

Unless I am

mistaken, this is the same with Hu Haipeng.



Without a pressing reason to do otherwise, I would tend to keep the

names in

their natural order.


I don't have a strong preference either way: on one hand I like to find
myself under B rather than P; on the other hand I like people not
confusing my given name and surname.

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

Re: staff_radius fixes (issue 6202048)

[benko . pal]

On 2012/05/07 18:25:35, Graham Percival wrote:

looks like this simplifies things, which is always nice to see.



http://codereview.appspot.com/6202048/diff/1/lily/slur-configuration.cc

File lily/slur-configuration.cc (right):



http://codereview.appspot.com/6202048/diff/1/lily/slur-configuration.cc#newcode291

lily/slur-configuration.cc:291: size_t n = convex_head_distances.size

();

is .size() the number of staff spaces, or the amount of bytes it takes

in

memory, or what?  I'm slightly wondering about changing n from a Real

to a

size_t.


convex_head_distances is a std::vector, so size () counts its elements.
I've changed the type of n exactly because as Real it triggered a
warning (now another warning is triggered by /= n, but the code is more
readable this way, I hope).

A new patch set is uploaded, with all convex_head_distances.size ()
calls united.  regression tests are unchanged relative to the original
patch set.

http://codereview.appspot.com/6202048/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel