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

Issue 6344092: Adds support for cross-staff-stems (Closed)

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

Description

Included the regtest and the snippet on this patch, and added texi-string for the regtest. Hope this is good to go.

Patch Set 1 #

Total comments: 5

Patch Set 2 : Completely reworked patch set - should answer all comments #

Total comments: 1

Patch Set 3 : Final version #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -169 lines) Patch
A Documentation/snippets/new/cross-staff-stems.ly View 1 2 1 chunk +31 lines, -0 lines 1 comment Download
D Documentation/snippets/new/stem-cross-staff-engraver.ly View 1 chunk +0 lines, -168 lines 0 comments Download
A input/regression/cross-staff-stems.ly View 1 2 1 chunk +33 lines, -0 lines 0 comments Download
M ly/music-functions-init.ly View 1 2 chunks +12 lines, -1 line 1 comment Download
M scm/music-functions.scm View 1 1 chunk +89 lines, -0 lines 2 comments Download

Messages

Total messages: 13
PhilEHolmes
Please review.
7 years ago (2012-07-09 14:39:08 UTC) #1
Trevor Daniels
I don't think all these functions should be in ly/music-functions-init.ly, but I don't know the ...
7 years ago (2012-07-09 21:38:13 UTC) #2
email_philholmes.net
----- Original Message ----- From: <tdanielsmusic@googlemail.com> To: <PhilEHolmes@googlemail.com> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Monday, July 09, ...
7 years ago (2012-07-10 11:06:37 UTC) #3
Graham Percival
http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross-staff-stems.ly File Documentation/snippets/new/cross-staff-stems.ly (right): http://codereview.appspot.com/6344092/diff/1/Documentation/snippets/new/cross-staff-stems.ly#newcode5 Documentation/snippets/new/cross-staff-stems.ly:5: texidoc = "This file demonstrates a scheme engraver that ...
7 years ago (2012-07-13 07:20:54 UTC) #4
PhilEHolmes
Please review
7 years ago (2012-07-13 11:38:46 UTC) #5
Graham Percival
http://codereview.appspot.com/6344092/diff/7001/input/regression/cross-staff-stems.ly File input/regression/cross-staff-stems.ly (right): http://codereview.appspot.com/6344092/diff/7001/input/regression/cross-staff-stems.ly#newcode2 input/regression/cross-staff-stems.ly:2: Sorry, I was unclear. I meant to ask for ...
7 years ago (2012-07-14 01:39:02 UTC) #6
email_philholmes.net
----- Original Message ----- From: <graham@percival-music.ca> To: <PhilEHolmes@googlemail.com>; <tdanielsmusic@googlemail.com> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Saturday, July ...
7 years ago (2012-07-14 10:46:06 UTC) #7
PhilEHolmes
Please review final updates.
7 years ago (2012-07-14 15:23:49 UTC) #8
Keith
LGTM. Scheme-only patches are nice, because we don't have to book Linux to try them ...
7 years ago (2012-07-14 19:57:46 UTC) #9
Graham Percival
LGTM
7 years ago (2012-07-15 01:58:59 UTC) #10
Trevor Daniels
LGTM Trevor
6 years, 12 months ago (2012-07-19 13:21:07 UTC) #11
dak
http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm File scm/music-functions.scm (right): http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newcode1784 scm/music-functions.scm:1784: (if (<= 2 (length stems)) Why not use "and" ...
6 years, 10 months ago (2012-08-30 23:12:45 UTC) #12
mail_philholmes.net
6 years, 10 months ago (2012-08-31 08:46:12 UTC) #13
----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <tdanielsmusic@googlemail.com>; 
<graham@percival-music.ca>; <k-ohara5a5a@oco.net>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Friday, August 31, 2012 12:12 AM
Subject: Re: Adds support for cross-staff-stems (issue 6344092)


>
> http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm
> File scm/music-functions.scm (right):
>
>
http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newc...
> scm/music-functions.scm:1784: (if (<= 2 (length stems))
> Why not use "and" here and leave out the #f below?  Then one does not
> hunt for #f to figure out what happens when the condition is false.
>
>
http://codereview.appspot.com/6344092/diff/12002/scm/music-functions.scm#newc...
> scm/music-functions.scm:1801: (let ((span (ly:engraver-make-grob trans
> 'Stem '())))
> Why not have "root" as the cause, the last element of
> ly:engraver-make-grob?  That would make the stem span tweakable.
>
> http://codereview.appspot.com/6344092/


This patch has been approved, pushed and the Rietveld closed.  If you want 
to propose changes, it should be a new patch.

--
Phil Holmes 

Sign in to reply to this message.

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