Move note names to scm

Move note names to scm

This patch moves the pitchnames alist declarations to
define-note-names.scm, where it would seem more consistent
with other variables (the only reason why note names were
defined in ly/*.ly files was the old \include "foobar.ly"
syntax for switching note names, but the new syntax doesn't
require this anymore).

(This will also allow to optionally take the current input
language into account when displaying or printing note names,
as will be demonstrated in an upcoming patch.)

[Valentin Villenave, 2010-11-21 10:12:41]

Greetings everybody,

Here's a new episode of my Quest For The Ideal Note-Names Implementation™: since
all pitchnames alists are now defined in a single file, there's little reason to
keep this file a .ly file: most other definitions (e.g. paper variables, grob
properties etc.) are to be found in scm/, not in ly/.

So, here's what I'd suggest. (Additionally, I need this for a future patch I'm
working on which I'll be discussing on the frogs list, so there actually /is/ a
point to this.)

Thanks for having a look!

Cheers,
Valentin.

[valentin_villenave.net, 2010-11-21 10:19:27]

On Sun, Nov 21, 2010 at 11:12 AM,  <v.villenave@gmail.com> wrote:
> Thanks for having a look!

SH*T. There we go again with that bloody annoying "upload in progress"
message. If you can't see some of these files, please use the unified
diffs instead (it should work).

What Rietveld doesn't mention is that this patch also removes
ly/language-init.ly (actually, define-note-names.scm is the exact same
file, so git just regards it as a renaming).

Cheers,
Valentin.

[Trevor Daniels, 2010-11-21 11:25:04]

The several legacy language files should also be changed:

ly/english.ly:%%%% Legacy file. (see language-init.ly)

[Valentin Villenave, 2010-11-21 11:34:44]

On 2010/11/21 11:25:04, Trevor Daniels wrote:
> The several legacy language files should also be changed:
> 
> ly/english.ly:%%%% Legacy file. (see language-init.ly)

Indeed. Nice catch!
Done.

Valentin

[Valentin Villenave, 2010-11-23 12:45:33]

Greetings everybody,

new patch set with a minor documentation addition. It should be okay to push
(unless someone objects to moving the pitchnames alist definitions to scm/ at
all).

Cheers!
Valentin.

[Carl, 2010-11-23 15:09:14]

I couldn't comment on the last two files in the patch set.

Here are some comments on the other files.

Thanks,

Carl

http://codereview.appspot.com/3247041/diff/9001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/3247041/diff/9001/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:365: (_i "Music-function wrapper for selecting note
names language.")
I prefer the old doc string to the new doc string.  Actually, I would prefer it
to say "Set note names for language @var{str}."  And I would prefer the name of
the argument to be "lang" or "note-language" instead of "str", because "str"
indicates the type of the variable, not the purpose of the variable.

http://codereview.appspot.com/3247041/diff/9001/ly/predefined-guitar-fretboar...
File ly/predefined-guitar-fretboards.ly (right):

http://codereview.appspot.com/3247041/diff/9001/ly/predefined-guitar-fretboar...
ly/predefined-guitar-fretboards.ly:456: #(ly:parser-set-note-names parser
pitchnames)
Why is this call different from the call in line 20?  It seems to me that it
should be the same.

[Valentin Villenave, 2010-11-23 16:25:13]

On 2010/11/23 15:09:14, Carl wrote:
> I couldn't comment on the last two files in the patch set.

This is getting bloody annoying. Am I the only one who can't make Rietveld work
in a sane way?

> I prefer the old doc string to the new doc string.  Actually, I would prefer
it
> to say "Set note names for language @var{str}."  And I would prefer the name
of
> the argument to be "lang" or "note-language" instead of "str", because "str"
> indicates the type of the variable, not the purpose of the variable.

You're absolutely right. Done.

> ly/predefined-guitar-fretboards.ly:456: #(ly:parser-set-note-names parser
> pitchnames)
> Why is this call different from the call in line 20?  It seems to me that it
> should be the same.

Ideally it should, yes; it isn't anymore because my new functions
note-names-language takes a string as argument, and the pitchnames variable
contains an alist instead.
Previously, I did store an alist in the default-language variable, but I think
it's better to only define as a string (namely, "nederlands").
This is more elegant and consistent (see ly/declarations-init.ly), and offers
additional flexibility for my upcoming note-names->markup patches (not sure this
qualifies as a good reason though :)

Thanks for your input!
Valentin.

[Trevor Daniels, 2010-11-23 17:04:23]

LGTM

Trevor

[Carl, 2010-11-23 17:54:36]

On 2010/11/23 16:25:13, Valentin Villenave wrote:
> On 2010/11/23 15:09:14, Carl wrote:

> > ly/predefined-guitar-fretboards.ly:456: #(ly:parser-set-note-names parser
> > pitchnames)
> > Why is this call different from the call in line 20?  It seems to me that it
> > should be the same.
> 
> Ideally it should, yes; it isn't anymore because my new functions
> note-names-language takes a string as argument, and the pitchnames variable
> contains an alist instead.
> Previously, I did store an alist in the default-language variable, but I think
> it's better to only define as a string (namely, "nederlands").
> This is more elegant and consistent (see ly/declarations-init.ly), and offers
> additional flexibility for my upcoming note-names->markup patches (not sure
this
> qualifies as a good reason though :)

So what if you defined a couple of music functions in
ly/music-functions-init.ly.

/changeLanguageAndSave #default-language

/restoreSavedLanguage

Then anybody else who wants to specify a language in an included file can do so
quite easily.

Thanks,

Carl

[Neil Puttock, 2010-11-24 00:10:01]

Hi Valentin,

Here are a few nitpicks (some of which I couldn't comment on directly):

All the language files have a rogue newline at the end.

The indentation inside language-pitch-names is artificially compressed, e.g.,

(bad)

    (nederlands . (
      (ceses . ,(ly:make-pitch -1 0 DOUBLE-FLAT))

(good)

    (nederlands . (
                   (ceses . ,(ly:make-pitch -1 0 DOUBLE-FLAT))

note-names-language needs some minor reindenting.

Cheers,
Neil

http://codereview.appspot.com/3247041/diff/19001/ly/predefined-guitar-fretboa...
File ly/predefined-guitar-fretboards.ly (right):

http://codereview.appspot.com/3247041/diff/19001/ly/predefined-guitar-fretboa...
ly/predefined-guitar-fretboards.ly:456: #(ly:parser-set-note-names parser
pitchnames)
need newline

[Valentin Villenave, 2010-11-25 12:24:44]

On 2010/11/24 00:10:01, Neil Puttock wrote:
> The indentation inside language-pitch-names is artificially compressed, e.g.,

That allowed me to make git regard the file as renamed instead of rewritten. But
you're right, I've fixed the indentation.

BTW: Neil, I did follow your advice or loading all the pitchnames alists and
then selecting the one that's needed (if at all), but am still concerned by the
overhead. The previous implementation \include "foobar.ly", as well as my
initial ly:gulp-file hack, had both the same advantage, in that it didn't load
alists that weren't actually required by the user (many users won't ever use
\language, after all). Don't you think things are a bit heavier now?

Carl: instead of implementing two new functions, I've created a new
other-language variable. I think it's slightly more elegant, can you have a look
and let me know what you think?

(Yes, there is one caveat compared to the prev-pitchnames approach you
previously used: in the (unlikely) case where a user does
  \language "italiano"
  { some music }
  \language "nederlands"
  { some music }
  \include "predefined-guitar-fretboards.ly"

Then the language will be set back to "italiano". If you think that's
unacceptable, I can cook something up to avoid it. But then it won't be as
elegant as it is now :-)

Cheers,
Valentin.

[Carl, 2010-11-25 15:19:17]

On 2010/11/25 12:24:44, Valentin Villenave wrote:
> On 2010/11/24 00:10:01, Neil Puttock wrote:
> > The indentation inside language-pitch-names is artificially compressed,
e.g.,
> 
> That allowed me to make git regard the file as renamed instead of rewritten.
But
> you're right, I've fixed the indentation.
> 
> BTW: Neil, I did follow your advice or loading all the pitchnames alists and
> then selecting the one that's needed (if at all), but am still concerned by
the
> overhead. The previous implementation \include "foobar.ly", as well as my
> initial ly:gulp-file hack, had both the same advantage, in that it didn't load
> alists that weren't actually required by the user (many users won't ever use
> \language, after all). Don't you think things are a bit heavier now?

They may be a bit heavier now, but the amount of storage that the language
alists take is insignificant compared to LilyPond's overall memory usage.  I
don't see a problem at all.

> 
> Carl: instead of implementing two new functions, I've created a new
> other-language variable. I think it's slightly more elegant, can you have a
look
> and let me know what you think?
> 

I think the previous approach was better.  Saving a setting of the current
language and then restoring it is exactly the desired functionality.  You wrote
exactly the right code previously; I think it should just be made more
accessible than it was.

Now that I think about it, I assume that the problem you were having is that
music functions aren't evaluated during parsing, and that the changes you need
have to be made during parsing.  So we need to have Scheme functions, not music
functions.

So I think 

#(save-and-change-language default-language)

and

#(restore-saved-language)

would handle the situation very nicely.

Thanks,

Carl

[Valentin Villenave, 2010-11-25 22:50:53]

On 2010/11/25 15:19:17, Carl wrote:
> They may be a bit heavier now, but the amount of storage that the language
> alists take is insignificant compared to LilyPond's overall memory usage.  I
> don't see a problem at all.

It's not just about memory usage: isn't it possible that the lookup time for
every pitches might be increased, even just by a tiny amount? (Not that I could
measure it in any way, but I'm wondering.)

> Now that I think about it, I assume that the problem you were having is that
> music functions aren't evaluated during parsing, and that the changes you need
> have to be made during parsing.  So we need to have Scheme functions, not
music
> functions.

Not necessarily: after all, predefined-guitar-fretboards.ly is loaded long after
music-functions-init or declarations-init.ly. What did worry me was the
prev-pitchnames variable I previously used: should I declare this variable at a
global level in define-note-names.scm, or locally in predefined-fretboards.ly?

(The *actual* reason is that I'm a total psycho, and every time I introduce a
new global variable I'm thinking that this will take extra space in memory, and
make the program more bloated -- Gee, I need to remind myself that I'm not in
high school anymore, and that I'm not writing programs for a 32Ko programmable
calculator! :-)

> So I think 
> #(save-and-change-language default-language)
> and
> #(restore-saved-language)
> would handle the situation very nicely.

Indeed, that's quite elegant. I'll get right on it, thanks!

Cheers,
Valentin.

[Neil Puttock, 2010-11-25 23:05:48]

On 2010/11/25 22:50:53, Valentin Villenave wrote:

> It's not just about memory usage: isn't it possible that the lookup time for
> every pitches might be increased, even just by a tiny amount? (Not that I
could
> measure it in any way, but I'm wondering.)

No, since the underlying method for setting the language hasn't changed: the
language sublist retrieved from the master list is copied into a fresh hash
table, so the lexer never accesses the list directly.

Cheers,
Neil

[Valentin Villenave, 2010-11-25 23:13:52]

On 2010/11/25 23:05:48, Neil Puttock wrote:
> No, since the underlying method for setting the language hasn't changed: the
> language sublist retrieved from the master list is copied into a fresh hash
> table, so the lexer never accesses the list directly.

Oh, I didn't know Guile had any notion of "sublists", which is why I was afraid
that the whole "master" alist would be processed everytime. Thanks for the
explanation!

Valentin.

[Valentin Villenave, 2010-12-06 15:01:50]

On 2010/11/25 15:19:17, Carl wrote:
> I think the previous approach was better.  Saving a setting of the current
> language and then restoring it is exactly the desired functionality.  You
wrote
> exactly the right code previously; I think it should just be made more
> accessible than it was.
> 
> Now that I think about it, I assume that the problem you were having is that
> music functions aren't evaluated during parsing, and that the changes you need
> have to be made during parsing.  So we need to have Scheme functions, not
music
> functions.
> 
> So I think 
> #(save-and-change-language default-language)
> and
> #(restore-saved-language)
> would handle the situation very nicely.

Hi Carl,

here's a new patch set that should do the trick. The only inconvenience is that
(restore-saved-language) needs a "parser" argument, but I don't think it's that
bad.

Oh look, the side-by-side diff view's working! (er, sort of.)

Cheers,
Valentin.

[Valentin Villenave, 2010-12-07 22:09:30]

On 2010/11/21 10:12:41, Valentin Villenave wrote:
> So, here's what I'd suggest. (Additionally, I need this for a future patch I'm
> working on which I'll be discussing on the frogs list, so there actually /is/
a
> point to this.)

FTR, the frogs discussion I was referring to in my initial mail above, may now
be found here:
http://lilypond-frogs.2124236.n2.nabble.com/PATCH-chord-names-language-td5812...
(my initial mail from two weeks ago didn't go through, so I've just reposted
it).
Please let me know if I'm on the right track!

Cheers,
Valentin.

[Neil Puttock, 2010-12-08 00:11:06]

On 2010/12/06 15:01:50, Valentin Villenave wrote:

> here's a new patch set that should do the trick. The only inconvenience is
that
> (restore-saved-language) needs a "parser" argument, but I don't think it's
that
> bad.

You can use music functions for this.  The parser evaluates all music functions
immediately they're invoked via run_music_function ().

Cheers,
Neil

[Valentin Villenave, 2010-12-08 07:41:27]

On 2010/12/08 00:11:06, Neil Puttock wrote:
> You can use music functions for this.  The parser evaluates all music
functions
> immediately they're invoked via run_music_function ().

Indeed, it was also Carl's idea in the first place.
Here's a version with music functions instead of Scheme functions, hopefully
this should be good to go :-)

I'm sorry that this (otherwise trivial) patch has been taking such a long time!

Cheers,
Valentin.

[Carl, 2010-12-09 13:17:17]

LGTM.

Carl

P.S.  Right now I can't see anything for scm/note-names.scm without downloading
the patch.  I don't know how this is so messed up on rietveld, but it certainly
is....