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

Issue 6422061: Second set of volta changes (Issue 2673) (Closed)

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

Description

The second set of proposed changes to volta syntax, posted on behalf of Arnold Theresius. This version is updated to eliminate the regtest error found in the previous version. Checked against regtests and clean.

Patch Set 1 #

Total comments: 2

Patch Set 2 : Updated version to eliminate regtest error #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -20 lines) Patch
M lily/volta-bracket.cc View 1 chunk +0 lines, -19 lines 0 comments Download
M lily/volta-engraver.cc View 1 2 chunks +38 lines, -1 line 0 comments Download
M ly/engraver-init.ly View 1 1 chunk +4 lines, -0 lines 1 comment Download
M scm/define-context-properties.scm View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9
PhilEHolmes
Please review
6 years, 11 months ago (2012-07-24 14:05:20 UTC) #1
dak
http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly File ly/engraver-init.ly (right): http://codereview.appspot.com/6422061/diff/1/ly/engraver-init.ly#newcode621 ly/engraver-init.ly:621: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|" Trailing ...
6 years, 11 months ago (2012-07-24 19:15:15 UTC) #2
email_philholmes.net
----- Original Message ----- From: <dak@gnu.org> To: <PhilEHolmes@googlemail.com>; <ianhulin44@gmail.com>; <graham@percival-music.ca> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> Sent: Tuesday, ...
6 years, 11 months ago (2012-07-24 19:40:35 UTC) #3
dak
On 2012/07/24 19:40:35, email_philholmes.net wrote: > ----- Original Message ----- > From: <mailto:dak@gnu.org> > To: ...
6 years, 11 months ago (2012-07-24 20:31:29 UTC) #4
Keith
http://codereview.appspot.com/6422061/diff/1/lily/volta-engraver.cc File lily/volta-engraver.cc (right): http://codereview.appspot.com/6422061/diff/1/lily/volta-engraver.cc#newcode159 lily/volta-engraver.cc:159: SCM glyph = endbar ? endbar->get_property ("glyph-name") : SCM_EOL; ...
6 years, 10 months ago (2012-08-04 07:04:33 UTC) #5
PhilEHolmes
Please review
6 years, 10 months ago (2012-08-07 13:03:42 UTC) #6
Keith
I don't see any more problems. I understand the reason to create a configurable list ...
6 years, 10 months ago (2012-08-08 06:09:09 UTC) #7
email_philholmes.net
----- Original Message ----- From: <k-ohara5a5a@oco.net> To: <PhilEHolmes@googlemail.com>; <ianhulin44@gmail.com>; <dak@gnu.org>; <graham@percival-music.ca>; <email@philholmes.net> Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com> ...
6 years, 10 months ago (2012-08-08 08:28:32 UTC) #8
dak
6 years, 10 months ago (2012-08-08 08:35:30 UTC) #9
On 2012/08/08 08:28:32, email_philholmes.net wrote:
> ----- Original Message ----- 
>
> > ly/engraver-init.ly:622: ":|" "|:" "||:" "|." ":|:" ":|.|:" ":|.:" ".|"
> > trim trailing whitespace before pushing if you remember
> >
> > http://codereview.appspot.com/6422061/
> 
> I didn't trim the whitespace because I wasn't sure of the effect in a list 
> like this.  Will it definitely have no effect?

Inside of Scheme but outside of a string or character constant, one
whitespace (including newline) is as good as many.  So there is no
reason for trailing whitespace on a line.
Sign in to reply to this message.

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