Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)

[david . nalesnik]

Thanks for your review, Janek!


https://codereview.appspot.com/6730044/diff/10001/input/regression/measure-counter-broken.ly
File input/regression/measure-counter-broken.ly (right):

https://codereview.appspot.com/6730044/diff/10001/input/regression/measure-counter-broken.ly#newcode6
input/regression/measure-counter-broken.ly:6: enclosed in parentheses."
On 2012/10/24 10:37:08, janek wrote:
> Do you mean that the number before the break is not parenthesized, and
the
> number of the part after the break is parenthesized?
> It may be worth rewording this sentence.

Done.

https://codereview.appspot.com/6730044/diff/10001/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

https://codereview.appspot.com/6730044/diff/10001/scm/define-event-classes.scm#newcode49
scm/define-event-classes.scm:49: tremolo-span-event tuplet-span-event))
On 2012/10/24 10:37:08, janek wrote:
> i think there's something wrong with the indentation here, but
considering how
> nitpicking about indentation didn't do us any good in the past i'm
leaning
> towards not caring about it.

Yes, it looks pretty bad, and the original doesn't appear to be right
either.  I made this consistent with `rhythmic-event' below.

https://codereview.appspot.com/6730044/diff/10001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/6730044/diff/10001/scm/define-grob-properties.scm#newcode204
scm/define-grob-properties.scm:204: (count-from ,integer? "The number
beginning a measure count.")
On 2012/10/24 10:37:08, janek wrote:

> I mean, count-from could work either like this
> {
>    \override MeasureCount count-from = 3
>    \startMeasureCount
>    a1 % no number
>    b1 % no number
>    a1 % 3
>    b1 % 4
> }
> or like this
> {
>    \override MeasureCount count-from = 3
>    \startMeasureCount
>    a1 % 3
>    b1 % 4
>    a1 % 5
>    b1 % 6
> }

The second example above is the way it works.  Hopefully my revisions
make this a bit clearer!

https://codereview.appspot.com/6730044/diff/10001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/6730044/diff/10001/scm/scheme-engravers.scm#newcode27
scm/scheme-engravers.scm:27: spanners.  Each spanner is bounded by the
first command column of successive
On 2012/10/24 10:37:08, janek wrote:
> what is a command column?

In the IR we find:

currentCommandColumn (graphical (layout) object)
Grob that is X-parent to all current breakable (clef, key signature,
etc.) items.

I changed the capitalization and enclosed it in @code{ } elsewhere.
Hope this makes it clearer (or a search more fruitful).

https://codereview.appspot.com/6730044/diff/10001/scm/scheme-engravers.scm#newcode61
scm/scheme-engravers.scm:61: ; if this is not done, _all_ command
columns will be numbered
On 2012/10/24 10:37:08, janek wrote:
> so, sometimes all command columns will be nubmered?

> or do you mean that the code needs to make sure that we're in a new
bar because
> if it didn't, all command columns would be numbered and that's
something we
> don't want?

Yes, exactly.  If this check weren't in place, you would get many many
numbers.

> In other words, maybe "_all_ command columns would be numbered" would
be a
> better wording?
>         

Yes, it would.  I made some more substantial changes here.

https://codereview.appspot.com/6730044/diff/10001/scm/scheme-engravers.scm#newcode67
scm/scheme-engravers.scm:67: ; first column of measure, even if grace
notes appear?
On 2012/10/24 10:37:08, janek wrote:
> umm, i don't understand.  Is it a question about what this code
actually does?
> Or some kind of rhethoric-question-comment? ;)

Oh, I was just mimicking the variable name with its question mark.  The
comment refers to the need to check for the first column of the measure.
 I made this a little less chatty :)

https://codereview.appspot.com/6730044/