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

Issue 5318046: Get rid of most of the insane string-tunings API (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 5 months ago by dak
Modified:
12 years, 4 months ago
Reviewers:
carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Get rid of most of the insane string-tunings API convert-ly rules are there. Translations should compile again after applying convert-ly, but of course the changes to text and examples should be propagated eventually with the usual processes. This is intended for dev/staging as soon as staging contains a convert-ly-clean tree.

Patch Set 1 #

Patch Set 2 : Add convertrules (some artifacts due to current state of master expected). #

Patch Set 3 : State after update-with-convert-ly. Intended for pushing as a merge commit. #

Total comments: 2

Patch Set 4 : Adjusted for versioning. No idea why not patch-review. Will push to dev/staging after tests. #

Patch Set 5 : Get \contextStringTuning conversion fit for music lists. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+133 lines, -196 lines) Patch
M Documentation/de/notation/fretted-strings.itely View 1 2 3 4 3 chunks +4 lines, -3 lines 0 comments Download
M Documentation/es/notation/fretted-strings.itely View 1 2 3 4 3 chunks +4 lines, -3 lines 0 comments Download
M Documentation/fr/notation/fretted-strings.itely View 1 2 3 4 3 chunks +4 lines, -3 lines 0 comments Download
M Documentation/included/display-predefined-string-tunings.ly View 1 chunk +12 lines, -24 lines 0 comments Download
M Documentation/notation/fretted-strings.itely View 1 2 3 6 chunks +28 lines, -46 lines 0 comments Download
M input/regression/context-string-tuning.ly View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M input/regression/tablature-letter.ly View 1 2 3 2 chunks +2 lines, -7 lines 0 comments Download
M input/regression/tablature-string-tunings.ly View 1 2 3 2 chunks +2 lines, -5 lines 0 comments Download
M ly/string-tunings-init.ly View 1 2 3 1 chunk +62 lines, -103 lines 0 comments Download
M python/convertrules.py View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 2
Carl
LGTM. Thanks, David. Nicely done. Carl http://codereview.appspot.com/5318046/diff/12002/input/regression/tablature-letter.ly File input/regression/tablature-letter.ly (right): http://codereview.appspot.com/5318046/diff/12002/input/regression/tablature-letter.ly#newcode32 input/regression/tablature-letter.ly:32: stringTunings = \stringTuning ...
12 years, 4 months ago (2011-10-25 15:55:34 UTC) #1
dak
12 years, 4 months ago (2011-10-25 16:05:42 UTC) #2
http://codereview.appspot.com/5318046/diff/12002/input/regression/tablature-l...
File input/regression/tablature-letter.ly (right):

http://codereview.appspot.com/5318046/diff/12002/input/regression/tablature-l...
input/regression/tablature-letter.ly:32: stringTunings = \stringTuning \notemode
{ <a d' f' a' d'' f''> }
On 2011/10/25 15:55:34, Carl wrote:
> Why not just 
> 
> stringTunings = \stringTuning <a d' f' a' d'' f''>
> 
> ?

Music functions parse their arguments in the current mode.  Context
modifications are delivered in "initial mode" that knows no notenames but can
deal with identifiers containing hyphens, something which is not possible in
"note mode".

Why it would be a good idea to define identifiers for context variables that
can't be entered in notemode escapes me, but there you are.

Oh, and switching on notename conversions just in case is too expensive anyway:
whenever you switch on notemode, the current notename alist is translated into a
hashtable again.

At some point of time, I plan to permit mode-switching while scanning music
function argument lists.

At some point of time, I plan to decrease the necessity for all those stupid
different modes.

This point of time is not there.  Hence the example with \notemode to avoid
tripping users up all too bad.
Sign in to reply to this message.

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