Issue 318890043: unfoldRepeats can be restricted to certain repeat-types

Rietveld

Please review
Hi Harm,

thanks for the patch.

As for the user-interface, I’d prefer something like

\unfoldRepeats tremolo \m
\unfoldRepeats volta,tremolo \m

IIUC, that would require using predicate symbol-or-symbol-list? for the 
optional argument. (I hope that works… didn’t test TBH) Also, I think we 
shouldn’t require the user to give the technical names for the music 
types, but just the term as used with \repeat.

Best, Simon

On 01.12.2016 23:02, thomasmorley65@gmail.com wrote:
> Reviewers: ,
>
> Message:
> Please review
>
> Description:
> unfoldRepeats can be restricted to certain repeat-types
>
> It now takes an additional, optional argument.
> It's a list of symbols representing the repeat-types which should
> be unfolded.
> Possible settings are percent-repeated-music, tremolo-repeated-music,
> volta-repeated-music and of course repeated-music which is the
> default and will unfold all.
> A new regtest is added to cover the new feature.
> Changes is extended accordingly.
>
> Also correcting two small typos in Documentation/notation/staff.itely
>
> Please review this at https://codereview.appspot.com/318890043/
>
> Affected files (+73, -20 lines):
>   M Documentation/changes.tely
>   M Documentation/notation/staff.itely
>   A input/regression/repeat-unfold-partial.ly
>   M ly/music-functions-init.ly
>   M scm/music-functions.scm
>
>
> Index: Documentation/changes.tely
> diff --git a/Documentation/changes.tely b/Documentation/changes.tely
> index 
>
2407b3d018f3c664b9e6c387b04647684db2cea4..a085eb57a2ac72bd0135ac0c58e573715132d4a2

> 100644
> --- a/Documentation/changes.tely
> +++ b/Documentation/changes.tely
> @@ -62,6 +62,13 @@ which scares away people.
>  @end ignore
>
>  @item
> +The music function @code{\\unfoldRepeats} can now take an
> +optional argument-list specifying which type(s) of repeated music
> +should be unfolded.  Possible entries are @code{percent-repeated-music},
> +@code{tremolo-repeated-music}, @code{volta-repeated-music} and of course
> +the general @code{repeated-music} which is the default and will 
> unfold all.
> +
> +@item
>  A new @code{output-attributes} grob property is now used for svg output
>  instead of the @code{id} grob property.  It allows multiple attributes
>  to be defined as an association list.  For example, @code{#'((id . 123)
> Index: Documentation/notation/staff.itely
> diff --git a/Documentation/notation/staff.itely 
> b/Documentation/notation/staff.itely
> index 
>
a1144346644864b8298f30a4fe6e9df2423c9078..d097535643b0bb9a4f6e6841755b11c07b65fa5c

> 100644
> --- a/Documentation/notation/staff.itely
> +++ b/Documentation/notation/staff.itely
> @@ -1078,9 +1078,9 @@ oboeNotes = \relative {
>  }
>  @end lilypond
>
> -If an @code{\unfoldRepeat} command in a music expression is required to
> +If an @code{\unfoldRepeats} command in a music expression is required to
>  be printed when using @code{\quoteDuring}, then it too must also contain
> -its own @code{\unfoldRepeat} command;
> +its own @code{\unfoldRepeats} command;
>
>  @lilypond[verbatim,quote]
>  fluteNotes = \relative {
> Index: input/regression/repeat-unfold-partial.ly
> diff --git a/input/regression/repeat-unfold-partial.ly 
> b/input/regression/repeat-unfold-partial.ly
> new file mode 100644
> index 
>
0000000000000000000000000000000000000000..cb6d05627ca3f22c8f376a3adc1d7e14c5519783
> --- /dev/null
> +++ b/input/regression/repeat-unfold-partial.ly
> @@ -0,0 +1,37 @@
> +\header {
> +  texidoc = "The music function @code{\\unfoldRepeats} can take an
> +optional argument-list specifying which type(s) of repeated music has
> +to be unfolded."
> +}
> +
> +m =
> +  \repeat volta 2 {
> +      \repeat percent 2 { c'1 }
> +      \repeat tremolo 4 { c'16 d' }
> +      f'2
> +  }
> +  \alternative {
> +      { d'1 }
> +      { e'1 }
> +  }
> +
> +\markup "not expanding"
> +\m
> +
> +\markup "expanding all"
> +\unfoldRepeats \m
> +
> +\markup "expanding percent-repeated-music"
> +\unfoldRepeats #'(percent-repeated-music) \m
> +
> +\markup "expanding tremolo-repeated-music"
> +\unfoldRepeats #'(tremolo-repeated-music) \m
> +
> +\markup "expanding volta-repeated-music"
> +\unfoldRepeats #'(volta-repeated-music) \m
> +
> +\markup \column {
> +  "combinations are possible:"
> +  "expanding percent-repeated-music and tremolo-repeated-music"
> +}
> +\unfoldRepeats #'(percent-repeated-music tremolo-repeated-music) \m
> Index: ly/music-functions-init.ly
> diff --git a/ly/music-functions-init.ly b/ly/music-functions-init.ly
> index 
>
d96e655370612f6e107ac3259df3a4c066071fd1..b6b10e98fd12c28e927af43c9791cd0728560628

> 100644
> --- a/ly/music-functions-init.ly
> +++ b/ly/music-functions-init.ly
> @@ -1976,11 +1976,17 @@ unsets already in @var{music} cause a 
> warning.  Non-property-related music is ig
>          (else (make-sequential-music lst))))))
>
>  unfoldRepeats =
> -#(define-music-function (music) (ly:music?)
> -   (_i "Force any @code{\\repeat volta}, @code{\\repeat tremolo} or
> +#(define-music-function (types music)
> +   ((symbol-list? '(repeated-music)) ly:music?)
> +   (_i "Force @code{\\repeat volta}, @code{\\repeat tremolo} or
>  @code{\\repeat percent} commands in @var{music} to be interpreted
> -as @code{\\repeat unfold}.")
> -   (unfold-repeats music))
> +as @code{\\repeat unfold}, if specified in the optional symbol-list 
> @var{types},
> +which defaults to @code{'(repeated-music)}, which will force any of 
> those
> +commands in @var{music} to be interpreted as @code{\\repeat unfold}.
> +Possible other entries are @code{volta-repeated-music},
> +@code{tremolo-repeated-music} or @code{percent-repeated-music}.
> +Multiple entries are possible.")
> +   (unfold-repeats types music))
>
>  void =
>  #(define-void-function (arg) (scheme?)
> Index: scm/music-functions.scm
> diff --git a/scm/music-functions.scm b/scm/music-functions.scm
> index 
>
3e9818d0093b48c1d0072dbfe3d7fa2d97efcf18..7a7fdf293ea8dd16057df0fea8e6d6485eddc7fc

> 100644
> --- a/scm/music-functions.scm
> +++ b/scm/music-functions.scm
> @@ -390,19 +390,22 @@ beats to be distinguished."
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ;; repeats.
>
> -(define-public (unfold-repeats music)
> -  "Replace all repeats with unfolded repeats."
> -  (let ((es (ly:music-property music 'elements))
> -        (e (ly:music-property music 'element)))
> -    (if (music-is-of-type? music 'repeated-music)
> -        (set! music (make-music 'UnfoldedRepeatedMusic music)))
> -    (if (pair? es)
> -        (set! (ly:music-property music 'elements)
> -              (map unfold-repeats es)))
> -    (if (ly:music? e)
> -        (set! (ly:music-property music 'element)
> -              (unfold-repeats e)))
> -    music))
> +(define-public (unfold-repeats types music)
> +  "Replace repeats of the types given by @var{types} with unfolded 
> repeats."
> +  (for-each
> +    (lambda (type)
> +      (let ((es (ly:music-property music 'elements))
> +            (e (ly:music-property music 'element)))
> +        (if (music-is-of-type? music type)
> +            (set! music (make-music 'UnfoldedRepeatedMusic music)))
> +        (if (pair? es)
> +            (set! (ly:music-property music 'elements)
> +                  (map (lambda (x) (unfold-repeats types x)) es)))
> +        (if (ly:music? e)
> +            (set! (ly:music-property music 'element)
> +                  (unfold-repeats types e)))))
> +    types)
> +  music)
>
>  (define-public (unfold-repeats-fully music)
>    "Unfolds repeats and expands the resulting 
> @code{unfolded-repeated-music}."
> @@ -411,7 +414,7 @@ beats to be distinguished."
>       (and (music-is-of-type? m 'unfolded-repeated-music)
>            (make-sequential-music
>             (ly:music-deep-copy (make-unfolded-set m)))))
> -   (unfold-repeats music)))
> +   (unfold-repeats '(repeated-music) music)))
>
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ;; property setting music objs.
>
>
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel

Simon Albrecht <simon.albrecht@mail.de> writes:

> Hi Harm,
>
> thanks for the patch.
>
> As for the user-interface, I’d prefer something like
>
> \unfoldRepeats tremolo \m
> \unfoldRepeats volta,tremolo \m
>
> IIUC, that would require using predicate symbol-or-symbol-list? for
> the optional argument.

I don't think so: LilyPond will try a symbol list when a symbol doesn't
work.  However, people might get surprised by

#(unfoldRepeats 'tremolo m)

not working.  So it's probably less surprising to use
symbol-or-symbol-list? overall.

> (I hope that works… didn’t test TBH) Also, I think we shouldn’t
> require the user to give the technical names for the music types, but
> just the term as used with \repeat.

As a user interface, that sounds very much desirable.  I don't like
converting symbols to strings, putting stuff on them, and converting
them back again.

But it's probably the lesser evil.

-- 
David Kastrup

Improve user-interface
On 2016/12/01 22:42:38, simon.albrecht wrote:
> Hi Harm,
> 
> thanks for the patch.
> 
> As for the user-interface, I’d prefer something like
> 
> \unfoldRepeats tremolo \m
> \unfoldRepeats volta,tremolo \m

Done.
Thanks for review.
On 2016/12/02 19:45:02, dak wrote:
> However, people might get surprised by
> 
> #(unfoldRepeats 'tremolo m)
> 
> not working. 

Btw, it turned out the previous unfold-repeats destructively changed the
music-argument, if called directly. See:

\version "2.19.51"

m =
  \repeat volta 2 { 
  	  \repeat percent 2 { c'1 } 
  	  \repeat tremolo 4 { c'16 d' } 
  	  f'2
  }
  \alternative {
  	  { d'1 }
  	  { e'1 }
  }

\new Staff \m
\new Staff $(unfold-repeats m)
\new Staff \m

Using ly:music-deep-copy cures it.
thomasmorley65@gmail.com writes:

> On 2016/12/02 19:45:02, dak wrote:
>> However, people might get surprised by
>
>> #(unfoldRepeats 'tremolo m)
>
>> not working.
>
> Btw, it turned out the previous unfold-repeats destructively changed the
> music-argument, if called directly.

That's common for LilyPond functions.

-- 
David Kastrup

On 2016/12/03 11:28:24, dak wrote:
> mailto:thomasmorley65@gmail.com writes:
> 
> > On 2016/12/02 19:45:02, dak wrote:
> >> However, people might get surprised by
> >
> >> #(unfoldRepeats 'tremolo m)
> >
> >> not working.
> >
> > Btw, it turned out the previous unfold-repeats destructively changed the
> > music-argument, if called directly.
> 
> That's common for LilyPond functions.
> 
> -- 
> David Kastrup

Ok.
Should I remove the call for ly:music-deep-copy?
David Kastrup <dak@gnu.org> writes:

> As a user interface, that sounds very much desirable.  I don't like
> converting symbols to strings, putting stuff on them, and converting
> them back again.
>
> But it's probably the lesser evil.

Actually, use an alist or a hash internally for mapping
tremolo,volta,whatever to a music type even though it looks like string
manipulation might do the trick.

That way you can also get #f and put out an error instead of creating an
illegal music type.

And the result is again reasonably efficient.

-- 
David Kastrup

thomasmorley65@gmail.com writes:

> On 2016/12/03 11:28:24, dak wrote:
>> mailto:thomasmorley65@gmail.com writes:
>
>> > On 2016/12/02 19:45:02, dak wrote:
>> >> However, people might get surprised by
>> >
>> >> #(unfoldRepeats 'tremolo m)
>> >
>> >> not working.
>> >
>> > Btw, it turned out the previous unfold-repeats destructively changed
> the
>> > music-argument, if called directly.
>
>> That's common for LilyPond functions.
>
>> --
>> David Kastrup
>
> Ok.
> Should I remove the call for ly:music-deep-copy?

In the function, I think that is only consistent with what we do and
expect elsewhere.  Of course, that might mean you have to review its
callers to make sure they don't get tripped up by that.

-- 
David Kastrup

On 2016/12/03 11:33:15, dak wrote:
> David Kastrup <mailto:dak@gnu.org> writes:
> 
> > As a user interface, that sounds very much desirable.  I don't like
> > converting symbols to strings, putting stuff on them, and converting
> > them back again.
> >
> > But it's probably the lesser evil.
> 
> Actually, use an alist or a hash internally for mapping

Much better

> tremolo,volta,whatever to a music type even though it looks like string
> manipulation might do the trick.
> 
> That way you can also get #f and put out an error instead of creating an
> illegal music type.
> 
> And the result is again reasonably efficient.

Efficiency is one of my concerns as well.

For now I'll change the issue to needs_work (I've found another oversight ...)
so James doesn't need to test.
On 2016/12/03 11:34:48, dak wrote:
> mailto:thomasmorley65@gmail.com writes:
> 
> > On 2016/12/03 11:28:24, dak wrote:
> >> mailto:thomasmorley65@gmail.com writes:
> >
> >> > On 2016/12/02 19:45:02, dak wrote:
> >> >> However, people might get surprised by
> >> >
> >> >> #(unfoldRepeats 'tremolo m)
> >> >
> >> >> not working.
> >> >
> >> > Btw, it turned out the previous unfold-repeats destructively changed
> > the
> >> > music-argument, if called directly.
> >
> >> That's common for LilyPond functions.
> >
> >> --
> >> David Kastrup
> >
> > Ok.
> > Should I remove the call for ly:music-deep-copy?
> 
> In the function, I think that is only consistent with what we do and
> expect elsewhere.  Of course, that might mean you have to review its
> callers to make sure they don't get tripped up by that.
> 
> -- 
> David Kastrup

Will do.
Thanks for review.
Use hash-table, fix articulate.ly
On 2016/12/03 11:33:15, dak wrote:
> David Kastrup <mailto:dak@gnu.org> writes:
> 
> > As a user interface, that sounds very much desirable.  I don't like
> > converting symbols to strings, putting stuff on them, and converting
> > them back again.
> >
> > But it's probably the lesser evil.
> 
> Actually, use an alist or a hash internally for mapping
> tremolo,volta,whatever to a music type even though it looks like string
> manipulation might do the trick.
> 
> That way you can also get #f and put out an error instead of creating an
> illegal music type.
> 
> And the result is again reasonably efficient.
> 
> -- 
> David Kastrup

Done.
Now a hash-table is used.
I don't think an error is needed, a warning should be sufficient.

Issue 318890043: unfoldRepeats can be restricted to certain repeat-types (Closed)

Description

Patch Set 1 #

Patch Set 2 : Improve user-interface #

Patch Set 3 : Use hash-table, fix articulate.ly #

Messages