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

Issue 5440084: Don't wrap EventChord around rhythmic events by default. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 4 months ago by dak
Modified:
11 years, 7 months ago
Reviewers:
md5i, MikeSol, carl.d.sorensen, Neil Puttock, Keith
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Don't wrap EventChord around rhythmic events by default. This changes quite a number of things and required quite a few code changes. It is obvious that there is a lot of code duplication in Lilypond. A number of regtest differences show up: most of them actually demonstrate bugs in the preexisting code base that fails to typeset things like <c-.> with the same carefulness as c-. is typeset. Since c-. is no longer treated like <c>-. this becomes obvious in a number of cases. The part combiner has several problems, the tablature code has a few, relying on string numbers getting lost by default does no longer work, the fingering engraver is affected. Displaying music obviously is no longer working; this is not a defect of the old code but rather needs work to match the new realities. All in all, things could be worse. This is merely a first sketch and should at least provide for interesting experiments.

Patch Set 1 #

Patch Set 2 : Implement default -devent-chord-wrapper #

Patch Set 3 : Some more fixes. #

Patch Set 4 : Fix balloon.ly regtest http://code.google.com/p/lilypond/issues/detail?id=2070#c7 #

Total comments: 2

Patch Set 5 : Use work on stream events and rhythmic music iterator. Still spanner and gracenote problems. #

Total comments: 3

Patch Set 6 : Apart from pitched trills, shaping up well. #

Patch Set 7 : song/festival stuff needs checking, changes in to-xml expected, rest ok #

Patch Set 8 : Should fix the song problems. #

Patch Set 9 : Allow music identifiers in chords just like music functions #

Total comments: 7

Patch Set 10 : Remove music typecheck in \endSpanners #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -21 lines) Patch
M ly/music-functions-init.ly View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -21 lines 0 comments Download

Messages

Total messages: 22
MikeSol
Hey David, This patch is way over my head, so I can't really comment on ...
12 years, 4 months ago (2011-12-02 19:50:03 UTC) #1
dak
On 2011/12/02 19:50:03, MikeSol wrote: > Hey David, > > This patch is way over ...
12 years, 4 months ago (2011-12-03 08:27:14 UTC) #2
dak
Ok, I need some pointers here. In order to make this work compatibly at the ...
12 years, 3 months ago (2011-12-22 12:09:06 UTC) #3
dak
Ok, this latest iteration is quite close to what I want to end up committing. ...
12 years, 2 months ago (2012-01-20 12:53:12 UTC) #4
md5i
http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy File lily/parser.yy (right): http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy#newcode432 lily/parser.yy:432: %type <scm> list_music I must *strongly* recommend that the ...
12 years, 2 months ago (2012-01-20 13:55:56 UTC) #5
MikeSol
http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc File lily/rhythmic-music-iterator.cc (right): http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc#newcode62 lily/rhythmic-music-iterator.cc:62: if (scm_is_true ly_lily_module_constant should only be called for things ...
12 years, 2 months ago (2012-01-20 14:08:18 UTC) #6
dak
http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy File lily/parser.yy (right): http://codereview.appspot.com/5440084/diff/7001/lily/parser.yy#newcode432 lily/parser.yy:432: %type <scm> list_music On 2012/01/20 13:55:56, md5i wrote: > ...
12 years, 2 months ago (2012-01-20 14:52:09 UTC) #7
dak
On 2012/01/20 14:08:18, MikeSol wrote: > http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc > File lily/rhythmic-music-iterator.cc (right): > > http://codereview.appspot.com/5440084/diff/11001/lily/rhythmic-music-iterator.cc#newcode62 > ...
12 years, 2 months ago (2012-01-20 14:55:38 UTC) #8
dak
On 2012/01/20 14:55:38, dak wrote: > On 2012/01/20 14:08:18, MikeSol wrote: > > > Otherwise, ...
12 years, 2 months ago (2012-01-20 15:33:07 UTC) #9
MikeSol
On 2012/01/20 15:33:07, dak wrote: > Should I be forking the rhythmic-music-iterator stuff into the ...
12 years, 2 months ago (2012-01-20 15:37:13 UTC) #10
Neil Puttock
Hi David, Should I wait for a new patch or can I test using the ...
12 years, 2 months ago (2012-01-20 16:32:12 UTC) #11
dak
n.puttock@gmail.com writes: > Hi David, > > Should I wait for a new patch or ...
12 years, 2 months ago (2012-01-20 17:33:44 UTC) #12
dak
On 2012/01/20 17:33:44, dak wrote: > mailto:n.puttock@gmail.com writes: > > > Hi David, > > ...
12 years, 2 months ago (2012-01-20 17:58:13 UTC) #13
Neil Puttock
On 20 January 2012 17:32, David Kastrup <dak@gnu.org> wrote: > n.puttock@gmail.com writes: > >> Hi ...
12 years, 2 months ago (2012-01-20 17:59:40 UTC) #14
dak
On 2012/01/20 17:59:40, Neil Puttock wrote: > I can't think of anything better than adding ...
12 years, 2 months ago (2012-01-20 22:33:16 UTC) #15
Carl
Wow -- what a lot of work. And it really seems to be cleaning things ...
12 years, 2 months ago (2012-01-20 23:02:20 UTC) #16
dak
http://codereview.appspot.com/5440084/diff/11001/scm/modal-transforms.scm File scm/modal-transforms.scm (right): http://codereview.appspot.com/5440084/diff/11001/scm/modal-transforms.scm#newcode129 scm/modal-transforms.scm:129: (define (make-scale music) On 2012/01/20 23:02:21, Carl wrote: > ...
12 years, 2 months ago (2012-01-21 07:27:24 UTC) #17
Neil Puttock
Great work David. I like how \harmonic works properly for single notes now. :) http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc ...
12 years, 2 months ago (2012-01-24 22:47:31 UTC) #18
dak
On 2012/01/24 22:47:31, Neil Puttock wrote: > Great work David. I like how \harmonic works ...
12 years, 2 months ago (2012-01-24 23:19:10 UTC) #19
dak
http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc File lily/music-scheme.cc (right): http://codereview.appspot.com/5440084/diff/14005/lily/music-scheme.cc#newcode79 lily/music-scheme.cc:79: "Is @var{obj} a proper (non-rhythmic) event object?") On 2012/01/24 ...
12 years, 2 months ago (2012-01-24 23:19:25 UTC) #20
Keith
Needs a tweak for \endSpanners http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly File ly/music-functions-init.ly (right): http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly#newcode296 ly/music-functions-init.ly:296: (if (memq (ly:music-property music ...
11 years, 7 months ago (2012-08-14 02:45:39 UTC) #21
dak
11 years, 7 months ago (2012-08-14 05:33:49 UTC) #22
On 2012/08/14 02:45:39, Keith wrote:
> Needs a tweak for \endSpanners
> 
> http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly
> File ly/music-functions-init.ly (right):
> 
>
http://codereview.appspot.com/5440084/diff/14005/ly/music-functions-init.ly#n...
> ly/music-functions-init.ly:296: (if (memq (ly:music-property music 'name)
> '(EventChord NoteEvent))
> and   RestEvent SkipEvent
> { \endSpanners r4\cresc b b b }

I lean towards removing the check altogether.  Extract-typed-music is quite more
robust here than the original code was, and the worst that can happen is that
one finishes spanners that are both started and ended already in complex music a
second time.  But that would generate a separate error/warning anyway, and a
warning much more likely to make sense than the one we are generating here.
Sign in to reply to this message.

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