Updated patch for issue 155

Updated patch for issue 155

This patch is updated from Joe Neeman's patch for issue 155 so it applies to current master.

The main difference is the change in handling pure functions.  I'm not an expert on this so I'm not sure I have it 100% right.

But at least this works on the regtests.

Please review.

[Valentin Villenave, 2015-04-15 06:26:02]

Hi Carl,
thanks for resurrecting this patch! From what I understand, the logic behind
this patch looks quite sensible (but IANAP, so please take it with a grain of
salt).

Perhaps you could add a regtest demonstrating this feature (or add a
dotted+accidental note to parenthesize.ly)? Just in case it breaks again in the
future.

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

https://codereview.appspot.com/230860044/diff/1/scm/output-lib.scm#newcode909
scm/output-lib.scm:909: (let* ((paren
(parentheses-iotem::calc-parenthesis-stencils grob))
Possible typo?

[Carl, 2015-04-16 02:20:34]

Updated patch.  Fixes typo, adds regtest.

[Carl, 2015-04-16 02:23:14]

On 2015/04/15 06:26:02, Valentin Villenave wrote:

> Perhaps you could add a regtest demonstrating this feature (or add a
> dotted+accidental note to parenthesize.ly)? Just in case it breaks again in
the
> future.

Actually, I had a regtest done and had tested it on my machine, but I didn't do
my git manipulation properly.  Thanks for catching it!

> https://codereview.appspot.com/230860044/diff/1/scm/output-lib.scm#newcode909
> scm/output-lib.scm:909: (let* ((paren
> (parentheses-iotem::calc-parenthesis-stencils grob))
> Possible typo?

Definitely a typo.  Fixed in the next patch.  Obviously the regtest doesn't test
this function (pure-y-extent).  The function may not be necessary any longer,
with the new way of handling pure calls.  But I don't know enough to make that
decision.

Thanks!

[Keith, 2015-04-19 19:32:29]

LGTM.

> The main difference is the change in handling pure functions.

I can't see any difference here relative to Joe's patch, but it looks fine.

https://codereview.appspot.com/230860044/diff/20001/scm/define-grob-propertie...
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/230860044/diff/20001/scm/define-grob-propertie...
scm/define-grob-properties.scm:777: (parenthesis-friends ,list? "A lisy of
symbols.  Any parenthesis
"A list of Grob types, as symbols.  When parentheses enclose a Grob that has
'parenthesis-friends, the parentheses widen to include any child Grobs with type
among 'parethesis-friends."

https://codereview.appspot.com/230860044/diff/20001/scm/define-grob-propertie...
scm/define-grob-properties.scm:779: these symbols on the parent grob.")
I think 'this grob' and 'parent grob' are the same, but I was confused, so I
tried to rewrite what I hope is simpler and correct.

https://codereview.appspot.com/230860044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/230860044/diff/20001/scm/output-lib.scm#newcod...
scm/output-lib.scm:908: (define-public (parentheses-item::pure-y-extent grob
start end)
I do not see why this function is necessary, but it looks correct.

There is a distinction between pure-y-extent and y-extent for objects whose
vertical position or height might change depending on where the line-breaks
fall.  

Maybe it was formerly necessary for the accidental on the second note of a tie,
which might disappear, but we let those accidentals have a simple height now.

The only need I could see for pure-y-extent here, would be to estimate
horizontal space for the parentheses around an accent outside a slur or beam.

[Carl, 2015-04-21 18:46:14]

On 2015/04/19 19:32:29, Keith wrote:
> LGTM.
> 
> > The main difference is the change in handling pure functions.
> 
> I can't see any difference here relative to Joe's patch, but it looks fine.

When Joe's patch was written, we had a scheme procedure that turned impure
functions pure, based on an alist.  That is now removed from LilyPond, so there
is no place that uses pure-y-extent.

Thanks,

Carl

[Carl, 2015-04-21 18:46:38]

https://codereview.appspot.com/230860044/diff/20001/scm/define-grob-propertie...
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/230860044/diff/20001/scm/define-grob-propertie...
scm/define-grob-properties.scm:777: (parenthesis-friends ,list? "A lisy of
symbols.  Any parenthesis
On 2015/04/19 19:32:29, Keith wrote:
> "A list of Grob types, as symbols.  When parentheses enclose a Grob that has
> 'parenthesis-friends, the parentheses widen to include any child Grobs with
type
> among 'parethesis-friends."

Done.

https://codereview.appspot.com/230860044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/230860044/diff/20001/scm/output-lib.scm#newcod...
scm/output-lib.scm:908: (define-public (parentheses-item::pure-y-extent grob
start end)
On 2015/04/19 19:32:29, Keith wrote:
> I do not see why this function is necessary, but it looks correct.
> 
> There is a distinction between pure-y-extent and y-extent for objects whose
> vertical position or height might change depending on where the line-breaks
> fall.  
> 
> Maybe it was formerly necessary for the accidental on the second note of a
tie,
> which might disappear, but we let those accidentals have a simple height now.
> 
> The only need I could see for pure-y-extent here, would be to estimate
> horizontal space for the parentheses around an accent outside a slur or beam.

So is it best to remove the code, since we don't have a regtest that exercises
it?

I think that if it turns out we need a pure-y-extent we'll know it because of a
cyclic dependency when someone tries to use \parenthesize, and then we can
reinstate the code and have a regtest for it.

Do you agree?

[Keith, 2015-04-22 04:04:33]

https://codereview.appspot.com/230860044/diff/20001/input/regression/parenthe...
File input/regression/parenthesize-notes-accidentals.ly (right):

https://codereview.appspot.com/230860044/diff/20001/input/regression/parenthe...
input/regression/parenthesize-notes-accidentals.ly:10: <\parenthesize ais'>4.
<\parenthesize bes'>
These parentheses overlap, which seems to be an accepted behavior of the
parentheses, but it will make us look twice every time we check the regression
tests.  Maybe just boost the bes' up a bit.  
And you could just modify 'parenthesize.ly' to have an accidental and a dot, so
there is one fewer test to check.

https://codereview.appspot.com/230860044/diff/20001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/230860044/diff/20001/scm/define-grobs.scm#newc...
scm/define-grobs.scm:1736: (X-extent . (0 . 0))
It looks like parentheses don't get horizontal space, so there seems to be no
need for a pure-height.

https://codereview.appspot.com/230860044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/230860044/diff/20001/scm/output-lib.scm#newcod...
scm/output-lib.scm:908: (define-public (parentheses-item::pure-y-extent grob
start end)
On 2015/04/21 18:46:38, Carl wrote:
> On 2015/04/19 19:32:29, Keith wrote:
> > 
> > The only need I could see for pure-y-extent here, would be to estimate
> > horizontal space for the parentheses around an accent outside a slur or
beam.
> 
> So is it best to remove the code, since we don't have a regtest that exercises
> it?

I agree, we should not add an unused function.

[Carl, 2015-04-22 21:50:54]

Respond to Keith's comments.  Eliminate pure-Y-extent. Clean up regression test.

[Carl, 2015-04-22 21:51:31]

Respond to Keith's comments.  Eliminate pure-Y-extent.  Clean up regression test