Issue 4465: Auto_change_iterator: move staff creation to scheme

Auto_change_iterator no longer creates staff contexts itself.  They are created in scheme.

[Dan Eble, 2015-06-27 00:45:14]

...

[Dan Eble, 2015-06-27 00:48:23]

...

[Dan Eble, 2015-06-27 00:48:47]

...

[Dan Eble, 2015-06-27 00:55:52]

...

[Dan Eble, 2015-06-27 00:58:04]

...

[Dan Eble, 2015-06-27 01:02:17]

...

[Dan Eble, 2015-06-27 18:54:03]

https://codereview.appspot.com/249970043/diff/1/scm/define-context-properties...
File scm/define-context-properties.scm (left):

https://codereview.appspot.com/249970043/diff/1/scm/define-context-properties...
scm/define-context-properties.scm:179: (bassStaffProperties ,list? "An alist of
property settings to
It would be responsible of me to draw attention to this.  I have simply removed
bassStaffProperties and trebleStaffProperties.  The NR section on \autochange
does not mention them, so I concluded that they were intended for internal use
only.  I hope there are no problems with this.

[thomasmorley651, 2015-06-27 19:45:12]

https://codereview.appspot.com/249970043/diff/1/scm/define-context-properties...
File scm/define-context-properties.scm (left):

https://codereview.appspot.com/249970043/diff/1/scm/define-context-properties...
scm/define-context-properties.scm:179: (bassStaffProperties ,list? "An alist of
property settings to
On 2015/06/27 18:54:03, Dan Eble wrote:
> It would be responsible of me to draw attention to this.  I have simply
removed
> bassStaffProperties and trebleStaffProperties.  The NR section on \autochange
> does not mention them, so I concluded that they were intended for internal use
> only.  I hope there are no problems with this.


Thanks pointing to it again. I already stumbled upon it, though, maybe I'd have
forgotten to reply.


bassStaffProperties and trebleStaffProperties are not really documented, though
I'd call it more a documentation issue.

There are use-cases thinkable, like:

\new PianoStaff 
\with {
  trebleStaffProperties = 
    #'((assign clefGlyph "clefs.C")	
       (assign clefPosition 0)
       (assign middleCPosition -5)
       (assign middleCClefPosition -5))
}

{
  \autochange {
    g4 a b c'
    d'4 r a g
  }
}

I vote for keeping them, although I think it's a very cumbersome method to
specify which clefs the PianoStaff should use.

[dak, 2015-06-27 20:26:10]

On 2015/06/27 19:45:12, thomasmorley651 wrote:
>
https://codereview.appspot.com/249970043/diff/1/scm/define-context-properties...
> File scm/define-context-properties.scm (left):
> 
>
https://codereview.appspot.com/249970043/diff/1/scm/define-context-properties...
> scm/define-context-properties.scm:179: (bassStaffProperties ,list? "An alist
of
> property settings to
> On 2015/06/27 18:54:03, Dan Eble wrote:
> > It would be responsible of me to draw attention to this.  I have simply
> removed
> > bassStaffProperties and trebleStaffProperties.  The NR section on
\autochange
> > does not mention them, so I concluded that they were intended for internal
use
> > only.  I hope there are no problems with this.
> 
> 
> Thanks pointing to it again. I already stumbled upon it, though, maybe I'd
have
> forgotten to reply.
> 
> 
> bassStaffProperties and trebleStaffProperties are not really documented,
though
> I'd call it more a documentation issue.
> 
> There are use-cases thinkable, like:
> 
> \new PianoStaff 
> \with {
>   trebleStaffProperties = 
>     #'((assign clefGlyph "clefs.C")	
>        (assign clefPosition 0)
>        (assign middleCPosition -5)
>        (assign middleCClefPosition -5))
> }
> 
> {
>   \autochange {
>     g4 a b c'
>     d'4 r a g
>   }
> }
> 
> I vote for keeping them, although I think it's a very cumbersome method to
> specify which clefs the PianoStaff should use.

guile> (ly:get-context-mods #{ \with { \clef treble } #})
((assign clefGlyph "clefs.G") (assign middleCClefPosition -6) (assign
clefPosition -2) (assign clefTransposition 0) (assign clefTranspositionStyle
default) (apply #<primitive-procedure ly:set-middle-C!>))
guile> 

So maybe \autochange \with { \clef treble } \with { \clef bass } { ... }

[Dan Eble, 2015-06-28 00:17:58]

On 2015/06/27 19:45:12, thomasmorley651 wrote:
> bassStaffProperties and trebleStaffProperties are not really documented,
though
> I'd call it more a documentation issue.
...
> I vote for keeping them, although I think it's a very cumbersome method to
> specify which clefs the PianoStaff should use.

If one follows the NR's suggestion to instantiate staves explicitly, your
example is transformed into something more lilypondish (below).  Removing these
properties will make it easier for me to consolidate some code for \autochange
and \partcombine.

\version "2.19.0"

\score {
\new PianoStaff <<
  \context Staff = "up" \with {
    clefGlyph = "clefs.C"
    clefPosition = 0
    middleCPosition = -5
    middleCClefPosition = -5
  } {
    \new Voice = "melOne" {
      \autochange {
	g4 a b c'
	d'4 r a g
      }
    }
  }
  >>
}

[thomasmorley651, 2015-06-28 11:17:58]

On 2015/06/28 00:17:58, Dan Eble wrote:
> On 2015/06/27 19:45:12, thomasmorley651 wrote:
> > bassStaffProperties and trebleStaffProperties are not really documented,
> though
> > I'd call it more a documentation issue.
> ...
> > I vote for keeping them, although I think it's a very cumbersome method to
> > specify which clefs the PianoStaff should use.
> 
> If one follows the NR's suggestion to instantiate staves explicitly, your
> example is transformed into something more lilypondish (below).  Removing
these
> properties will make it easier for me to consolidate some code for \autochange
> and \partcombine.

Let me clearify, I don't insist in keeping those properties, though I'd like to
keep the possibility to simply write \autochange { ... } _and_ to have the
possibility to set the clefs in both staves.
The NR shows an example for \autochange { ... } and I think the
treble/bassStaffProperties were meant to set the clefs.

Following Davids suggestion, why not:

%% begin snippet

\version "2.19.22"

%% with patch applied

%% optional context-modifications working only, if contexts are not instantiated

%% explicitly
autochange =
#(define-music-function (cm-1 cm-2 music) 
  ((ly:context-mod? #{ \with { \clef treble} #})
   (ly:context-mod? #{ \with { \clef bass } #})
   ly:music?)
  (_i "Make voices that switch between staves automatically")
  (let ;; keep the contexts alive for the full duration
       ((skip (make-skip-music (make-duration-of-length
                                     (ly:music-length music)))))
    #{
      <<
        \context Staff = "up" $cm-1
          <<
          #(make-autochange-music music)
          \new Voice { #skip }
          >>
        \context Staff = "down" $cm-2
          \new Voice { #skip } 
      >>
    #}))

%% TESTS
    
music = {
  g4 a b c'
  d'4 r a g
}

\autochange \music
\autochange \with { \clef alto } \music
\autochange \with { \clef alto } \with { \clef tenor }\music

%% end snippet

No need for treble/bassStaffProperties anymore

[Dan Eble, 2015-06-28 12:30:21]

On 2015/06/28 11:17:58, thomasmorley651 wrote:
>
> Let me clearify, I don't insist in keeping those properties, though I'd like
to
> keep the possibility to simply write \autochange { ... } _and_ to have the
> possibility to set the clefs in both staves.

What do you think about putting that enhancement into a ticket of its own?  My
only motivation to touch \autochange is to remove what I see as unnecessary
duplication in the innards of \partcombine and \autochange.

> The NR shows an example for \autochange { ... } and I think the
> treble/bassStaffProperties were meant to set the clefs.

Maybe, but the split point is hard-coded to middle C, isn't it?  So that makes
it a little weird with other clefs.  (Of course that could be fixed as well.)

[dak, 2015-06-28 12:49:00]

On 2015/06/28 12:30:21, Dan Eble wrote:
> On 2015/06/28 11:17:58, thomasmorley651 wrote:
> >
> > Let me clearify, I don't insist in keeping those properties, though I'd like
> to
> > keep the possibility to simply write \autochange { ... } _and_ to have the
> > possibility to set the clefs in both staves.
> 
> What do you think about putting that enhancement into a ticket of its own?  My
> only motivation to touch \autochange is to remove what I see as unnecessary
> duplication in the innards of \partcombine and \autochange.
> 
> > The NR shows an example for \autochange { ... } and I think the
> > treble/bassStaffProperties were meant to set the clefs.
> 
> Maybe, but the split point is hard-coded to middle C, isn't it?  So that makes
> it a little weird with other clefs.  (Of course that could be fixed as well.)

Well, the fully general version would likely be something like
\changeOn e' { \context Voice = "high" \with { \clef treble } {} \change Voice =
"high" }
          bes { \context Voice = "low" \with { \clef bass } {} \change Voice =
"low" }
             { ...music-to-autochange... }

Namely change up when hitting more than e' and down when going below bes. 
Something like that.

[thomasmorley651, 2015-06-29 08:54:18]

On 2015/06/28 12:30:21, Dan Eble wrote:
> On 2015/06/28 11:17:58, thomasmorley651 wrote:
> >
> > Let me clearify, I don't insist in keeping those properties, though I'd like
> to
> > keep the possibility to simply write \autochange { ... } _and_ to have the
> > possibility to set the clefs in both staves.
> 
> What do you think about putting that enhancement into a ticket of its own?  My
> only motivation to touch \autochange is to remove what I see as unnecessary
> duplication in the innards of \partcombine and \autochange.

Ok with me, if it speeds up your work on /partcombine.
I did a search in the user-archives for bassStaffProperties and
trebleStaffProperties, result: zero.
So it will do no real harm, undocumented features tend to be effectively
non-existent.

More, I'd offer to create an issue and provide a patch for it. I've already a
sketch of a working patch locally. Though, it needs to touch autochange.scm as
well. Regarding your other patches affecting autochange.scm I'd like to postpone
it, until they are though.

> > The NR shows an example for \autochange { ... } and I think the
> > treble/bassStaffProperties were meant to set the clefs.
> 
> Maybe, but the split point is hard-coded to middle C, isn't it?  So that makes
> it a little weird with other clefs.  (Of course that could be fixed as well.)

Ofcourse that should be fixed then!

[dak, 2015-07-02 10:45:49]

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

https://codereview.appspot.com/249970043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:189: \context Staff = "down" \with {
Why not \with { \clef "bass" } instead?  Should not squeeze this kind of change
without Patchy, but at least consider it for the next iteration of your ongoing
work.

[Dan Eble, 2015-07-02 12:15:37]

On 2015/07/02 10:45:49, dak wrote:
>
https://codereview.appspot.com/249970043/diff/1/ly/music-functions-init.ly#ne...
> ly/music-functions-init.ly:189: \context Staff = "down" \with {
> Why not \with { \clef "bass" } instead?  Should not squeeze this kind of
change
> without Patchy, but at least consider it for the next iteration of your
ongoing
> work.

(sigh) Yes; good advice is always appreciated.  Since Thomas already offered to
add the optional context mod arguments to \autochange, maybe he would be willing
to act on this suggestion at the same time.  Thomas?

[thomasmorley651, 2015-07-02 12:21:57]

On 2015/07/02 12:15:37, Dan Eble wrote:
> On 2015/07/02 10:45:49, dak wrote:
> >
>
https://codereview.appspot.com/249970043/diff/1/ly/music-functions-init.ly#ne...
> > ly/music-functions-init.ly:189: \context Staff = "down" \with {
> > Why not \with { \clef "bass" } instead?  Should not squeeze this kind of
> change
> > without Patchy, but at least consider it for the next iteration of your
> ongoing
> > work.
> 
> (sigh) Yes; good advice is always appreciated.  Since Thomas already offered
to
> add the optional context mod arguments to \autochange, maybe he would be
willing
> to act on this suggestion at the same time.  Thomas?

I'll do, if it's ok for David.
Though, as said before I'll postpone it until your other patches for
autochangescm are through