Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(441)

Issue 340650043: Issue 5301: Refactor mark events ...

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by Dan Eble
Modified:
3 months, 1 week ago
Reviewers:
dak, thomasmorley651
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -25 lines) Patch
M input/regression/display-lily-tests.ly View 1 chunk +3 lines, -3 lines 0 comments Download
M lily/mark-engraver.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M ly/music-functions-init.ly View 1 chunk +10 lines, -4 lines 4 comments Download
M scm/c++.scm View 1 chunk +3 lines, -0 lines 1 comment Download
M scm/define-context-properties.scm View 1 chunk +2 lines, -2 lines 0 comments Download
M scm/define-event-classes.scm View 2 chunks +3 lines, -0 lines 1 comment Download
M scm/define-music-display-methods.scm View 1 chunk +10 lines, -4 lines 0 comments Download
M scm/define-music-properties.scm View 1 chunk +1 line, -1 line 1 comment Download
M scm/define-music-types.scm View 4 chunks +25 lines, -9 lines 3 comments Download

Messages

Total messages: 4
Dan Eble
3 months, 1 week ago (2018-04-06 02:02:18 UTC) #1
thomasmorley651
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 ...
3 months, 1 week ago (2018-04-08 23:34:02 UTC) #2
dak
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?)) On 2018/04/08 23:34:01, thomasmorley651 wrote: > ...
3 months, 1 week ago (2018-04-09 06:18:44 UTC) #3
Dan Eble
3 months, 1 week ago (2018-04-09 22:30:20 UTC) #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b