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

Issue 5320074: Fixes to jazz chord displays (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 1 month ago by Carl
Modified:
8 years, 1 month ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fixes to jazz chord displays This issue consists of a well-formed series of commits created by Adam Spiers. The commits are independent; each commit modifies a different set of files. If the complete patch set builds, each of the individual commits should build. I have not tested them, but I've read them carefully and judge that the risk is low enough to review the complete set. I have removed the .scm file reformatting from this patch set, as the final definition of our .scm format is not yet agreed upon. Therefore, our policy is to leave spacing as is until we have agreed. Issue 1503 - add support for "altered" jazz chord (super-Locrian) fix indentation in chord-ignatzek-names.scm Doc: fix spurious pluralisation typo in Chord name chart appendix Issue 1503 - add additionalPitchPrefix to allow choice of prefix for additional pitches in chords. This was previously "add", e.g. "Cmaj7 add6add9", but this results in too much clutter and is rarely used. Issue 1503 - Allow choice of minor chord modifier. For example, often it is preferred to use a hyphen instead of "m". This can now be achieved via: \set minorChordModifier = \markup { "-" } add slashChordSeparator Issue 1572 and issue 1503 - Allow choice of chord modifier separator independently of chord inversion separator, since conventionally the latter is always a slash (hence the term "slash chords"), whereas the former seldom involves slashes. add some missing chords to the ignatzek regression test Issue 1503 - Recognise Lydian chords enlarge half-diminished slashed circle symbol

Patch Set 1 : Fix scm file upload #

Patch Set 2 : Rebased to current master #

Patch Set 3 : Fix regression involving slashSeparator #

Patch Set 4 : Rebased to current master #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -125 lines) Patch
M Documentation/included/chord-names-jazz.ly View 3 chunks +82 lines, -78 lines 0 comments Download
M Documentation/notation/chords.itely View 3 chunks +74 lines, -7 lines 0 comments Download
M Documentation/notation/notation-appendices.itely View 1 chunk +1 line, -1 line 0 comments Download
A input/regression/chord-additional-pitch-prefix.ly View 1 chunk +11 lines, -0 lines 2 comments Download
A input/regression/chord-name-minor.ly View 1 chunk +13 lines, -0 lines 0 comments Download
A input/regression/chord-slash-separator.ly View 1 chunk +11 lines, -0 lines 0 comments Download
M input/regression/chords-funky-ignatzek.ly View 1 chunk +6 lines, -0 lines 0 comments Download
M ly/chord-modifiers-init.ly View 2 3 1 chunk +3 lines, -1 line 0 comments Download
M ly/engraver-init.ly View 1 chunk +4 lines, -1 line 0 comments Download
M scm/chord-ignatzek-names.scm View 1 2 3 chunks +41 lines, -37 lines 0 comments Download
M scm/define-context-properties.scm View 3 chunks +7 lines, -0 lines 2 comments Download

Messages

Total messages: 16
Carl
I've uploaded Adam's changes for review. I removed the spacing changes in scm/chord-ignatzek-names.scm awaiting resolution ...
8 years, 1 month ago (2011-11-03 23:33:27 UTC) #1
pkx166h
Patch doesn't apply to current master.. --snip-- jlowe@jlowe-lilybuntu2:~/lilypond-git$ patch -p1 < ../Desktop/issue5320074_2001.diff patching file Documentation/included/chord-names-jazz.ly ...
8 years, 1 month ago (2011-11-03 23:47:51 UTC) #2
Carl
New patch set uploaded. Rebased to current master.
8 years, 1 month ago (2011-11-04 03:12:16 UTC) #3
pkx166h
Passes Make. Reg test diffs are shown here http://code.google.com/p/lilypond/issues/detail?id=1503#c20 James
8 years, 1 month ago (2011-11-04 07:07:57 UTC) #4
Carl
New patch set uploaded
8 years, 1 month ago (2011-11-04 12:42:09 UTC) #5
pkx166h
Carl, sorry patch needs rebasing again -snip-- jlowe@jlowe-lilybuntu2:~/lilypond-git$ patch -p1 < ../Desktop/issue5320074_11001.diff patching file Documentation/included/chord-names-jazz.ly ...
8 years, 1 month ago (2011-11-04 22:30:40 UTC) #6
pkx166h
Passes Make and reg test diffs here http://code.google.com/p/lilypond/issues/detail?id=1503#c27 James
8 years, 1 month ago (2011-11-05 11:38:42 UTC) #7
Carl
Looks very good to me; a couple of minor changes suggested. http://codereview.appspot.com/5320074/diff/14001/input/regression/chord-additional-pitch-prefix.ly File input/regression/chord-additional-pitch-prefix.ly (right): ...
8 years, 1 month ago (2011-11-05 14:33:32 UTC) #8
adam.spiers
Thanks Carl, I've made these changes, and I've also made a corresponding patch for changes.tely, ...
8 years, 1 month ago (2011-11-07 11:56:42 UTC) #9
Graham Percival
On Mon, Nov 07, 2011 at 11:56:42AM +0000, adam.spiers@gmail.com wrote: > Thanks Carl, I've made ...
8 years, 1 month ago (2011-11-07 12:12:25 UTC) #10
adam.spiers
On Mon, Nov 7, 2011 at 12:12 PM, Graham Percival <graham@percival-music.ca> wrote: > On Mon, ...
8 years, 1 month ago (2011-11-07 12:34:16 UTC) #11
c_sorensen
On Nov 7, 2011, at 5:34 AM, "Adam Spiers" <adam.spiers@gmail.com> wrote: > On Mon, Nov ...
8 years, 1 month ago (2011-11-07 12:44:49 UTC) #12
Graham Percival
On Mon, Nov 07, 2011 at 12:44:42PM +0000, Carl Sorensen wrote: > > I'll get ...
8 years, 1 month ago (2011-11-07 14:13:51 UTC) #13
adam.spiers
On Mon, Nov 7, 2011 at 2:11 PM, Graham Percival <graham@percival-music.ca> wrote: > On Mon, ...
8 years, 1 month ago (2011-11-07 14:44:01 UTC) #14
adam.spiers
On Mon, Nov 7, 2011 at 2:13 PM, Graham Percival <graham@percival-music.ca> wrote: > On Mon, ...
8 years, 1 month ago (2011-11-07 14:46:36 UTC) #15
adam.spiers
8 years, 1 month ago (2011-11-07 19:08:29 UTC) #16
Please close this issue as it's now superceded by
http://codereview.appspot.com/5343050/ - thanks!
Sign in to reply to this message.

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