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

Issue 8874: New markup commands: \left-brace & \right-brace. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by Neil Puttock
Modified:
9 years, 10 months ago
Reviewers:
Patrick McCarty, carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

New markup commands: \left-brace & \right-brace.

Patch Set 1 #

Patch Set 2 : Use binary search to select brace glyph. #

Patch Set 3 : Move binary search to lily-library.scm. #

Total comments: 1

Patch Set 4 : Add exported function ly:otf-glyph-count. #

Total comments: 2

Patch Set 5 : Release candidate #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -14 lines) Patch
A input/regression/markup-brace-warning.ly View 1 chunk +17 lines, -0 lines 0 comments Download
A input/regression/markup-braces.ly View 2 3 4 1 chunk +15 lines, -0 lines 0 comments Download
M lily/open-type-font-scheme.cc View 4 4 chunks +30 lines, -13 lines 0 comments Download
M scm/define-markup-commands.scm View 1 2 3 4 1 chunk +58 lines, -0 lines 0 comments Download
M scm/lily-library.scm View 3 4 1 chunk +16 lines, -1 line 0 comments Download

Messages

Total messages: 5
Patrick McCarty
http://codereview.appspot.com/8874/diff/2201/3202 File scm/define-markup-commands.scm (right): http://codereview.appspot.com/8874/diff/2201/3202#newcode2625 Line 2625: (find-brace (binary-search 0 575 get-y-from-brace scaled-size)) Would Open_type_font::count ...
9 years, 11 months ago (2009-07-16 03:40:58 UTC) #1
Neil Puttock
Thanks for the review, Patrick. On 2009/07/16 03:40:58, Patrick McCarty wrote: > http://codereview.appspot.com/8874/diff/2201/3202 > File ...
9 years, 11 months ago (2009-07-16 22:43:08 UTC) #2
Neil Puttock
Add exported function ly:otf-glyph-count. * use this function to remove hard-coded value in binary search ...
9 years, 11 months ago (2009-07-16 23:53:45 UTC) #3
Carl
Code looks good to me. I have a couple of optional minor nitpicks. Thanks, Carl ...
9 years, 11 months ago (2009-07-17 01:40:04 UTC) #4
Neil Puttock
9 years, 11 months ago (2009-07-28 21:31:27 UTC) #5
On 2009/07/17 01:40:04, Carl wrote:
> Code looks good to me.

Thanks for taking a look.

> http://codereview.appspot.com/8874/diff/5202/4204#newcode2623
> Line 2623: (ly:font-get-glyph font (string-append "brace" (number->string
n)))))
> Do we want to keep line length to <80 chars?

Definitely.

I've turned on column numbers in Emacs to remind me not to breach the 80th
column (keeping the window unmaximized helps too). :)

> http://codereview.appspot.com/8874/diff/5202/4205#newcode555
> Line 555: "Find the index between START and END (an integer) which
> Use @var{start} instead of START ?
> Use (_i "docstring") to allow internationalization?

OK, will do.  It's probably a good habit to get into if it saves hassle in the
future.

Do you have any idea how we're going to implement documentation of these
functions?  I'm imagining it'll require some kind of macro which would override
the default behaviour of `define-public' when documentation-generate.scm is run.

Regards,
Neil
Sign in to reply to this message.

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