Various clean-ups in stems and beams.

Various clean-ups in stems and beams.

*) Eliminates code dups for Kievan work.
*) Transfers functions that are called a lot to C++ to speed things up.
*) Eliminates unused variables.

No regtest changes.

[aleksandr.andreev, 2012-10-01 20:22:24]

http://codereview.appspot.com/6584045/diff/2001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/6584045/diff/2001/ly/engraver-init.ly#newcode1150
ly/engraver-init.ly:1150: \override Beam #'positions =
#beam::get-kievan-positions
The only issue with putting this here is that if the user enters Kievan notes
without using the default KievanVoice context, the properties don't get
initialized. For example, this no longer works:

\relative c' {
   \override NoteHead #'style = #'kievan
   e8 [ g]
   a[ bes]
}

Either we have to initialize these properties when the style property of the
NoteHead is set to kievan or we have to change the documentation to indicate
that Kievan notation can only be used with the default contexts. The latter
option would require changing the snippets in the documentation, among other
things.

[MikeSol, 2012-10-01 23:34:39]

I think a user should be able to use Kievan notation and normal
stems/flags/beams if she so chooses. I'll define something like

startKievanNotation = {
   %% Set glyph styles.
 \override NoteHead #'style = #'kievan
 \override Rest #'style = #'mensural
 \override Accidental #'glyph-name-alist = #alteration-kievan-glyph-name-alist
 \override Dots #'style = #'kievan
 \override Slur #'stencil = ##f

 %% There are beams in Kievan notation, but they are invoked manually
 \set autoBeaming = ##f
}

stopKievanNotation = {
 \revert NoteHead #'style
 \revert Rest #'style
 \revert Accidental #'glyph-name-alist
 \revert Dots #'style
 \revert Slur #'stencil

 \unset autoBeaming

}

and then use it in the documentation.

[aleksandr.andreev, 2012-10-02 03:02:15]

On 2012/10/01 23:34:39, MikeSol wrote:
> I think a user should be able to use Kievan notation and normal
> stems/flags/beams if she so chooses. I'll define something like
> 
> [snip]
>
> and then use it in the documentation.

I still see some issues with this. As written now in stem.cc, when the style
property of NoteHead is set to kievan, the stems are not "normal". In this case,
it is not clear what Kievan notation with "normal stems/flags/beams" would look
like. At best, there would be no stems and beams flat across the center of the
staff. Or, something like

\relative c' {
   \override NoteHead #'style = #'kievan
   a'8[ bes]
}

gives the warning "weird stem size, check for narrow beams". 

Also, because the calculation of the duration log moves to
note-head::calc-kievan-duration-log, something like

\relative c' {
   \override NoteHead #'style = #'kievan
   a'8
}

produces the wrong NoteHead stencil (a quarter note instead of an eighth note).

If we implement the patch as written, Kievan note heads outside of the
KievanVoice context wouldn't make sense. I'm not altogether opposed to that (I
see no real reason to not use KievanVoice), but it seems like a "policy" issue,
as it would make Kievan notation different from how other ancient notations are
handled.

[pkx166h, 2012-10-02 07:28:10]

On 2012/10/02 03:02:15, aleksandr.andreev wrote:
...
> 
> If we implement the patch as written, Kievan note heads outside of the
> KievanVoice context wouldn't make sense. I'm not altogether opposed to that (I
> see no real reason to not use KievanVoice), but it seems like a "policy"
issue,
> as it would make Kievan notation different from how other ancient notations
are
> handled.

If you are talking about the Documentation in the Notation Reference, the
Ancient Notation doesn't follow (mostly) the standard Policies at all - indeed
there is a while tracker to tidy this section up, I started but I don't have any
knowlegde of Ancient Notation to make distinctions between the different types.

So I wouldn't worry about that. If examples need a specific context include in
the @lilypond examples then so be it. I don't think that that is a huge deal.

We do try not to have tweaks and overrides in the examples (they go into
snippets in that case) but this is hardly a tweak and so could be made into an
exception.

[mike7, 2012-10-02 08:07:06]

On 2 oct. 2012, at 05:02, aleksandr.andreev@gmail.com wrote:

> On 2012/10/01 23:34:39, MikeSol wrote:
>> I think a user should be able to use Kievan notation and normal
>> stems/flags/beams if she so chooses. I'll define something like
> 
>> [snip]
> 
>> and then use it in the documentation.
> 
> I still see some issues with this. As written now in stem.cc, when the
> style property of NoteHead is set to kievan, the stems are not "normal".
> In this case, it is not clear what Kievan notation with "normal
> stems/flags/beams" would look like. At best, there would be no stems and
> beams flat across the center of the staff. Or, something like
> 
> \relative c' {
>   \override NoteHead #'style = #'kievan
>   a'8[ bes]
> }
> 
> gives the warning "weird stem size, check for narrow beams".
> 
> Also, because the calculation of the duration log moves to
> note-head::calc-kievan-duration-log, something like
> 
> \relative c' {
>   \override NoteHead #'style = #'kievan
>   a'8
> }
> 
> produces the wrong NoteHead stencil (a quarter note instead of an eighth
> note).
> 
> If we implement the patch as written, Kievan note heads outside of the
> KievanVoice context wouldn't make sense. I'm not altogether opposed to
> that (I see no real reason to not use KievanVoice), but it seems like a
> "policy" issue, as it would make Kievan notation different from how
> other ancient notations are handled.

Hey Aleksandr,

All good points.  The convention in LilyPond is that grob properties of grob X
are used downstream to calculate other properties of grob X (and not of grob Y).
 This is why, for example, both the stem and rhythmic-head interface have
duration-log.  There are, of course, exceptions, but these could have almost
always been written another way.  For example, Stem::offset_callback should
likely be rewritten such that the early music stuff is a different function
implemented in Scheme.

The goal should be to get all references to non-common-practice-era notation out
of the main interface files and into either separate C++ files or
output-lib.scm.

The issue here is that the set_property calls for stem-begin-position and
length, which I've always run up against, pose new challenges for Kievan beams. 
I'll try to work on eliminating these calls, as they are opaque and make modular
add-ons difficult.

Cheers,
MS

[aleksandr.andreev, 2012-10-02 14:42:09]

On 2012/10/02 07:28:10, J_lowe wrote:
> If you are talking about the Documentation in the Notation Reference, the
> Ancient Notation doesn't follow (mostly) the standard Policies at all - indeed
> there is a while tracker to tidy this section up, I started but I don't have
any
> knowlegde of Ancient Notation to make distinctions between the different
types.
> 
> So I wouldn't worry about that. If examples need a specific context include in
> the @lilypond examples then so be it. I don't think that that is a huge deal.

What I mean is that if something like 

{
\override NoteHead #'style = #'kievan
c'8
}

produces a quarter note in the output, the user is likely to be thoroughly
confused.

If NoteHead and Stem properties are set up in engraver-init.ly, then if the user
wants a Kievan NoteHead somewhere other than in a KievanVoice context, he will
need to set up other overrides in addition to NoteHead style.

This is different from other ancient notations in the sense that if I do, e.g.,
{
\override NoteHead #'style = #'mensural
}

things work as expected. Of course, Mensural notation is simpler in the sense
that it lacks beams and specialized eighth heads.

I think it would be best if setting NoteHead style to Kievan would set the
necessary duration-log and Stem properties, but I am not sure how that would be
implemented given the clean-ups that Mike is proposing with this patch.

[mike7, 2012-10-02 14:56:28]

On 2 oct. 2012, at 16:42, aleksandr.andreev@gmail.com wrote:

> What I mean is that if something like
> 
> {
> \override NoteHead #'style = #'kievan
> c'8
> }
> 
> produces a quarter note in the output, the user is likely to be
> thoroughly confused.

Is this because of the duration-log override?  NoteHead #'duration-log for
Kievan notation should be a separate function that is not based on style (this
is what I propose in my patch).

> 
> If NoteHead and Stem properties are set up in engraver-init.ly, then if
> the user wants a Kievan NoteHead somewhere other than in a KievanVoice
> context, he will need to set up other overrides in addition to NoteHead
> style.
> 

Why can't we have commands \startKievan and \stopKievan that do all the
overrides simultaneously?

For example, the command \hideNotes groups together several overrides.  I think
it would be a mistake to make \override NoteHead #'transparent also control Dots
and Stems by default.  It's better to have a global command that groups together
all these overrides.

> This is different from other ancient notations in the sense that if I
> do, e.g.,
> {
> \override NoteHead #'style = #'mensural
> }
> 
> things work as expected. Of course, Mensural notation is simpler in the
> sense that it lacks beams and specialized eighth heads.
> 
> I think it would be best if setting NoteHead style to Kievan would set
> the necessary duration-log and Stem properties, but I am not sure how
> that would be implemented given the clean-ups that Mike is proposing
> with this patch.

I think it's important to keep properties as separate as possible.  For example,
take the command arpeggioParenthesisDashed.  In theory, we could make it such
that arpeggio has a style property that'd set all these things, but it's better
to keep them as separate overrides to various parameters.  Same for easyHeadsOn,
improvisationOn, and tabFullNotation.  A command like \startKievan or \kievanOn
or whatever should work like this.

Cheers,
MS

[aleksandr.andreev, 2012-10-02 15:38:29]

On 2012/10/02 14:56:28, mike7 wrote:
> On 2 oct. 2012, at 16:42, mailto:aleksandr.andreev@gmail.com wrote:
> 
> > What I mean is that if something like
> > 
> > {
> > \override NoteHead #'style = #'kievan
> > c'8
> > }
> > 
> > produces a quarter note in the output, the user is likely to be
> > thoroughly confused.
> 
> Is this because of the duration-log override? 

Yes.

> NoteHead #'duration-log for
> Kievan notation should be a separate function that is not based on style (this
> is what I propose in my patch).
> 

Then it has to be set with an override whenever the style is set to kievan.

> 
> Why can't we have commands \startKievan and \stopKievan that do all the
> overrides simultaneously?

I think that would work.
 
> For example, the command \hideNotes groups together several overrides.  I
think
> it would be a mistake to make \override NoteHead #'transparent also control
Dots
> and Stems by default.

Sure. The trouble is that Stems don't make much sense when NoteHead style is set
to kievan and warnings are produced with beams. Of course, we could rewrite the
way that is handled in stem::is_normal_stem, but that would take some thought.

>  It's better to have a global command that groups together
> all these overrides.
>

Definitely.
 
> I think it's important to keep properties as separate as possible.  For
example,
> take the command arpeggioParenthesisDashed.  In theory, we could make it such
> that arpeggio has a style property that'd set all these things, but it's
better
> to keep them as separate overrides to various parameters.  Same for
easyHeadsOn,
> improvisationOn, and tabFullNotation.  A command like \startKievan or
\kievanOn
> or whatever should work like this.

I agree, but see the above comment about Stems not being normal.

[mike7, 2012-10-02 21:22:37]

On 2 oct. 2012, at 17:38, aleksandr.andreev@gmail.com wrote:

> On 2012/10/02 14:56:28, mike7 wrote:
>> On 2 oct. 2012, at 16:42, mailto:aleksandr.andreev@gmail.com wrote:
> 
>> > What I mean is that if something like
>> >
>> > {
>> > \override NoteHead #'style = #'kievan
>> > c'8
>> > }
>> >
>> > produces a quarter note in the output, the user is likely to be
>> > thoroughly confused.
> 
>> Is this because of the duration-log override?
> 
> Yes.
> 
>> NoteHead #'duration-log for
>> Kievan notation should be a separate function that is not based on
> style (this
>> is what I propose in my patch).
> 
> 
> Then it has to be set with an override whenever the style is set to
> kievan.
> 
> 
>> Why can't we have commands \startKievan and \stopKievan that do all
> the
>> overrides simultaneously?
> 
> I think that would work.
> 
>> For example, the command \hideNotes groups together several overrides.
> I think
>> it would be a mistake to make \override NoteHead #'transparent also
> control Dots
>> and Stems by default.
> 
> Sure. The trouble is that Stems don't make much sense when NoteHead
> style is set to kievan and warnings are produced with beams. Of course,
> we could rewrite the way that is handled in stem::is_normal_stem, but
> that would take some thought.

I don't think this'd be necessary - you'd just have to override
quantize-position with a callback that sets stem-begin-position and length.  The
problem is that Stem::internal_stem_begin_position expects that, if there is a
beam, the value of stem-begin-position will be set by Beam::set_stem_lengths and
that it can recuperate this value as the result of the callback.  So long as the
quantize callback does this, there shouldn't be a circular dependency.  I can
write this on the plane tomorrow.

Cheers,
MS

[dak, 2012-11-03 11:26:44]

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

http://codereview.appspot.com/6584045/diff/1/lily/beam.cc#newcode197
lily/beam.cc:197: Grob *me = unsmob_grob (smob);
Looking at the combination of this and is_kievan, it would appear that the
expected response when calling Beam::calc-is-kievan (why no question mark in the
name?) with a non-Grob is a segmentation fault.

That's sub-fabulous.

http://codereview.appspot.com/6584045/diff/1/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/6584045/diff/1/lily/stem.cc#newcode815
lily/stem.cc:815: Grob *beam = unsmob_grob (me->get_object ("beam"));
Again: it would appear that calling this with a non-grob is intended to crash
LilyPond.  Is there a particular reason you write your callbacks without
typechecking their arguments?

We have things like LY_ASSERT_TYPE for a reason.

http://codereview.appspot.com/6584045/diff/1/lily/stem.cc#newcode833
lily/stem.cc:833: Grob *beam = unsmob_grob (me->get_object ("beam"));
See above.

http://codereview.appspot.com/6584045/diff/1/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/6584045/diff/1/scm/output-lib.scm#newcode70
scm/output-lib.scm:70: (next-stem (cadr stems-grobs))
The above line may set stems-grobs to '(), in which case this line will bomb out
with a Scheme error.  Where is the point?

http://codereview.appspot.com/6584045/diff/1/scm/output-lib.scm#newcode85
scm/output-lib.scm:85: (left-height (if (= direction DOWN)
Is direction sure to be non-null?

http://codereview.appspot.com/6584045/diff/12001/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/6584045/diff/12001/lily/stem.cc#newcode812
lily/stem.cc:812: Real beg = robust_scm2double (me->get_pure_property
("stem-begin-position", 0, INT_MAX), 0.0);
Using robust_scm2double does not help when me is 0.  This still needs
LY_ASSERT_TYPE or similar.

http://codereview.appspot.com/6584045/diff/12001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/6584045/diff/12001/ly/engraver-init.ly#newcode1145
ly/engraver-init.ly:1145: \override Stem.stencil = #'()
Overriding a stencil with an empty list seems strange: is there a difference in
meaning to overriding it with #f?

http://codereview.appspot.com/6584045/diff/12001/ly/property-init.ly
File ly/property-init.ly (right):

http://codereview.appspot.com/6584045/diff/12001/ly/property-init.ly#newcode315
ly/property-init.ly:315: kievanOff = {
Could be just

    kievanOff = \undo \kievanOn

Less efficient (if we ever find it makes a difference, \undo could be done in
C++ mostly), but less prone to oversights as well.

[mike7, 2012-11-03 11:37:08]

On 3 nov. 2012, at 12:26, dak@gnu.org wrote:

> 
> http://codereview.appspot.com/6584045/diff/1/lily/beam.cc
> File lily/beam.cc (right):
> 
> http://codereview.appspot.com/6584045/diff/1/lily/beam.cc#newcode197
> lily/beam.cc:197: Grob *me = unsmob_grob (smob);
> Looking at the combination of this and is_kievan, it would appear that
> the expected response when calling Beam::calc-is-kievan (why no question
> mark in the name?) with a non-Grob is a segmentation fault.
> 
> That's sub-fabulous.
> 

Quick response - if you're looking at changes in beam.cc, you are reviewing an
old patch set from a month ago or so.  It seems that some of your comments are
in this and some are in the new one.  I'll figure out which ones still apply,
but just giving you and other reviewers the heads up.

Cheers,
MS

[dak, 2012-11-03 11:43:26]

On 2012/11/03 11:37:08, mike7 wrote:
> On 3 nov. 2012, at 12:26, mailto:dak@gnu.org wrote:
> 
> > 
> > http://codereview.appspot.com/6584045/diff/1/lily/beam.cc
> > File lily/beam.cc (right):
> > 
> > http://codereview.appspot.com/6584045/diff/1/lily/beam.cc#newcode197
> > lily/beam.cc:197: Grob *me = unsmob_grob (smob);
> > Looking at the combination of this and is_kievan, it would appear that
> > the expected response when calling Beam::calc-is-kievan (why no question
> > mark in the name?) with a non-Grob is a segmentation fault.
> > 
> > That's sub-fabulous.
> > 
> 
> Quick response - if you're looking at changes in beam.cc, you are reviewing an
> old patch set from a month ago or so.

The problem with incremental changes like this is that the humongous total
change set is hard to review, and the incremental change is considered
"outdated".

If we were reviewing this as a commit series of git, every commit in the series
of only loosely related changes would be expected to be sane.  With Rietveld,
one can't really distinguish between history of changes and sequence of commits.

So I apologize for having looked at this in parts, but it would appear that the
LY_ASSERT_TYPE thing applies equally well to the latest change.

[mike7, 2012-11-03 18:07:20]

On 3 nov. 2012, at 12:26, dak@gnu.org wrote:

> http://codereview.appspot.com/6584045/diff/12001/lily/stem.cc#newcode812
> lily/stem.cc:812: Real beg = robust_scm2double (me->get_pure_property
> ("stem-begin-position", 0, INT_MAX), 0.0);
> Using robust_scm2double does not help when me is 0.  This still needs
> LY_ASSERT_TYPE or similar.
> 

Hey David,

I finally had a chance to read these over - many thanks!

For LY_ASSERT_TYPE, I agree with you that this should be used all over the
place.  Unfortunately, it is not.  If you read the code base, you will see that
almost every single callback leaves grobs untested, which would cause segfaults
for null pointers.

I think the best idea would be to open up a separate tracker issue that talks
about this problem.  Given how often the first argument of scheme callbacks is a
grob, it may be worth it to create macros MAKE_SCHEME_GROB_CALLBACK,
MAKE_SCHEME_ITEM_CALLBACK, and MAKE_SCHEME_SPANNER_CALLBACK.  The macros would
roll in a LY_ASSERT_TYPE, thus avoiding writing it out in every function.

I'll make sure to fix everything in your other comments, but stuff pertaining to
LY_ASSERT_TYPE should wait until we figure out a policy on how this should be
handled across all callbacks.  While we wait for that to get sorted out, we
could create a moratorium on pushing any patches that create new callbacks,
although I think that'd be a bit excessive.

Cheers,
MS

[mike7, 2012-11-03 18:21:09]

On 3 nov. 2012, at 12:26, dak@gnu.org wrote:

> http://codereview.appspot.com/6584045/diff/1/scm/output-lib.scm#newcode85
> scm/output-lib.scm:85: (left-height (if (= direction DOWN)
> Is direction sure to be non-null?
> 

Thanks again for the review.  Aleksandr can respond to this better than I can.

Cheers,
MS

[marc, 2012-11-04 07:22:44]

http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-s...
File input/regression/note-head-style.ly (right):

http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-s...
input/regression/note-head-style.ly:106: \override Staff.Stem.stencil = #'()
Should this not rather read

\override Staff.Stem.stencil = ##f
or \omit Staff.Stem ?

[mike7, 2012-11-04 07:38:14]

On 4 nov. 2012, at 08:22, marc@hohlart.de wrote:

> 
>
http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-s...
> File input/regression/note-head-style.ly (right):
> 
>
http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-s...
> input/regression/note-head-style.ly:106: \override Staff.Stem.stencil =
> #'()
> Should this not rather read
> 
> \override Staff.Stem.stencil = ##f
> or \omit Staff.Stem ?
> 
> http://codereview.appspot.com/6584045/

Fixed.

Cheers,
MS

[aleksandr.andreev, 2012-11-04 22:40:50]

http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-nota...
File input/regression/kievan-notation.ly (right):

http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-nota...
input/regression/kievan-notation.ly:12: c4 c4 c8 [ d8 ] c4 c2 b,\longa
We should add an unbeamed eighth note to the regtest, since its correct
appearance is now controlled by 

note-head::calc-kievan-duration-log

Maybe something like

c4 c8 c8[ d8] c4 c2 b,\longa

http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
File input/regression/note-head-style.ly (right):

http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
input/regression/note-head-style.ly:108: \override Staff.Dots.style = #'kievan
Why can't we use the new function here, e.g.,

\kievanOn

http://codereview.appspot.com/6584045/diff/13014/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/6584045/diff/13014/ly/engraver-init.ly#newcode1150
ly/engraver-init.ly:1150: \override Stem.length = #0.0
It seems like we also need something like:

\override Flag.stencil = ##f

Otherwise "flags" appear on Kievan eighth notes.

http://codereview.appspot.com/6584045/diff/13014/ly/property-init.ly
File ly/property-init.ly (right):

http://codereview.appspot.com/6584045/diff/13014/ly/property-init.ly#newcode310
ly/property-init.ly:310: \override Stem.length = #0.0
Also need here:

\override Flag.stencil = ##f

http://codereview.appspot.com/6584045/diff/13014/ly/property-init.ly#newcode323
ly/property-init.ly:323: \revert Stem.length
And here:

\revert Flag.stencil

[aleksandr.andreev, 2012-11-04 22:43:08]

On 2012/11/03 18:21:09, mike7 wrote:
> On 3 nov. 2012, at 12:26, mailto:dak@gnu.org wrote:
> 
> > http://codereview.appspot.com/6584045/diff/1/scm/output-lib.scm#newcode85
> > scm/output-lib.scm:85: (left-height (if (= direction DOWN)
> > Is direction sure to be non-null?
> > 
> 
> Thanks again for the review.  Aleksandr can respond to this better than I can.
> 
> Cheers,
> MS
> 

Sorry for the delay in responding -- weekends are always very busy for me. The
default direction is set to DOWN in Beam::calc_direction, starting on line 291.

[aleksandr.andreev, 2012-11-04 22:51:12]

http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-nota...
File input/regression/kievan-notation.ly (right):

http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-nota...
input/regression/kievan-notation.ly:13: \bar "kievan"
Sorry, I missed one more thing.

Evidently, after 2790, this line should now be:
\bar "k"

[mike7, 2012-11-06 21:25:30]

On 5 nov. 2012, at 00:40, aleksandr.andreev@gmail.com wrote:

> 
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-nota...
> File input/regression/kievan-notation.ly (right):
> 
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-nota...
> input/regression/kievan-notation.ly:12: c4 c4 c8 [ d8 ] c4 c2 b,\longa
> We should add an unbeamed eighth note to the regtest, since its correct
> appearance is now controlled by
> 
> note-head::calc-kievan-duration-log
> 
> Maybe something like
> 
> c4 c8 c8[ d8] c4 c2 b,\longa

Done

> 
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
> File input/regression/note-head-style.ly (right):
> 
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
> input/regression/note-head-style.ly:108: \override Staff.Dots.style =
> #'kievan
> Why can't we use the new function here, e.g.,
> 
> \kievanOn
> 

\kievenOn only works on the voice level and the overrides happen on the staff
level.

> http://codereview.appspot.com/6584045/diff/13014/ly/engraver-init.ly
> File ly/engraver-init.ly (right):
> 
>
http://codereview.appspot.com/6584045/diff/13014/ly/engraver-init.ly#newcode1150
> ly/engraver-init.ly:1150: \override Stem.length = #0.0
> It seems like we also need something like:
> 
> \override Flag.stencil = ##f
> 
> Otherwise "flags" appear on Kievan eighth notes.
> 

True. Fixed.

> http://codereview.appspot.com/6584045/diff/13014/ly/property-init.ly
> File ly/property-init.ly (right):
> 
>
http://codereview.appspot.com/6584045/diff/13014/ly/property-init.ly#newcode310
> ly/property-init.ly:310: \override Stem.length = #0.0
> Also need here:
> 
> \override Flag.stencil = ##f

Also fixed.

> 
>
http://codereview.appspot.com/6584045/diff/13014/ly/property-init.ly#newcode323
> ly/property-init.ly:323: \revert Stem.length
> And here:
> 
> \revert Flag.stencil

Also also fixed.

Many thanks!  Will post on Rietveld tonight or tomorrow.

Cheers,
MS

[dak, 2012-11-06 22:50:07]

>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
> > input/regression/note-head-style.ly:108: \override Staff.Dots.style =
> > #'kievan
> > Why can't we use the new function here, e.g.,
> > 
> > \kievanOn
> > 
> 
> \kievenOn only works on the voice level and the overrides happen on the staff
> level.

How about making \pattern a music function taking a context
modification as argument?  Then you could write

\pattern \with { \override NoteHead.style = #'slash }
\pattern \with { \kievanOn }
\pattern \with { ... }

and just pass the context mod on to the \new Voice within \pattern.

It seems awkward to use Staff-wide overrides here.

By the way: it's frightening how fast one gets used to the new
override syntax.  I had to think really hard about whether this was
supposed to be different previously or not.  And then it seems strange
that there would have been no dot.

[mike7, 2012-11-07 05:32:40]

On 7 nov. 2012, at 00:50, dak@gnu.org wrote:

> 
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
>> > input/regression/note-head-style.ly:108: \override Staff.Dots.style
> =
>> > #'kievan
>> > Why can't we use the new function here, e.g.,
>> >
>> > \kievanOn
>> >
> 
>> \kievenOn only works on the voice level and the overrides happen on
> the staff
>> level.
> 
> How about making \pattern a music function taking a context
> modification as argument?  Then you could write
> 
> \pattern \with { \override NoteHead.style = #'slash }
> \pattern \with { \kievanOn }
> \pattern \with { ... }
> 
> and just pass the context mod on to the \new Voice within \pattern.
> 
> It seems awkward to use Staff-wide overrides here.
> 
> By the way: it's frightening how fast one gets used to the new
> override syntax.  I had to think really hard about whether this was
> supposed to be different previously or not.  And then it seems strange
> that there would have been no dot.
> 

I'm not exactly sure how this'd be done - if it's OK w/ you, I'll push the patch
after a countdown and then you can propose a followup patch.  Sorry - I have
been out of the loop for a bit and am not up-to-date on the most recent syntax
modifications.

Cheers,
MS

[aleksandr.andreev, 2012-11-07 06:00:23]

http://codereview.appspot.com/6584045/diff/13015/input/regression/kievan-nota...
File input/regression/kievan-notation.ly (right):

http://codereview.appspot.com/6584045/diff/13015/input/regression/kievan-nota...
input/regression/kievan-notation.ly:13: \bar "kievan"
This needs to be:

\bar "k"

given the changes made by issue 2790.

[dak, 2012-11-07 07:50:07]

On 2012/11/07 05:32:40, mike7 wrote:
> On 7 nov. 2012, at 00:50, mailto:dak@gnu.org wrote:
> 
> > 
> >
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
> >> > input/regression/note-head-style.ly:108: \override Staff.Dots.style
> > =
> >> > #'kievan
> >> > Why can't we use the new function here, e.g.,
> >> >
> >> > \kievanOn
> >> >
> > 
> >> \kievenOn only works on the voice level and the overrides happen on
> > the staff
> >> level.
> > 
> > How about making \pattern a music function taking a context
> > modification as argument?  Then you could write
> > 
> > \pattern \with { \override NoteHead.style = #'slash }
> > \pattern \with { \kievanOn }
> > \pattern \with { ... }
> > 
> > and just pass the context mod on to the \new Voice within \pattern.
> > 
> > It seems awkward to use Staff-wide overrides here.
> > 
> > By the way: it's frightening how fast one gets used to the new
> > override syntax.  I had to think really hard about whether this was
> > supposed to be different previously or not.  And then it seems strange
> > that there would have been no dot.
> > 
> 
> I'm not exactly sure how this'd be done - if it's OK w/ you, I'll push the
patch
> after a countdown and then you can propose a followup patch.  Sorry - I have
> been out of the loop for a bit and am not up-to-date on the most recent syntax
> modifications.

There are no really new syntax modifications required.  The problem with the "OK
w/ you" part is that it is simply wrong _not_ to use the \kievanOn function in
this regtest and instead copy and modify its contents.  That way, the regtests
ceases to test \kievanOn and depends on its internals not changing.

I'll put up a git-format patch on the Google issue tracker as Rietveld does not
appear to take attachments.

[mike7, 2012-11-07 07:52:33]

On 7 nov. 2012, at 09:50, dak@gnu.org wrote:

> On 2012/11/07 05:32:40, mike7 wrote:
>> On 7 nov. 2012, at 00:50, mailto:dak@gnu.org wrote:
> 
>> >
>> >
> 
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
>> >> > input/regression/note-head-style.ly:108: \override
> Staff.Dots.style
>> > =
>> >> > #'kievan
>> >> > Why can't we use the new function here, e.g.,
>> >> >
>> >> > \kievanOn
>> >> >
>> >
>> >> \kievenOn only works on the voice level and the overrides happen on
>> > the staff
>> >> level.
>> >
>> > How about making \pattern a music function taking a context
>> > modification as argument?  Then you could write
>> >
>> > \pattern \with { \override NoteHead.style = #'slash }
>> > \pattern \with { \kievanOn }
>> > \pattern \with { ... }
>> >
>> > and just pass the context mod on to the \new Voice within \pattern.
>> >
>> > It seems awkward to use Staff-wide overrides here.
>> >
>> > By the way: it's frightening how fast one gets used to the new
>> > override syntax.  I had to think really hard about whether this was
>> > supposed to be different previously or not.  And then it seems
> strange
>> > that there would have been no dot.
>> >
> 
>> I'm not exactly sure how this'd be done - if it's OK w/ you, I'll push
> the patch
>> after a countdown and then you can propose a followup patch.  Sorry -
> I have
>> been out of the loop for a bit and am not up-to-date on the most
> recent syntax
>> modifications.
> 
> There are no really new syntax modifications required.  The problem with
> the "OK w/ you" part is that it is simply wrong _not_ to use the
> \kievanOn function in this regtest and instead copy and modify its
> contents.  That way, the regtests ceases to test \kievanOn and depends
> on its internals not changing.
> 
> I'll put up a git-format patch on the Google issue tracker as Rietveld
> does not appear to take attachments.
> 

Excellent - I'll copy and paste whatever you come up with into my patch.  Thanks
for taking the time to do this.

Cheers,
MS

[aleksandr.andreev, 2012-11-07 16:27:39]

On 2012/11/07 07:52:33, mike7 wrote:
> On 7 nov. 2012, at 09:50, mailto:dak@gnu.org wrote:
> 
> > On 2012/11/07 05:32:40, mike7 wrote:
> >> On 7 nov. 2012, at 00:50, mailto:dak@gnu.org wrote:
> > 
> >> >
> >> >
> > 
> >
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-s...
> >> >> > input/regression/note-head-style.ly:108: \override
> > Staff.Dots.style
> >> > =
> >> >> > #'kievan
> >> >> > Why can't we use the new function here, e.g.,
> >> >> >
> >> >> > \kievanOn
> >> >> >
> >> >
> >> >> \kievenOn only works on the voice level and the overrides happen on
> >> > the staff
> >> >> level.
> >> >
> >> > How about making \pattern a music function taking a context
> >> > modification as argument?  Then you could write
> >> >
> >> > \pattern \with { \override NoteHead.style = #'slash }
> >> > \pattern \with { \kievanOn }
> >> > \pattern \with { ... }
> >> >
> >> > and just pass the context mod on to the \new Voice within \pattern.
> >> >
> >> > It seems awkward to use Staff-wide overrides here.
> >> >
> >> > By the way: it's frightening how fast one gets used to the new
> >> > override syntax.  I had to think really hard about whether this was
> >> > supposed to be different previously or not.  And then it seems
> > strange
> >> > that there would have been no dot.
> >> >
> > 
> >> I'm not exactly sure how this'd be done - if it's OK w/ you, I'll push
> > the patch
> >> after a countdown and then you can propose a followup patch.  Sorry -
> > I have
> >> been out of the loop for a bit and am not up-to-date on the most
> > recent syntax
> >> modifications.
> > 
> > There are no really new syntax modifications required.  The problem with
> > the "OK w/ you" part is that it is simply wrong _not_ to use the
> > \kievanOn function in this regtest and instead copy and modify its
> > contents.  That way, the regtests ceases to test \kievanOn and depends
> > on its internals not changing.
> > 
> > I'll put up a git-format patch on the Google issue tracker as Rietveld
> > does not appear to take attachments.
> > 
> 
> Excellent - I'll copy and paste whatever you come up with into my patch. 
Thanks
> for taking the time to do this.
> 
> Cheers,
> MS
> 

LGTM.

[janek, 2012-11-10 14:42:47]

This patch is a bit too complex for me to understand after just reading, sorry
:(
But i didn't spot anything wrong.
Janek

http://codereview.appspot.com/6584045/diff/27001/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/6584045/diff/27001/scm/output-lib.scm#newcode68
scm/output-lib.scm:68: ;; x extrema will fall
That's a nice comment.  I'd find it nice if a similar one was in
ly/engraver-init.ly.