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

Issue 321930043: Create engravers for merging rests

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by horndude77
Modified:
2 months 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: 7

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

Patch Set 8 : Don't merge pitched rests #

Total comments: 1

Patch Set 9 : Rebase, simplify scheme with every as suggested #

Total comments: 2

Patch Set 10 : Minor edit to docs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -0 lines) Patch
M Documentation/notation/simultaneous.itely View 1 2 3 4 5 6 7 8 9 2 chunks +34 lines, -0 lines 0 comments Download
A input/regression/merge-rests-engraver.ly View 1 2 3 4 5 6 7 1 chunk +79 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 7 8 1 chunk +84 lines, -0 lines 0 comments Download

Messages

Total messages: 29
Dan Eble
I'm currently trying to give other things priority over Lilypond, so I'm not going to ...
3 months ago (2017-05-14 19:26:57 UTC) #1
horndude77
Create engravers for merging rests
3 months 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 ...
3 months ago (2017-05-16 06:04:34 UTC) #3
horndude77
Rebased on master
3 months ago (2017-05-17 06:24:16 UTC) #4
m17spear80brymer
3 months 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 ...
3 months ago (2017-05-17 22:01:42 UTC) #6
horndude77
Rebase, set tracker issue
3 months ago (2017-05-18 05:14:49 UTC) #7
pkx166h
Passes make, make check and a full make doc.
3 months 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 ...
3 months 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 ...
3 months ago (2017-05-18 20:54:41 UTC) #10
horndude77
Address review comments
3 months 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 months ago (2017-05-20 05:18:38 UTC) #12
horndude77
Add suspendRestMerging context property
3 months 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 months, 4 weeks 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 months, 4 weeks ago (2017-05-20 14:33:15 UTC) #15
horndude77
Make overlapped grobs invisible, update regression test for attached text, fix error
2 months, 4 weeks 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 months, 4 weeks 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 > ...
2 months, 4 weeks 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 ...
2 months, 4 weeks ago (2017-05-21 17:12:26 UTC) #19
Trevor Daniels
On 2017/05/21 17:12:26, thomasmorley651 wrote: > I'd like to mention another point: > What to ...
2 months, 4 weeks ago (2017-05-21 19:04:46 UTC) #20
david.nalesnik
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) (ly:grob-set-property! rest 'Y-offset (rest-offset rest))) On 2017/05/21 ...
2 months, 3 weeks ago (2017-05-23 18:51:51 UTC) #21
horndude77
Don't merge pitched rests
2 months, 3 weeks ago (2017-05-27 19:50:59 UTC) #22
horndude77
> On 2017/05/21 17:12:26, thomasmorley651 wrote: > > I'd like to mention another point: > ...
2 months, 3 weeks ago (2017-05-27 20:03:43 UTC) #23
dak
On 2017/05/27 20:03:43, horndude77 wrote: > > On 2017/05/21 17:12:26, thomasmorley651 wrote: > > > ...
2 months, 3 weeks ago (2017-05-27 20:39:34 UTC) #24
thomasmorley651
I think this one warrants an entry in changes. https://codereview.appspot.com/321930043/diff/140001/scm/scheme-engravers.scm File scm/scheme-engravers.scm (right): https://codereview.appspot.com/321930043/diff/140001/scm/scheme-engravers.scm#newcode152 scm/scheme-engravers.scm:152: ...
2 months, 2 weeks ago (2017-05-29 23:13:30 UTC) #25
horndude77
Rebase, simplify scheme with every as suggested
2 months, 1 week ago (2017-06-12 05:18:30 UTC) #26
thomasmorley651
One nit. See below. No need for a new patch-set, imho. You could change it ...
2 months, 1 week ago (2017-06-12 07:40:42 UTC) #27
dak
https://codereview.appspot.com/321930043/diff/160001/Documentation/notation/simultaneous.itely File Documentation/notation/simultaneous.itely (right): https://codereview.appspot.com/321930043/diff/160001/Documentation/notation/simultaneous.itely#newcode917 Documentation/notation/simultaneous.itely:917: parts. This can be accomplished using the merge rests ...
2 months, 1 week ago (2017-06-12 08:19:34 UTC) #28
horndude77
2 months ago (2017-06-14 05:03:38 UTC) #29
Minor edit to docs
Sign in to reply to this message.

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