[Patch] Add support for tempo ranges

Add support for tempo ranges

This patch set
 - extends the parser to accept the following syntax:
     \tempo numA ~ numB
 - creates a 'tempoUnitRange property (a pair of numbers)
 - sets 'tempoUnitCount accordingly by averaging the two
numbers whenever appropriate
 - modifies the metronome engraver to return the range pair
instead of the number when appropriate
 - adapts the format-metronome-markup translation function
to print the tempo range properly
 - adds a metronome-range.ly regtest.

[Valentin Villenave, 2010-11-22 16:54:03]

Greetings everybody,

here's a patch to add the ability to enter tempo indications as number ranges:
   \tempo 4 = 76~82

Please have a look and let me know!

[PS: the side-by-side diff doesn't work for me, but please have a look at the
unified diffs!]

[Carl, 2010-11-22 17:54:18]

Valentin,

When the side-by-side diffs don't work, we can't publish comments on the files.

There's an indentation error in scm/ly-syntax-constructors.scm, line 90.

I'd prefer that we not define a new context property TempoRange.  Instead, I'd
prefer that we define TempoCount to be either a number or a pair, and modify the
code that uses TempoCount so it can handle either one.

Thanks,

Carl

[Valentin Villenave, 2010-11-22 18:05:57]

On 2010/11/22 17:54:18, Carl wrote:
> Valentin,
>
> When the side-by-side diffs don't work, we can't publish comments on the
files.

Indeed.  That's unconvenient.

> There's an indentation error in scm/ly-syntax-constructors.scm, line 90.

Oh, indeed. I thought emacs would fix it, but it didn't.

> I'd prefer that we not define a new context property TempoRange.  Instead, I'd
> prefer that we define TempoCount to be either a number or a pair, and modify
the
> code that uses TempoCount so it can handle either one.

You're absolutely right. The reason why I did that was because I was
afraid some other code (e.g. performers) may rely on tempoUnitCount,
but I was wrong. I'll publish a new patch set shortly.

Thanks!

Valentin.

[Reinhold, 2010-11-22 18:51:55]

In scm/translation-functions.scm:
Why do you define two different variables count-markup and range-markup here,
one of which is always #f?
I would simply create one count-markup, defined as (if (pair? count-or-range)
(create markup for range) (create markup for count)). 
Then you don't need the (if count-markup...) here, either.

[Valentin Villenave, 2010-11-22 20:21:26]

On 2010/11/22 18:51:55, Reinhold wrote:
> In scm/translation-functions.scm:
> Why do you define two different variables count-markup and range-markup here,
> one of which is always #f?
> I would simply create one count-markup, defined as (if (pair? count-or-range)
> (create markup for range) (create markup for count)). 
> Then you don't need the (if count-markup...) here, either.

Indeed; as I told Carl I suspected it would be much harder, but it's quite
straightforward actually.

Here's a new patchset (the side-by-side diff seems to work, yay!). Thanks for
having a look!

[Neil Puttock, 2010-11-24 00:28:37]

Hi Valentin,

This generally looks fine (even if it runs a cart and horses through my patch
for issue 1205. ;)

The only thing I'm concerned about is the hybrid type; it's poorly named:
`number-or-pair?' doesn't suggest a number-pair, but `number-or-number-pair?' is
even worse.

I think using a list might a price worth paying to avoid creating a new
predicate.

Cheers,
Neil

http://codereview.appspot.com/3248042/diff/6001/input/regression/metronome-ra...
File input/regression/metronome-range.ly (right):

http://codereview.appspot.com/3248042/diff/6001/input/regression/metronome-ra...
input/regression/metronome-range.ly:11: \tempo 4 = 66~72
66 ~ 72

[Carl, 2010-11-24 01:16:46]

On 2010/11/24 00:28:37, Neil Puttock wrote:
> The only thing I'm concerned about is the hybrid type; it's poorly named:
> `number-or-pair?' doesn't suggest a number-pair, but `number-or-number-pair?'
is
> even worse.
> 
> I think using a list might a price worth paying to avoid creating a new
> predicate.

In my mind, it would be better to change the definition of number-or-pair? to

(or (number? x) (pair? x))

I think this number-or-pair? would be sufficient for this case.  It's certainly
as specific as a list? or cheap-list? check.

Thanks,

Carl

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

On 2010/11/24 01:16:46, Carl wrote:

> I think this number-or-pair? would be sufficient for this case.  It's
certainly
> as specific as a list? or cheap-list? check.

That sounds OK to me.

Cheers,
Neil

[Valentin Villenave, 2010-11-25 11:42:40]

On 2010/11/25 00:10:01, Neil Puttock wrote:
> That sounds OK to me.

Here's a new patch set; I've only changed the number-or-pair definition.

Thanks!
Valentin.

[Carl, 2010-11-25 15:24:34]

On 2010/11/25 11:42:40, Valentin Villenave wrote:
> On 2010/11/25 00:10:01, Neil Puttock wrote:
> > That sounds OK to me.
> 

In scm/translation-functions.scm, I'd prefer to see the argument called "count",
not "count-or-range".  The property is timeUnitCount, IIUC.  The count can
either be a number or a range, but it's still a count.

But I won't hold up approval of the patch for this; it's just an opinion.

Thanks,

Carl

[Neil Puttock, 2010-11-25 22:20:17]

On 2010/11/25 11:42:40, Valentin Villenave wrote:

> Here's a new patch set; I've only changed the number-or-pair definition.

Don't forget to change the docstring in lily.scm.

Cheers,
Neil

[Valentin Villenave, 2010-11-25 23:10:49]

On 2010/11/22 18:51:55, Reinhold wrote:
> Why do you define two different variables count-markup and range-markup here,
> one of which is always #f?
> I would simply create one count-markup, defined as (if (pair? count-or-range)
> (create markup for range) (create markup for count)). 
> Then you don't need the (if count-markup...) here, either.

Sorry Reinhold, I misunderstood your count-markup comment (I thought you were
referring to metronome-engraver.cc). You're right, it makes up for a much nicer
code.

New patch set, that addresses Carl's and Neil's comments as well. Thanks guys,
you've been really helpful!

Cheers,
Valentin.

[Neil Puttock, 2010-11-26 00:17:11]

On 2010/11/25 23:10:49, Valentin Villenave wrote:
> On 2010/11/22 18:51:55, Reinhold wrote:
> > Why do you define two different variables count-markup and range-markup
here,
> > one of which is always #f?
> > I would simply create one count-markup, defined as (if (pair?
count-or-range)
> > (create markup for range) (create markup for count)). 
> > Then you don't need the (if count-markup...) here, either.
> 
> Sorry Reinhold, I misunderstood your count-markup comment (I thought you were
> referring to metronome-engraver.cc). You're right, it makes up for a much
nicer
> code.

Where is this nicer code?  :)

Almost forgot: you need to tweak the display method for \tempo so

\tempo 4 = 60 ~ 63

doesn't look like this:

\tempo 4 = (60 . 63)

Cheers,
Neil

[Neil Puttock, 2010-11-26 00:17:43]

http://codereview.appspot.com/3248042/diff/23001/input/regression/metronome-r...
File input/regression/metronome-range.ly (right):

http://codereview.appspot.com/3248042/diff/23001/input/regression/metronome-r...
input/regression/metronome-range.ly:3: \header{
\header {

http://codereview.appspot.com/3248042/diff/23001/input/regression/metronome-r...
input/regression/metronome-range.ly:4: texidoc="
texidoc = "

[Valentin Villenave, 2010-11-26 00:30:39]

On 2010/11/26 00:17:43, Neil Puttock wrote:
> texidoc = "

Indeed. I just pasted it from another metronome-*.ly regtest.

Here's a suitable display-lily-music method. Thanks for your help, it's very
much appreciated!

Cheers,
Valentin.

[Neil Puttock, 2010-11-26 23:51:27]

On 2010/11/26 00:30:39, Valentin Villenave wrote:

> Here's a suitable display-lily-music method.

Great, but don't forget to do the same for this,

\tempo "foo" 60 ~ 63

and tests the display methods in display-lily-tests.ly (we're currently missing
tests for the newer \tempo commands anyway; you might as well add them too,
e.g., http://codereview.appspot.com/2145047/patch/1/2)

Once that's sorted, it should be ready to go.  Then I can set about undoing some
of your work when I rebase my patch. ;)

Cheers,
Neil

[Valentin Villenave, 2010-11-27 00:04:59]

On 2010/11/26 23:51:27, Neil Puttock wrote:
> Great, but don't forget to do the same for this,
> \tempo "foo" 60 ~ 63

Indeed.

> and tests the display methods in display-lily-tests.ly (we're currently
missing
> tests for the newer \tempo commands anyway; you might as well add them too,
> e.g., http://codereview.appspot.com/2145047/patch/1/2)

Good idea. I had totally forgotten about this regtest.

> Once that's sorted, it should be ready to go.  Then I can set about undoing
some
> of your work when I rebase my patch. ;)

I haven't really had a look at your patch so far, but it does look really
interesting! Look, it's really up to you whether I should push my patch first or
the other way around, I really don't mind rebasing my own patch onto your work
(if anything, this will be a good exercise).

Cheers,
Valentin.

[Neil Puttock, 2010-11-27 00:43:23]

On 2010/11/27 00:04:59, Valentin Villenave wrote:

> I haven't really had a look at your patch so far, but it does look really
> interesting! Look, it's really up to you whether I should push my patch first
or
> the other way around, I really don't mind rebasing my own patch onto your work
> (if anything, this will be a good exercise).

That's very kind of you, but I don't want to hold up your patch; it's been
reviewed thoroughly (I hope :), so go ahead and push.

Cheers,
Neil

[Valentin Villenave, 2010-11-30 20:41:46]

On 2010/11/27 00:43:23, Neil Puttock wrote:
> it's been
> reviewed thoroughly (I hope :), so go ahead and push.

Hi Neil, greetings everybody,
I've pushed this patch, so hopefully nothing will get broken. It is undocumented
as of yet, but I'll wait for Neil to complete his own work so that I can
possibly take this into account when writing the relevant documentation.

One caveat though: the display-lily regtest now outputs an unequal-thingy
because of the \tempo "blah" command that I added, upon Neil's suggestion:
Test 90 unequal: . 
in  = \tempo "Andante"
out = { \unset Score . tempoUnitDuration
  \unset Score . tempoUnitCount
  \set Score . tempoText = #"Andante"
  }
Then again, I'm pretty sure Neil's future patch will address this, so I'm not
too restless about it :-)

Cheers,
Valentin.