Re: Various clean-ups in stems and beams. (issue 6584045)
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. http://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2012/11/07 07:52:33, mike7 wrote:
On 7 nov. 2012, at 09:50, mailto:d...@gnu.org wrote:
> On 2012/11/07 05:32:40, mike7 wrote:
>> On 7 nov. 2012, at 00:50, mailto:d...@gnu.org wrote:
>
>> >
>> >
>
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108
>> >> > 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.
http://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 7 nov. 2012, at 09:50, d...@gnu.org wrote:
> On 2012/11/07 05:32:40, mike7 wrote:
>> On 7 nov. 2012, at 00:50, mailto:d...@gnu.org wrote:
>
>> >
>> >
>
> http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108
>> >> > 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2012/11/07 05:32:40, mike7 wrote:
On 7 nov. 2012, at 00:50, mailto:d...@gnu.org wrote:
>
>
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108
>> > 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.
http://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
http://codereview.appspot.com/6584045/diff/13015/input/regression/kievan-notation.ly File input/regression/kievan-notation.ly (right): http://codereview.appspot.com/6584045/diff/13015/input/regression/kievan-notation.ly#newcode13 input/regression/kievan-notation.ly:13: \bar "kievan" This needs to be: \bar "k" given the changes made by issue 2790. http://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 7 nov. 2012, at 00:50, d...@gnu.org wrote:
>
> http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108
>> > 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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108
> 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.
http://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 5 nov. 2012, at 00:40, aleksandr.andr...@gmail.com wrote: > > http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-notation.ly > File input/regression/kievan-notation.ly (right): > > http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-notation.ly#newcode12 > 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-style.ly > File input/regression/note-head-style.ly (right): > > http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108 > 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-notation.ly File input/regression/kievan-notation.ly (right): http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-notation.ly#newcode13 input/regression/kievan-notation.ly:13: \bar "kievan" Sorry, I missed one more thing. Evidently, after 2790, this line should now be: \bar "k" http://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2012/11/03 18:21:09, mike7 wrote: On 3 nov. 2012, at 12:26, mailto:d...@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. http://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-notation.ly File input/regression/kievan-notation.ly (right): http://codereview.appspot.com/6584045/diff/13014/input/regression/kievan-notation.ly#newcode12 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-style.ly File input/regression/note-head-style.ly (right): http://codereview.appspot.com/6584045/diff/13014/input/regression/note-head-style.ly#newcode108 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 http://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 4 nov. 2012, at 08:22, m...@hohlart.de wrote: > > http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-style.ly > File input/regression/note-head-style.ly (right): > > http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-style.ly#newcode106 > 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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-style.ly File input/regression/note-head-style.ly (right): http://codereview.appspot.com/6584045/diff/14002/input/regression/note-head-style.ly#newcode106 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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 3 nov. 2012, at 12:26, d...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 3 nov. 2012, at 12:26, d...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2012/11/03 11:37:08, mike7 wrote: On 3 nov. 2012, at 12:26, mailto:d...@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. http://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 3 nov. 2012, at 12:26, d...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
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.
http://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2 oct. 2012, at 17:38, aleksandr.andr...@gmail.com wrote:
> On 2012/10/02 14:56:28, mike7 wrote:
>> On 2 oct. 2012, at 16:42, mailto:aleksandr.andr...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2012/10/02 14:56:28, mike7 wrote:
On 2 oct. 2012, at 16:42, mailto:aleksandr.andr...@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.
http://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
Am 02.10.2012 16:56, schrieb m...@mikesolomon.org:
On 2 oct. 2012, at 16:42, aleksandr.andr...@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?
That might be the best way to go indeed.
While I support the idea of supporting the user as much as possible, the
underlying structure
of properties and callbacks should separate different 'tasks'.
For educational reasons, one might want to switch between normal and
kievan notation,
but in this case,
c d e f
\startKievan
c d e f
\stopKievan
looks way better to me than
c d e f
\override NoteHead #'style = #"kievan"
c d e f
\revert NoteHead #'style
(if that would work, it is).
Taking David's recent patches into account there seems to be a global
'simplification for the user' direction concerning lilypond's usability, so
getting rid of \override stuff in this case looks like the right way to go.
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.
+1
Marc
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2 oct. 2012, at 16:42, aleksandr.andr...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
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.
https://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
On 2 oct. 2012, at 05:02, aleksandr.andr...@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
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
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. https://codereview.appspot.com/6584045/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
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.
https://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Various clean-ups in stems and beams. (issue 6584045)
Reviewers: aleksandr.andreev,
Message:
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.
Description:
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.
Please review this at https://codereview.appspot.com/6584045/
Affected files:
A input/regression/kievan-notation.ly
M lily/include/beam.hh
M lily/include/stem.hh
M lily/stem.cc
M ly/engraver-init.ly
M scm/define-grobs.scm
M scm/output-lib.scm
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Various clean-ups in stems and beams. (issue 6584045)
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.
http://codereview.appspot.com/6584045/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
