Re: Issue 5301: Refactor mark events ... (issue 340650043 by address@hidden)

[thomasmorley65]

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#newcode833
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#newcode840
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#newcode45
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.scm#newcode111
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#newcode41
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#newcode199
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#newcode615
scm/define-music-types.scm:615: (types . (specific-rehearsal-mark-event
event))

And here.

https://codereview.appspot.com/340650043/