Adds support for cross-staff-stems

Included the regtest and the snippet on this patch, and added texi-string for the regtest.  Hope this is good to go.

[PhilEHolmes, 2012-07-09 14:39:08]

Please review.

[Trevor Daniels, 2012-07-09 21:38:13]

I don't think all these functions should be in ly/music-functions-init.ly, but I
don't know the best place for them.  Someone more familiar with the layout of
code may advise.  The one function that should be here is the one you asked
about in your earlier mail - crossStaff.

Trevor

http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross...
File Documentation/snippets/new/cross-staff-stems.ly (right):

http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross...
Documentation/snippets/new/cross-staff-stems.ly:19: <<
<< and matching >> are not required; remove

http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross...
Documentation/snippets/new/cross-staff-stems.ly:22: <<
Only one voice, better to use { .. }

http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross...
Documentation/snippets/new/cross-staff-stems.ly:31: <<
Only one voice, better to use { .. }

http://codereview.appspot.com/6344092/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/6344092/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:327: #(define-music-function (parser location notes)
(ly:music?) #{
As I said earlier, this function needs a doc string

[email_philholmes.net, 2012-07-10 11:06:37]

----- Original Message ----- 
From: <tdanielsmusic@googlemail.com>
To: <PhilEHolmes@googlemail.com>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Monday, July 09, 2012 10:38 PM
Subject: Re: Adds support for cross-staff-stems (issue 6344092)


>I don't think all these functions should be in
> ly/music-functions-init.ly, but I don't know the best place for them.
> Someone more familiar with the layout of code may advise.  The one
> function that should be here is the one you asked about in your earlier
> mail - crossStaff.
>
> Trevor


Thanks for your comments, Trevor.  I'll await others before putting up a new 
patch. I agree with you about putting all those functions in 
music-functions-init.ly but also can't find a better home.  That was part of 
the reason for making a patch and submitting it for review.  Unless a better 
location is identified as part of the review, I think the functions will be 
"reviewed into" music-functions-init.ly.

--
Phil Holmes 

[Graham Percival, 2012-07-13 07:20:54]

http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross...
File Documentation/snippets/new/cross-staff-stems.ly (right):

http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross...
Documentation/snippets/new/cross-staff-stems.ly:5: texidoc = "This file
demonstrates a scheme engraver that
could you also add this file as a regtest?  Just remove the lsrtags and maybe
doctitle, then dump it in input/regression/.  The (almost) exact duplicate file
is fine.

[PhilEHolmes, 2012-07-13 11:38:46]

Please review

[Graham Percival, 2012-07-14 01:39:02]

http://codereview.appspot.com/6344092/diff/7001/input/regression/cross-staff-...
File input/regression/cross-staff-stems.ly (right):

http://codereview.appspot.com/6344092/diff/7001/input/regression/cross-staff-...
input/regression/cross-staff-stems.ly:2: 
Sorry, I was unclear.  I meant to ask for your new snippet to be a regtest in
*addition* to a snippet, not as a replacement.  We do not expect users to look
at regtests, so it's important to discuss this functionality in a snippet or the
docs.

In addition, the regtest should still have a texidoc header (like other
regtests) -- I meant to only ask you to remove the lsrtags and doctitle from the
regtest version of the snippet.

[email_philholmes.net, 2012-07-14 10:46:06]

----- Original Message ----- 
From: <graham@percival-music.ca>
To: <PhilEHolmes@googlemail.com>; <tdanielsmusic@googlemail.com>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Saturday, July 14, 2012 2:39 AM
Subject: Re: Adds support for cross-staff-stems (issue 6344092)


>
>
http://codereview.appspot.com/6344092/diff/7001/input/regression/cross-staff-...
> File input/regression/cross-staff-stems.ly (right):
>
>
http://codereview.appspot.com/6344092/diff/7001/input/regression/cross-staff-...
> input/regression/cross-staff-stems.ly:2:
> Sorry, I was unclear.  I meant to ask for your new snippet to be a
> regtest in *addition* to a snippet, not as a replacement.  We do not
> expect users to look at regtests, so it's important to discuss this
> functionality in a snippet or the docs.
>
> In addition, the regtest should still have a texidoc header (like other
> regtests) -- I meant to only ask you to remove the lsrtags and doctitle
> from the regtest version of the snippet.
>
> http://codereview.appspot.com/6344092/

I think I intended it to remain in snippets/new - it looks like my patch 
hasn't picked it up.  I'll also add texidoc to explain the regtest's 
function.

It's my intention to update the cross-staff stems part of the NR once this 
is in master.

New patch will be created over the weekend.

--
Phil Holmes 

[PhilEHolmes, 2012-07-14 15:23:49]

Please review final updates.

[Keith, 2012-07-14 19:57:46]

LGTM.

Scheme-only patches are nice, because we don't have to book Linux to try them
out.

I would not hesitate to put this in before 2.16, because it is adds capability
without changing anything existing.

http://codereview.appspot.com/6344092/diff/12002/Documentation/snippets/new/c...
File Documentation/snippets/new/cross-staff-stems.ly (right):

http://codereview.appspot.com/6344092/diff/12002/Documentation/snippets/new/c...
Documentation/snippets/new/cross-staff-stems.ly:17: 
The demonstration is rather repetitive, and as a regression test the odd-looking
output made me think something was wrong.  I suggest

{
  \new PianoStaff <<
    \new Staff {
      <b d'>4 r d'16\> e'8. g8 r\!
    }
   \new Staff {
     \clef bass
      \voiceOne
      \autoBeamOff
      \crossStaff { <e g>4 e, g16 a8. c8} d
    }
  >>
}

http://codereview.appspot.com/6344092/diff/12002/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/6344092/diff/12002/ly/music-functions-init.ly#n...
ly/music-functions-init.ly:230: (grob-interpret-markup grob
(format-compound-time args)))
You should probably leave the indent in place.

[Graham Percival, 2012-07-15 01:58:59]

LGTM

[Trevor Daniels, 2012-07-19 13:21:07]

LGTM

Trevor

[dak, 2012-08-30 23:12:45]

http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm
File scm/music-functions.scm (right):

http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newc...
scm/music-functions.scm:1784: (if (<= 2 (length stems))
Why not use "and" here and leave out the #f below?  Then one does not hunt for
#f to figure out what happens when the condition is false.

http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newc...
scm/music-functions.scm:1801: (let ((span (ly:engraver-make-grob trans 'Stem
'())))
Why not have "root" as the cause, the last element of ly:engraver-make-grob? 
That would make the stem span tweakable.

[mail_philholmes.net, 2012-08-31 08:46:12]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <tdanielsmusic@googlemail.com>; 
<graham@percival-music.ca>; <k-ohara5a5a@oco.net>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Friday, August 31, 2012 12:12 AM
Subject: Re: Adds support for cross-staff-stems (issue 6344092)


>
> http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm
> File scm/music-functions.scm (right):
>
>
http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newc...
> scm/music-functions.scm:1784: (if (<= 2 (length stems))
> Why not use "and" here and leave out the #f below?  Then one does not
> hunt for #f to figure out what happens when the condition is false.
>
>
http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newc...
> scm/music-functions.scm:1801: (let ((span (ly:engraver-make-grob trans
> 'Stem '())))
> Why not have "root" as the cause, the last element of
> ly:engraver-make-grob?  That would make the stem span tweakable.
>
> http://codereview.appspot.com/6344092/


This patch has been approved, pushed and the Rietveld closed.  If you want 
to propose changes, it should be a new patch.

--
Phil Holmes