Function to display the rhythmic location of a grob

Function to display the rhythmic location of a grob

A convenient way to return the musical position of a grob within
a score will be very helpful in debugging, among other uses.

This patch creates a function ly:grob-rhythmic-location which
returns location as a list with the following structure:

(global-timestep (measure-number . measure-position))

[david.nalesnik, 2015-02-18 17:53:44]

Please review.  Thanks!

[dak, 2015-02-18 18:02:38]

On 2015/02/18 17:53:44, david.nalesnik wrote:
> Please review.  Thanks!

Can't this be done just as easily in Scheme?

[david.nalesnik, 2015-02-18 18:05:17]

On 2015/02/18 18:02:38, dak wrote:
> On 2015/02/18 17:53:44, david.nalesnik wrote:
> > Please review.  Thanks!
> 
> Can't this be done just as easily in Scheme?

Sure--that crossed my mind.  The reason I went with C++ is so that it would be
documented prominently, but I could change it if that's not a sufficient reason.

[dak, 2015-02-18 18:07:19]

On 2015/02/18 18:05:17, david.nalesnik wrote:
> On 2015/02/18 18:02:38, dak wrote:
> > On 2015/02/18 17:53:44, david.nalesnik wrote:
> > > Please review.  Thanks!
> > 
> > Can't this be done just as easily in Scheme?
> 
> Sure--that crossed my mind.  The reason I went with C++ is so that it would be
> documented prominently, but I could change it if that's not a sufficient
reason.

It should not be a sufficient reason.  Document-string it just as you would the
C++ version, and we'll need to come up with a good way to pull those into the
internals manual anyway.

[david.nalesnik, 2015-02-18 18:08:32]

On 2015/02/18 18:07:19, dak wrote:
> On 2015/02/18 18:05:17, david.nalesnik wrote:
> > On 2015/02/18 18:02:38, dak wrote:
> > > On 2015/02/18 17:53:44, david.nalesnik wrote:
> > > > Please review.  Thanks!
> > > 
> > > Can't this be done just as easily in Scheme?
> > 
> > Sure--that crossed my mind.  The reason I went with C++ is so that it would
be
> > documented prominently, but I could change it if that's not a sufficient
> reason.
> 
> It should not be a sufficient reason.  Document-string it just as you would
the
> C++ version, and we'll need to come up with a good way to pull those into the
> internals manual anyway.

OK, will do.

[david.nalesnik, 2015-02-18 18:12:58]

On 2015/02/18 18:08:32, david.nalesnik wrote:

Putting it into Scheme will need to wait for Issue 4289 to make it to master,
since I'll be able to use ly:item-get-column here.

[david.nalesnik, 2015-02-18 20:39:19]

On 2015/02/18 18:12:58, david.nalesnik wrote:
> On 2015/02/18 18:08:32, david.nalesnik wrote:
> 
> Putting it into Scheme will need to wait for Issue 4289 to make it to master,
> since I'll be able to use ly:item-get-column here.

However, we lose the nice error reporting which happens when you do
(ly:grob-name 2) for example.

[david.nalesnik, 2015-02-18 20:43:51]

On 2015/02/18 20:39:19, david.nalesnik wrote:
> On 2015/02/18 18:12:58, david.nalesnik wrote:
> > On 2015/02/18 18:08:32, david.nalesnik wrote:
> > 
> > Putting it into Scheme will need to wait for Issue 4289 to make it to
master,
> > since I'll be able to use ly:item-get-column here.
> 
> However, we lose the nice error reporting which happens when you do
> (ly:grob-name 2) for example.

Arghh,  Comment attached to wrong issue.  But the same applies: it seems that a
drawback of using Scheme is losing the nice error messages that result from
doing LY_ASSERT_SMOB.

[david.nalesnik, 2015-02-19 20:37:35]

On 2015/02/18 20:43:51, david.nalesnik wrote:
> 
> it seems that a
> drawback of using Scheme is losing the nice error messages that result from
> doing LY_ASSERT_SMOB.

False alarm.

[pwm, 2015-02-22 03:01:04]

LGTM, except for a couple minor things.  Thanks for working on these scheme
functions.

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode30
scm/output-lib.scm:30: "Return the rhythmic position of grob @var{g} as a list. 
The
Do you mean @var{grob} ?  That would be more in line with other functions in
this file.

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode42
scm/output-lib.scm:42: (list when rl))
Instead of this let block I'd probably just do:

(list (ly:grob-property col 'when) 
  (ly:grob-property col 'rhythmic-location))

But either way is fine with me.

[dak, 2015-02-22 07:59:40]

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode29
scm/output-lib.scm:29: (define-public (grob::rhythmic-location grob)
The function is misnamed since rhythmic-location is an established term in the
code base with a number of operators that will not work on the result from this
function.  A "rhythmic location" is just the cadr of what you return here.

So you should split this function into two, one for returning the global
timestep (there probably is an established term for that, but if not, grob::when
would be an obvious candidate), and one for returning the rhythmic location.

There is likely some duplication of code then, but since it will be the
exception rather than rule that one needs both values, the resulting code will
likely be more rather than less efficient for the typical use case.

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode30
scm/output-lib.scm:30: "Return the rhythmic position of grob @var{g} as a list. 
The
On 2015/02/22 03:01:04, pwm wrote:
> Do you mean @var{grob} ?  That would be more in line with other functions in
> this file.  

It doesn't matter as much whether it would be more in line with other functions
(though it is of course a consideration), but as written currently it is not
even in line with the actual function argument name.

[david.nalesnik, 2015-02-22 20:21:25]

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode29
scm/output-lib.scm:29: (define-public (grob::rhythmic-location grob)
On 2015/02/22 07:59:40, dak wrote:
> The function is misnamed since rhythmic-location is an established term in the
> code base with a number of operators that will not work on the result from
this
> function.  A "rhythmic location" is just the cadr of what you return here.
> 
> So you should split this function into two, one for returning the global
> timestep (there probably is an established term for that, but if not,
grob::when
> would be an obvious candidate), and one for returning the rhythmic location.
> 
> There is likely some duplication of code then, but since it will be the
> exception rather than rule that one needs both values, the resulting code will
> likely be more rather than less efficient for the typical use case.

Done.

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode30
scm/output-lib.scm:30: "Return the rhythmic position of grob @var{g} as a list. 
The
On 2015/02/22 03:01:04, pwm wrote:
> Do you mean @var{grob} ?  That would be more in line with other functions in
> this file.  

Done.

https://codereview.appspot.com/197690044/diff/20001/scm/output-lib.scm#newcode42
scm/output-lib.scm:42: (list when rl))
On 2015/02/22 03:01:04, pwm wrote:
> Instead of this let block I'd probably just do:
> 
> (list (ly:grob-property col 'when) 
>   (ly:grob-property col 'rhythmic-location))
> 
> But either way is fine with me.

Done--in a way!