Re: Adds glissando stems to Lilypond. (issue4661061)

[address@hidden]

On Jun 30, 2011, at 5:29 PM, address@hidden wrote:

> can you show png examples of what you're trying to do?
>

PastedGraphic-1.pdf
Description: Adobe PDF document

> I think this patch has serious issues.
> 
> 
> http://codereview.appspot.com/4661061/diff/13/lily/music-sequence.cc
> File lily/music-sequence.cc (right):
> 
> http://codereview.appspot.com/4661061/diff/13/lily/music-sequence.cc#newcode118
> lily/music-sequence.cc:118: continue;
> why can't you do this with an override for the to-relative function
> property?
> 

The notes that bookend a series of glissando stems need to be able to be 
relative to each other, which requires looping through the list of pitches to 
get the next valid one (instead of just nixing the relative callback on one 
note).  I don't know if that makes sense...lemme know if it doesn't.

> http://codereview.appspot.com/4661061/diff/13/lily/note-spacing.cc
> File lily/note-spacing.cc (right):
> 
> http://codereview.appspot.com/4661061/diff/13/lily/note-spacing.cc#newcode53
> lily/note-spacing.cc:53: if (to_boolean (stem->get_property
> ("glissando-stem")))
> if (a&&b) ..
> 
> http://codereview.appspot.com/4661061/diff/13/lily/note-spacing.cc#newcode258
> lily/note-spacing.cc:258: if (!stem || !stem->is_live () ||
> Stem::is_invisible (stem) || to_boolean (stem->get_property
> ("glissando-stem")))
> didn't we have a gop or something prohibiting tabs?
> 

I don't think I added a tab...I may have preserved one, tho.

> http://codereview.appspot.com/4661061/diff/13/lily/stem.cc
> File lily/stem.cc (right):
> 
> http://codereview.appspot.com/4661061/diff/13/lily/stem.cc#newcode1082
> lily/stem.cc:1082: Stem::after_line_breaking (SCM smob)
> can you rethink your functionality to not need this callback?
> 
> we should try to kill the {after/before}_linebreaking functions as they
> dont play well with the  callback on get_property() mechanism
> 

The problem is that I need this to happen right after line-breaking but before 
anything else is called.
I see where you're coming from, though - I'll try to roll it into another 
callback.

> http://codereview.appspot.com/4661061/diff/13/lily/stem.cc#newcode1116
> lily/stem.cc:1116: Spanner *gls = dynamic_cast<Spanner *>
> (glissandi[i]);
> to_spanner() ?
> 
Ah, I didn't know such a function existed...useful...

> http://codereview.appspot.com/4661061/diff/13/lily/stem.cc#newcode1118
> lily/stem.cc:1118: Spanner *orig = gls->is_broken () ? gls :
> dynamic_cast<Spanner *> (gls->original ());
> hosed if gls == NULL
> 

Also useful....

> http://codereview.appspot.com/4661061/diff/13/lily/stem.cc#newcode1133
> lily/stem.cc:1133:
> huh?
> 
> What is this needed for?  The break processing automatically substitutes
> spanners to only point to the ones in the same system.
> 

Also useful...

All will be changed after I read Neil's comments!

Cheers,
MS