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

Issue 321930043: Create engravers for merging rests

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by horndude77
Modified:
1 day, 13 hours ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Create engravers for merging rests This commit includes engravers for merging rests and multimeasure rests among multiple voices.

Patch Set 1 #

Patch Set 2 : Create engravers for merging rests #

Patch Set 3 : Rebased on master #

Patch Set 4 : Rebase, set tracker issue #

Total comments: 22

Patch Set 5 : Address review comments #

Patch Set 6 : Add suspendRestMerging context property #

Total comments: 6

Patch Set 7 : Make overlapped grobs invisible, update regression test for attached text, fix error #

Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -0 lines) Patch
M Documentation/notation/simultaneous.itely View 1 2 3 4 5 2 chunks +34 lines, -0 lines 0 comments Download
A input/regression/merge-rests-engraver.ly View 1 2 3 4 5 6 1 chunk +75 lines, -0 lines 0 comments Download
M scm/define-context-properties.scm View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M scm/scheme-engravers.scm View 1 2 3 4 5 6 1 chunk +75 lines, -0 lines 0 comments Download

Messages

Total messages: 20
Dan Eble
I'm currently trying to give other things priority over Lilypond, so I'm not going to ...
1 week, 1 day ago (2017-05-14 19:26:57 UTC) #1
horndude77
Create engravers for merging rests
1 week ago (2017-05-16 05:58:21 UTC) #2
horndude77
On 2017/05/14 19:26:57, Dan Eble wrote: > I'm currently trying to give other things priority ...
1 week ago (2017-05-16 06:04:34 UTC) #3
horndude77
Rebased on master
6 days, 2 hours ago (2017-05-17 06:24:16 UTC) #4
m17spear80brymer
5 days, 14 hours ago (2017-05-17 17:58:30 UTC) #5
thomasmorley651
On 2017/05/17 06:24:16, horndude77 wrote: > Rebased on master Hi Jay, this one will not ...
5 days, 10 hours ago (2017-05-17 22:01:42 UTC) #6
horndude77
Rebase, set tracker issue
5 days, 3 hours ago (2017-05-18 05:14:49 UTC) #7
pkx166h
Passes make, make check and a full make doc.
4 days, 20 hours ago (2017-05-18 11:54:08 UTC) #8
david.nalesnik
https://codereview.appspot.com/321930043/diff/60001/ly/init.ly File ly/init.ly (right): https://codereview.appspot.com/321930043/diff/60001/ly/init.ly#newcode36 ly/init.ly:36: #(use-modules (scm merge-rests-engraver)) I'm not sure why you are ...
4 days, 18 hours ago (2017-05-18 14:15:24 UTC) #9
thomasmorley651
https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm File scm/merge-rests-engraver.scm (right): https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode10 scm/merge-rests-engraver.scm:10: (define (rest-length rest) This definition is unused later and ...
4 days, 11 hours ago (2017-05-18 20:54:41 UTC) #10
horndude77
Address review comments
3 days, 17 hours ago (2017-05-19 15:34:57 UTC) #11
horndude77
https://codereview.appspot.com/321930043/diff/60001/ly/init.ly File ly/init.ly (right): https://codereview.appspot.com/321930043/diff/60001/ly/init.ly#newcode36 ly/init.ly:36: #(use-modules (scm merge-rests-engraver)) On 2017/05/18 14:15:23, david.nalesnik wrote: > ...
3 days, 3 hours ago (2017-05-20 05:18:38 UTC) #12
horndude77
Add suspendRestMerging context property
3 days, 2 hours ago (2017-05-20 06:04:39 UTC) #13
thomasmorley651
Much better now, though: https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm File scm/scheme-engravers.scm (right): https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode151 scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop) The current ...
2 days, 20 hours ago (2017-05-20 12:18:34 UTC) #14
david.nalesnik
Looks much better -- see comments below. https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm File scm/scheme-engravers.scm (right): https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode167 scm/scheme-engravers.scm:167: (lambda (rest) ...
2 days, 18 hours ago (2017-05-20 14:33:15 UTC) #15
horndude77
Make overlapped grobs invisible, update regression test for attached text, fix error
2 days, 8 hours ago (2017-05-21 00:25:37 UTC) #16
horndude77
https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm File scm/scheme-engravers.scm (right): https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode151 scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop) On 2017/05/20 12:18:33, thomasmorley651 wrote: > ...
2 days, 4 hours ago (2017-05-21 04:27:33 UTC) #17
thomasmorley651
On 2017/05/21 04:27:33, horndude77 wrote: > https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm > File scm/scheme-engravers.scm (right): > > https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode151 > ...
1 day, 22 hours ago (2017-05-21 10:38:27 UTC) #18
thomasmorley651
I'd like to mention another point: What to do with pitched rests and rests with ...
1 day, 15 hours ago (2017-05-21 17:12:26 UTC) #19
Trevor Daniels
1 day, 13 hours ago (2017-05-21 19:04:46 UTC) #20
On 2017/05/21 17:12:26, thomasmorley651 wrote:
> I'd like to mention another point:
> What to do with pitched rests and rests with user-set staff-position, merge
them
> automatically to the zero-position?

If a user has explicitly set the position of a rest this should be honoured
by default, I think ...
 
> I'd say using suspendRestMerging-property is sufficient to cover this case,
but
> this is only me. Other opinions?

... unless some property (mergePitchedRests?) is set true.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted