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

Issue 207105: Page breaking for multiline embedded score

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 2 months ago by Boris Shingarov
Modified:
14 years ago
Reviewers:
carl.d.sorensen
Visibility:
Public.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -6 lines) Patch
M scm/define-markup-commands.scm View 3 chunks +16 lines, -5 lines 2 comments Download
M scm/markup.scm View 2 chunks +15 lines, -1 line 2 comments Download

Messages

Total messages: 1
Carl
14 years ago (2010-05-03 14:18:10 UTC) #1
As far as I can see, this patch can be applied when it has been updated to code
standards.

However, I don't yet see the application or the importance of the patch, because
I haven't seen the work that depends on it.

Thanks,

Carl

http://codereview.appspot.com/207105/diff/1/2
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/207105/diff/1/2#newcode623
scm/define-markup-commands.scm:623: (vector->list (ly:paper-score-paper-systems
ps))))
Indentation.  (vector->list should line up with paper-system.

http://codereview.appspot.com/207105/diff/1/2#newcode1364
scm/define-markup-commands.scm:1364: )))
these parentheses should be at the end of the previous line.

http://codereview.appspot.com/207105/diff/1/3
File scm/markup.scm (right):

http://codereview.appspot.com/207105/diff/1/3#newcode559
scm/markup.scm:559: (define (prepend-stencil aStencil stencilList)
Why call this prepend-stencil instead of prepend-stencils?

prepend-stencils would imply that it can be either a stencil or a list of
stencils.

And using prepended-stencils instead of aStencil would carry the meaning better.

We don't use stencilList in Scheme, we use stencil-list.  See the Contributor's
Guide, section 9.5.4.

http://codereview.appspot.com/207105/diff/1/3#newcode562
scm/markup.scm:562: but it is also allowed to be a list of stencils."
Docstring indentation
Sign in to reply to this message.

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