Issue 5301: Refactor mark events ...

https://sourceforge.net/p/testlilyissues/issues/5301/

Split mark events into three classes:
  * default rehearsal mark
  * specific rehearsal mark
  * ad-hoc mark

In anticipation of adding a fourth class with both a symbolic label
and text markup, stop using the 'label property to hold the markup of
ad-hoc marks.  Use 'text instead.

[thomasmorley651, 2018-04-08 23:34:02]

Hi Dan,

some remarks/questions:

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

https://codereview.appspot.com/340650043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:833: #(define-music-function (label)
((integer-or-markup?))
While you're on it, shouldn't the predicate be changed?
\mark #-1 full-fills integer-or-markup? but fails later in a markup-command with
vector-ref. Same for \mark 1.0
We probably need something like
exact-index?

https://codereview.appspot.com/340650043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:840: ((integer? label)
exact-index?

https://codereview.appspot.com/340650043/diff/1/scm/c++.scm
File scm/c++.scm (right):

https://codereview.appspot.com/340650043/diff/1/scm/c++.scm#newcode53
scm/c++.scm:53: (define-public (integer-or-symbol? x)
Similar here
exact-index-or-symbol?

https://codereview.appspot.com/340650043/diff/1/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

https://codereview.appspot.com/340650043/diff/1/scm/define-event-classes.scm#...
scm/define-event-classes.scm:45: (mark-event . (ad-hoc-mark-event
rehearsal-mark-event))
Am I right that something like:

foo =
#(define-music-function (mus) (ly:music?)
(music-map
  (lambda (m)
    (if (music-is-of-type? m 'mark-event)
        (begin
          (display-scheme-music m)
          m)
        m))
    mus))
    
\foo
{
  \mark \default
  r1
}

will still catch everything invoked by { \mark ... }

While changing it to 
    (if (music-is-of-type? m 'rehearsal-mark-event)
only not-text-marks will be found?

https://codereview.appspot.com/340650043/diff/1/scm/define-music-properties.scm
File scm/define-music-properties.scm (right):

https://codereview.appspot.com/340650043/diff/1/scm/define-music-properties.s...
scm/define-music-properties.scm:111: (label ,integer-or-symbol? "Sequence number
or name of a mark.")
Same here: exact-index-or-markup?

https://codereview.appspot.com/340650043/diff/1/scm/define-music-types.scm
File scm/define-music-types.scm (right):

https://codereview.appspot.com/340650043/diff/1/scm/define-music-types.scm#ne...
scm/define-music-types.scm:41: (types . (ad-hoc-mark-event event))

Shouldn't mark-event be added to the list of types?

https://codereview.appspot.com/340650043/diff/1/scm/define-music-types.scm#ne...
scm/define-music-types.scm:199: (types . (default-rehearsal-mark-event event))

Same here.

https://codereview.appspot.com/340650043/diff/1/scm/define-music-types.scm#ne...
scm/define-music-types.scm:615: (types . (specific-rehearsal-mark-event event))

And here.

[dak, 2018-04-09 06:18:44]

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

https://codereview.appspot.com/340650043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:833: #(define-music-function (label)
((integer-or-markup?))
On 2018/04/08 23:34:01, thomasmorley651 wrote:
> While you're on it, shouldn't the predicate be changed?
> \mark #-1 full-fills integer-or-markup? but fails later in a markup-command
with
> vector-ref. Same for \mark 1.0
> We probably need something like
> exact-index?

If exactness is required for indexing, we should fold it into the index?
predicate.

[Dan Eble, 2018-04-09 22:30:20]

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

https://codereview.appspot.com/340650043/diff/1/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:833: #(define-music-function (label)
((integer-or-markup?))
Alternatively, one could blame the mark formatter (or whatever it is) for
failing gracelessly given a value that satisfies the predicate; but if nobody
cares to argue that negative mark numbers might be valuable to somebody, I am
willing to use index? in this patch.

Using index? would still require the formatters to handle zero gracefully.  That
seems reasonable to me, but if there are other opinions, please express them. 
Either way, I do not want to revamp the formatters now.

David makes a good point that "if exactness is required for indexing, we should
fold it into the index? predicate," but I don't want to devote time to
investigating his predicate, and I don't want to risk side effects by mixing
such a change into my patch without investigation.