Second set of volta changes (Issue 2673)

The second set of proposed changes to volta syntax, posted on behalf of Arnold Theresius.  This version is updated to eliminate the regtest error found in the previous version.  Checked against regtests and clean.

[PhilEHolmes, 2012-07-24 14:05:20]

Please review

[dak, 2012-07-24 19:15:15]

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

http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly#newcode621
ly/engraver-init.ly:621: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
Trailing whitespace.

[email_philholmes.net, 2012-07-24 19:40:35]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <ianhulin44@gmail.com>; 
<graham@percival-music.ca>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Tuesday, July 24, 2012 8:15 PM
Subject: Re: Second set of volta changes (Issue 2673) (issue 6422061)


>
> http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly
> File ly/engraver-init.ly (right):
>
> http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly#newcode621
> ly/engraver-init.ly:621: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
> Trailing whitespace.

Git warned me about that, but I wasn't sure how to handle it in this 
context.  Given that each quoted string has a space between, is it OK to 
simply delete the trailing whitespace here?

> http://codereview.appspot.com/6422061/

--
Phil Holmes 

[dak, 2012-07-24 20:31:29]

On 2012/07/24 19:40:35, email_philholmes.net wrote:
> ----- Original Message ----- 
> From: <mailto:dak@gnu.org>
> To: <PhilEHolmes@googlemail.com>; <ianhulin44@gmail.com>; 
> <mailto:graham@percival-music.ca>
> Cc: <lilypond-devel@gnu.org>; <mailto:reply@codereview-hr.appspotmail.com>
> Sent: Tuesday, July 24, 2012 8:15 PM
> Subject: Re: Second set of volta changes (Issue 2673) (issue 6422061)
> 
> 
> >
> > http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly
> > File ly/engraver-init.ly (right):
> >
> > http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly#newcode621
> > ly/engraver-init.ly:621: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
> > Trailing whitespace.
> 
> Git warned me about that, but I wasn't sure how to handle it in this 
> context.  Given that each quoted string has a space between, is it OK to 
> simply delete the trailing whitespace here?

Sure, just the trailing one.  It's probably just a single space or tab.

[Keith, 2012-08-04 07:04:33]

http://codereview.appspot.com/6422061/diff/1/lily/volta-engraver.cc
File lily/volta-engraver.cc (right):

http://codereview.appspot.com/6422061/diff/1/lily/volta-engraver.cc#newcode159
lily/volta-engraver.cc:159: SCM glyph = endbar ? endbar->get_property
("glyph-name") : SCM_EOL;
This lookup might force a premature decision on whether to draw a repeat bar
(such as a |: ) before LilyPond knows if it will end up at the end of the line,
where it is normally omitted.
That might explain why the patch changes 'volta-broken-left-edge.ly'

[PhilEHolmes, 2012-08-07 13:03:42]

Please review

[Keith, 2012-08-08 06:09:09]

I don't see any more problems.

I understand the reason to create a configurable list of bar-types where the
volta bracket hooks.

I do not see any reason to move the decision on whether to draw the hook from
stencil-maker to engraver.  The engraver does not yet know where the line-breaks
will be, and the hook looks odd at the end of a line.

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

http://codereview.appspot.com/6422061/diff/8001/ly/engraver-init.ly#newcode622
ly/engraver-init.ly:622: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
trim trailing whitespace before pushing if you remember

[email_philholmes.net, 2012-08-08 08:28:32]

----- Original Message ----- 
From: <k-ohara5a5a@oco.net>
To: <PhilEHolmes@googlemail.com>; <ianhulin44@gmail.com>; <dak@gnu.org>; 
<graham@percival-music.ca>; <email@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Wednesday, August 08, 2012 7:09 AM
Subject: Re: Second set of volta changes (Issue 2673) (issue 6422061)


>I don't see any more problems.
>
> I understand the reason to create a configurable list of bar-types where
> the volta bracket hooks.
>
> I do not see any reason to move the decision on whether to draw the hook
> from stencil-maker to engraver.  The engraver does not yet know where
> the line-breaks will be, and the hook looks odd at the end of a line.
>
>
> http://codereview.appspot.com/6422061/diff/8001/ly/engraver-init.ly
> File ly/engraver-init.ly (right):
>
>
http://codereview.appspot.com/6422061/diff/8001/ly/engraver-init.ly#newcode622
> ly/engraver-init.ly:622: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
> trim trailing whitespace before pushing if you remember
>
> http://codereview.appspot.com/6422061/

I didn't trim the whitespace because I wasn't sure of the effect in a list 
like this.  Will it definitely have no effect?

--
Phil Holmes 

[dak, 2012-08-08 08:35:30]

On 2012/08/08 08:28:32, email_philholmes.net wrote:
> ----- Original Message ----- 
>
> > ly/engraver-init.ly:622: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
> > trim trailing whitespace before pushing if you remember
> >
> > http://codereview.appspot.com/6422061/
> 
> I didn't trim the whitespace because I wasn't sure of the effect in a list 
> like this.  Will it definitely have no effect?

Inside of Scheme but outside of a string or character constant, one
whitespace (including newline) is as good as many.  So there is no
reason for trailing whitespace on a line.