Issue 1646041: Implements woodwind diagrams in lilypond. Responds to previous

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 1646041: Implements woodwind diagrams in lilypond. Responds to previous (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
13 years, 11 months ago by Carl

Modified:
13 years, 10 months ago

Reviewers:

CC:
lilypond-devel_gnu.org

Visibility:
Public.

Description

Implements woodwind diagrams in lilypond. Responds to previous comments.

Patch Set 1 #

Patch Set 2 : Revised for Patrick's comments #

Total comments: 8

Patch Set 3 : Respond to Carl's comments #

Created: 13 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+3958 lines, -61 lines)			Patch
A	input/regression/woodwind-diagrams-empty.ly	View		1 chunk	+70 lines, -0 lines	0 comments	Download
A	input/regression/woodwind-diagrams-key-lists.ly	View		1 chunk	+18 lines, -0 lines	0 comments	Download
M	ps/music-drawing-routines.ps	View		6 chunks	+63 lines, -5 lines	0 comments	Download
A	scm/bezier-tools.scm	View		1 chunk	+105 lines, -0 lines	0 comments	Download
M	scm/define-stencil-commands.scm	View		2 chunks	+2 lines, -0 lines	0 comments	Download
A	scm/define-woodwind-diagrams.scm	View		1 chunk	+1230 lines, -0 lines	0 comments	Download
A	scm/display-woodwind-diagrams.scm	View		1 chunk	+1987 lines, -0 lines	0 comments	Download
M	scm/flag-styles.scm	View		4 chunks	+12 lines, -27 lines	0 comments	Download
M	scm/lily.scm	View	1 2	2 chunks	+3 lines, -1 line	0 comments	Download
M	scm/lily-library.scm	View	1	5 chunks	+140 lines, -11 lines	0 comments	Download
M	scm/output-ps.scm	View	1 2	9 chunks	+45 lines, -12 lines	0 comments	Download
M	scm/output-svg.scm	View	1	1 chunk	+88 lines, -0 lines	0 comments	Download
M	scm/stencil.scm	View	1 2	3 chunks	+195 lines, -5 lines	0 comments	Download

Messages

Total messages: 1

Expand All Messages | Collapse All Messages

Carl

13 years, 10 months ago (2010-06-20 13:48:50 UTC) #1

Mike,

There are still some issues in code formatting in stencil.scm.

The general principle in Scheme indentation is that we like to get as much
as possible on a line.  In particular,

* We never leave an opening parenthesis alone on a line
* We try to never leave a procedure name alone on a line
* We try to never separate the parameter list from the lambda keyword

So we don't end up with lots of two-space-indented blocks.  Rather, we
tend to end up with blocks indented by the length of a procedure name plus a
space.  It helps us to accurately grok the structure.

Thanks,

Carl

http://codereview.appspot.com/1646041/diff/2001/3011
File scm/stencil.scm (right):

http://codereview.appspot.com/1646041/diff/2001/3011#newcode212
scm/stencil.scm:212: (lambda
Spacing -- (lambda (adder) ....

http://codereview.appspot.com/1646041/diff/2001/3011#newcode215
scm/stencil.scm:215: (lambda
(lambda (quadrant) ...

http://codereview.appspot.com/1646041/diff/2001/3011#newcode333
scm/stencil.scm:333: (lambda
(lambda (op) ...

http://codereview.appspot.com/1646041/diff/2001/3011#newcode350
scm/stencil.scm:350: (map
Actually, better spacing would be 
(map (lambda (x) ...
            (body of lambda)
          (list to be mapped)

In general, keep lines as full as you reasonably can.  Look for ways to continue
them, rather than to split them.

http://codereview.appspot.com/1646041/diff/2001/3011#newcode357
scm/stencil.scm:357: (map
(map (lambda (x) ...

http://codereview.appspot.com/1646041/diff/2001/3011#newcode366
scm/stencil.scm:366: (reduce min 10000 (map caar x))
why 10000 and -10000?  These numbers seem arbitrary.

http://codereview.appspot.com/1646041/diff/2001/3011#newcode382
scm/stencil.scm:382: `(
Never put an opening parenthesis by itself on a line in scheme.

http://codereview.appspot.com/1646041/diff/2001/3012
File scm/woodwind-diagrams.scm (right):

http://codereview.appspot.com/1646041/diff/2001/3012#newcode2986
scm/woodwind-diagrams.scm:2986: 
I thought you were going to split assembly from display?

Expand All Messages | Collapse All Messages