Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
David, I don't mind making the changes, but I'm running into some problems. I used git pull -r before making a patch set and uploading to rietveld, but looking over the patch set there I see that other changes have found their way in--including your check-grob-path function. I'm probably making some silly error here, and I appreciate any advice you can give me for this. (Unfortunately, I probably won't be able to get this until some hours from now.) https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
On 2012/10/31 11:41:19, david.nalesnik wrote: David, I don't mind making the changes, but I'm running into some problems. I used git pull -r before making a patch set and uploading to rietveld, but looking over the patch set there I see that other changes have found their way in--including your check-grob-path function. check-grob-path is in current master. Rietveld is not all too clever about changing material underneath a patch, but you don't really need a separate review for those changes, as they are just a consequence of running convert-ly, and your patch already passed countdown. If you are worried, you can send me the patch and I'll do what is needed. https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
Thank you--I am a bit worried! Speaking of patch, when I run git format-patch I get six separate patches. Is there any way to compress them into one so this is more convenient for you? -David https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
http://codereview.appspot.com/6730044/diff/40002/ly/engraver-init.ly File ly/engraver-init.ly (right): http://codereview.appspot.com/6730044/diff/40002/ly/engraver-init.ly#newcode88 ly/engraver-init.ly:88: \consists Footnote_engraver Any idea where this line is from? It just appears in the last patch set. And it should have been removed with commit 5801d8b438473ad68ae74f7dc742bbc8eea887fa So the question is why it resurfaces here. http://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
On 2012/10/31 11:55:12, david.nalesnik wrote: Thank you--I am a bit worried! Speaking of patch, when I run git format-patch I get six separate patches. Is there any way to compress them into one so this is more convenient for you? -David Try if you can git send-email to work for you. That still results in 6 different messages, but applying them with git am is easy. https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
Please make the indicated convert-ly-like changes. Thanks! http://codereview.appspot.com/6730044/diff/32001/input/regression/measure-counter.ly File input/regression/measure-counter.ly (right): http://codereview.appspot.com/6730044/diff/32001/input/regression/measure-counter.ly#newcode20 input/regression/measure-counter.ly:20: \override Staff.MeasureCounter #'count-from = #2 This should be \override Staff.MeasureCounter.count-from = #2 to simulate convert-ly for 2.17.6 being run (sorry). http://codereview.appspot.com/6730044/diff/32001/input/regression/measure-counter.ly#newcode26 input/regression/measure-counter.ly:26: \revert Staff.MeasureCounter #'count-from Should now be \revert Staff.MeasureCounter.count-from http://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
http://codereview.appspot.com/6730044/diff/32001/input/regression/measure-counter.ly File input/regression/measure-counter.ly (right): http://codereview.appspot.com/6730044/diff/32001/input/regression/measure-counter.ly#newcode1 input/regression/measure-counter.ly:1: \version 2.17.5 Oops: also change version header, or just run convert-ly -ed for doing all combined changes before committing. http://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
On 2012/10/24 20:28:36, janek wrote: On Wed, Oct 24, 2012 at 4:10 PM, mailto:pkx1...@gmail.com wrote: Also, should we include Measure_counter_engraver in the Staff context by default? (it'd make documenting it simpler in the @lilypond if nothing else :) ) +1 from me :) Janek OK--done! https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
I found some ambiguities in descriptions.
Janek
http://codereview.appspot.com/6730044/diff/10001/input/regression/measure-counter-broken.ly
File input/regression/measure-counter-broken.ly (right):
http://codereview.appspot.com/6730044/diff/10001/input/regression/measure-counter-broken.ly#newcode6
input/regression/measure-counter-broken.ly:6: enclosed in parentheses.
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.
http://codereview.appspot.com/6730044/diff/10001/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):
http://codereview.appspot.com/6730044/diff/10001/scm/define-event-classes.scm#newcode49
scm/define-event-classes.scm:49: tremolo-span-event tuplet-span-event))
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.
http://codereview.appspot.com/6730044/diff/10001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):
http://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.)
I suppose you mean In a measure count, don't print numbers lower than
this one?
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
}
It's important to make it clear in which way count-from works.
http://codereview.appspot.com/6730044/diff/10001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):
http://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
what is a command column?
http://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
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?
In other words, maybe _all_ command columns would be numbered would be
a better wording?
http://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?
umm, i don't understand. Is it a question about what this code actually
does? Or some kind of rhethoric-question-comment? ;)
http://codereview.appspot.com/6730044/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
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/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
much clearer now, thanks! LGTM Janek https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
I've opened http://code.google.com/p/lilypond/issues/detail?id=2924 for the Documentation in the NR https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
Also, should we include Measure_counter_engraver in the Staff context by default? (it'd make documenting it simpler in the @lilypond if nothing else :) ) https://codereview.appspot.com/6730044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 2445: Add measure counter to LilyPond (issue 6730044)
On Wed, Oct 24, 2012 at 4:10 PM, pkx1...@gmail.com wrote: Also, should we include Measure_counter_engraver in the Staff context by default? (it'd make documenting it simpler in the @lilypond if nothing else :) ) +1 from me :) Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
