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

Issue 13242056: Issue 1321: Enhancement: add partcombineUp and partcombineDown functions

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by dak
Modified:
9 years, 4 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Issue 1321: Enhancement: add partcombineUp and partcombineDown functions This is actually a reimplementation on the basis of context mods. It makes \partcombine itself quite more flexible, though I am less than enthused about the utility of \partcombineUp and \partcombineDown. They are more or less unnecessary now. This also renders issue 3534 moot.

Patch Set 1 #

Patch Set 2 : swap \voiceTwo and \voiceFour for \partcombineDown #

Patch Set 3 : Use voiceOne/Two for outer+joined voices, add \dynamicUp/\dynamicDown #

Patch Set 4 : Better match of partcombineUp/Down #

Patch Set 5 : It's conceptually cleaner to have a separate disjoint modshared #

Patch Set 6 : Rebase after issue 3534 #

Patch Set 7 : Rebase on current master #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -62 lines) Patch
M lily/part-combine-iterator.cc View 1 2 3 4 5 6 5 chunks +25 lines, -53 lines 3 comments Download
M ly/music-functions-init.ly View 1 2 3 4 5 6 1 chunk +38 lines, -7 lines 3 comments Download
M scm/define-music-properties.scm View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M scm/part-combiner.scm View 1 2 3 4 5 6 2 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 13
dak
swap \voiceTwo and \voiceFour for \partcombineDown
10 years, 6 months ago (2013-09-11 17:56:36 UTC) #1
dak
Use voiceOne/Two for outer+joined voices, add \dynamicUp/\dynamicDown
10 years, 6 months ago (2013-09-11 20:43:04 UTC) #2
Trevor Daniels
Patch set 3 LGTM.
10 years, 6 months ago (2013-09-11 20:52:00 UTC) #3
dak
Better match of partcombineUp/Down
10 years, 6 months ago (2013-09-11 20:53:11 UTC) #4
dak
It's conceptually cleaner to have a separate disjoint modshared
10 years, 6 months ago (2013-09-11 22:03:24 UTC) #5
dak
Rebase after issue 3534
10 years, 6 months ago (2013-09-12 14:42:52 UTC) #6
janek
just a quick look, but LGTM.
10 years, 6 months ago (2013-09-16 11:46:32 UTC) #7
dak
Rebase on current master
9 years, 5 months ago (2014-11-01 08:51:47 UTC) #8
Dan Eble
https://codereview.appspot.com/13242056/diff/26001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/13242056/diff/26001/ly/music-functions-init.ly#newcode1168 ly/music-functions-init.ly:1168: #{ \partcombine \with { \voiceOne } \with { \voiceOne ...
9 years, 5 months ago (2014-11-01 15:26:57 UTC) #9
Dan Eble
On 2014/11/01 15:26:57, Dan Eble wrote: > I *think* > it would improve the outcome ...
9 years, 5 months ago (2014-11-01 17:09:50 UTC) #10
dak
https://codereview.appspot.com/13242056/diff/26001/ly/music-functions-init.ly File ly/music-functions-init.ly (right): https://codereview.appspot.com/13242056/diff/26001/ly/music-functions-init.ly#newcode1168 ly/music-functions-init.ly:1168: #{ \partcombine \with { \voiceOne } \with { \voiceOne ...
9 years, 5 months ago (2014-11-01 17:27:39 UTC) #11
Keith
No need to pass the context modifications through the lower-level functions, when they can be ...
9 years, 5 months ago (2014-11-02 05:03:41 UTC) #12
Dan Eble
9 years, 4 months ago (2014-11-02 12:58:24 UTC) #13
https://codereview.appspot.com/13242056/diff/26001/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

https://codereview.appspot.com/13242056/diff/26001/lily/part-combine-iterator...
lily/part-combine-iterator.cc:37: = {"one", "two", "shared", "solo", "null"};
On 2014/11/02 05:03:41, Keith wrote:
> Setting these [hard-coded voice names] to  one, two, one, one, null
> seems to do what Dan wishes.

I'm laughing because this idea is the basis of what I tried years ago.  I can
confirm that it was useful.  I didn't have the stamina to see the review process
through.  As today, there was dissatisfaction that my approach wasn't more
general.
Sign in to reply to this message.

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