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

Issue 197690044: Function to display the rhythmic location of a grob

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 8 months ago by david.nalesnik
Modified:
4 years, 8 months ago
Reviewers:
dak, pwm
CC:
lilypond-devel_gnu.org
Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/
Visibility:
Public.

Description

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))

Patch Set 1 #

Patch Set 2 : rewrite in Scheme #

Total comments: 7

Patch Set 3 : two functions #

Patch Set 4 : texinfo nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -0 lines) Patch
M scm/output-lib.scm View 1 chunk +23 lines, -0 lines 0 comments Download

Messages

Total messages: 12
david.nalesnik
Please review. Thanks!
4 years, 8 months ago (2015-02-18 17:53:44 UTC) #1
dak
On 2015/02/18 17:53:44, david.nalesnik wrote: > Please review. Thanks! Can't this be done just as ...
4 years, 8 months ago (2015-02-18 18:02:38 UTC) #2
david.nalesnik
On 2015/02/18 18:02:38, dak wrote: > On 2015/02/18 17:53:44, david.nalesnik wrote: > > Please review. ...
4 years, 8 months ago (2015-02-18 18:05:17 UTC) #3
dak
On 2015/02/18 18:05:17, david.nalesnik wrote: > On 2015/02/18 18:02:38, dak wrote: > > On 2015/02/18 ...
4 years, 8 months ago (2015-02-18 18:07:19 UTC) #4
david.nalesnik
On 2015/02/18 18:07:19, dak wrote: > On 2015/02/18 18:05:17, david.nalesnik wrote: > > On 2015/02/18 ...
4 years, 8 months ago (2015-02-18 18:08:32 UTC) #5
david.nalesnik
On 2015/02/18 18:08:32, david.nalesnik wrote: Putting it into Scheme will need to wait for Issue ...
4 years, 8 months ago (2015-02-18 18:12:58 UTC) #6
david.nalesnik
On 2015/02/18 18:12:58, david.nalesnik wrote: > On 2015/02/18 18:08:32, david.nalesnik wrote: > > Putting it ...
4 years, 8 months ago (2015-02-18 20:39:19 UTC) #7
david.nalesnik
On 2015/02/18 20:39:19, david.nalesnik wrote: > On 2015/02/18 18:12:58, david.nalesnik wrote: > > On 2015/02/18 ...
4 years, 8 months ago (2015-02-18 20:43:51 UTC) #8
david.nalesnik
On 2015/02/18 20:43:51, david.nalesnik wrote: > > it seems that a > drawback of using ...
4 years, 8 months ago (2015-02-19 20:37:35 UTC) #9
pwm
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 ...
4 years, 8 months ago (2015-02-22 03:01:04 UTC) #10
dak
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 ...
4 years, 8 months ago (2015-02-22 07:59:40 UTC) #11
david.nalesnik
4 years, 8 months ago (2015-02-22 20:21:25 UTC) #12
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!
Sign in to reply to this message.

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