Implements the 2nd of 5 legs of getting footnotes up and running.

Implements the 2nd of 5 legs of getting footnotes up and running.

Gets the annotations to print next to the footnoted grobs.

The 3rd leg will be merging Balloon and FootnoteItem into one grob
that is responsible for annotations.

The 4th will be implementing endnotes.

The 5th will be automatic numbering.

[Neil Puttock, 2011-03-06 23:35:28]

Hi Mike,

Just a few minor nitpicks for you.

Cheers,
Neil

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc
File lily/balloon.cc (right):

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc#newcode38
lily/balloon.cc:38: static SCM internal_balloon_print (Grob *me, Grob *p, Offset
&off);
Offset *off (makes code fussy due to dereferencing)

or

Offset off (better: all existing code appears to pass Offset by value)

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc#newcode75
lily/balloon.cc:75: spanner_placement = 1.0;
 Real spanner_placement =
   min (1.0, max (robust_scm2double (me->get_property ("spanner-placement"),
-1.0), -1.0));

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc#newcode93
lily/balloon.cc:93: Drul_array<Item *> bounds;
remove

http://codereview.appspot.com/4254055/diff/1/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4254055/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:51: (annotation-balloon ,boolean? "Print the
ballon around an annotation.")
Print the balloon

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (left):

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#oldcode872
scm/define-grobs.scm:872: (Y-extent . 0.0)
Y-extent . #f

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#oldcode887
scm/define-grobs.scm:887: (Y-extent . 0.0)
Y-extent . #f

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#newcode180
scm/define-grobs.scm:180: (annotation-balloon . #f)
remove

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#newcode181
scm/define-grobs.scm:181: (annotation-line . #f)
remove

[Neil Puttock, 2011-03-06 23:37:55]

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#newcode180
scm/define-grobs.scm:180: (annotation-balloon . #f)
On 2011/03/06 23:35:28, Neil Puttock wrote:
> remove

Oops, I mean change to #t (same for annotation-line)

[MikeSol, 2011-03-06 23:46:56]

Thanks Neil!

New patch set uploaded.

Cheers,
Mike

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc
File lily/balloon.cc (right):

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc#newcode38
lily/balloon.cc:38: static SCM internal_balloon_print (Grob *me, Grob *p, Offset
&off);
On 2011/03/06 23:35:28, Neil Puttock wrote:
> Offset *off (makes code fussy due to dereferencing)
> 
> or
> 
> Offset off (better: all existing code appears to pass Offset by value)

Done.

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc#newcode75
lily/balloon.cc:75: spanner_placement = 1.0;
On 2011/03/06 23:35:28, Neil Puttock wrote:
>  Real spanner_placement =
>    min (1.0, max (robust_scm2double (me->get_property ("spanner-placement"),
> -1.0), -1.0));

Done.

http://codereview.appspot.com/4254055/diff/1/lily/balloon.cc#newcode93
lily/balloon.cc:93: Drul_array<Item *> bounds;
On 2011/03/06 23:35:28, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/4254055/diff/1/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4254055/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:51: (annotation-balloon ,boolean? "Print the
ballon around an annotation.")
On 2011/03/06 23:35:28, Neil Puttock wrote:
> Print the balloon

Done.

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (left):

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#oldcode872
scm/define-grobs.scm:872: (Y-extent . 0.0)
On 2011/03/06 23:35:28, Neil Puttock wrote:
> Y-extent . #f

Done.

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#oldcode887
scm/define-grobs.scm:887: (Y-extent . 0.0)
On 2011/03/06 23:35:28, Neil Puttock wrote:
> Y-extent . #f

Done.

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4254055/diff/1/scm/define-grobs.scm#newcode180
scm/define-grobs.scm:180: (annotation-balloon . #f)
On 2011/03/06 23:37:55, Neil Puttock wrote:
> On 2011/03/06 23:35:28, Neil Puttock wrote:
> > remove
> 
> Oops, I mean change to #t (same for annotation-line)
> 

Done.

[Neil Puttock, 2011-03-07 22:27:00]

LGTM.

http://codereview.appspot.com/4254055/diff/6001/lily/balloon.cc
File lily/balloon.cc (right):

http://codereview.appspot.com/4254055/diff/6001/lily/balloon.cc#newcode47
lily/balloon.cc:47: if (Item *item = dynamic_cast<Item *>(me))
dynamic_cast<Item *> (me))

[mike_apollinemike.com, 2011-03-09 08:21:11]

On Mar 7, 2011, at 11:27 PM, n.puttock@gmail.com wrote:

> LGTM.
> 
> 
> http://codereview.appspot.com/4254055/diff/6001/lily/balloon.cc
> File lily/balloon.cc (right):
> 
> http://codereview.appspot.com/4254055/diff/6001/lily/balloon.cc#newcode47
> lily/balloon.cc:47: if (Item *item = dynamic_cast<Item *>(me))
> dynamic_cast<Item *> (me))
> 

Thanks!

Pushed.

Cheers,
MS

[hanwenn, 2011-03-11 13:44:21]

Hi Mike,

can fix the below points before continuing with pushing stuff for this
patch series?

On Sun, Mar 6, 2011 at 8:46 PM,  <mtsolo@gmail.com> wrote:
> +// ugh...code dup...hopefully can be consolidated w/ above one day

can you make a priority to do this right now?  If it's not done
directly, 'one day' usually does not happen at all.

> +      int pos = orig->spanned_rank_interval ()[LEFT];
> +      Real spanner_placement = min (1.0,
> +                                    max (robust_scm2double
> (me->get_property ("spanner-placement"), -1.0),
> +                                         -1.0));
> +
> +      spanner_placement = (spanner_placement + 1.0) / 2.0;
> +      int rpos = orig->spanned_rank_interval ()[RIGHT];
> +      pos = (int)((rpos - pos) * spanner_placement + pos + 0.5);
> +
> +      if (pos < me->spanned_rank_interval () [LEFT])
> +        return SCM_EOL;

Can you rethink this? The rank numbers are not good measures of
anything.  In particular, adding a new voice/staff with different
rhythmic texture will cause these numbers to fluctuate, and mess up
your positioning.  I still think this is a overall bad idea, and would
love to see examples where users need placement of footnotes, but
placement so inexact that they do not want to use per-item
positioning.  One possibility is to use column->when() as a measure,
as it is least somewhat invariant, but preferably I'd like to scrap
this (especially given the code dup and overall ugliness) until we
have proof that it's really needed.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mike_apollinemike.com, 2011-03-11 17:00:40]

On Mar 11, 2011, at 2:44 PM, Han-Wen Nienhuys wrote:

> Hi Mike,
> 
> can fix the below points before continuing with pushing stuff for this
> patch series?
> 
> On Sun, Mar 6, 2011 at 8:46 PM,  <mtsolo@gmail.com> wrote:
>> +// ugh...code dup...hopefully can be consolidated w/ above one day
> 
> can you make a priority to do this right now?  If it's not done
> directly, 'one day' usually does not happen at all.

Sorry - the consolidation already happened in the internal print function.  I
forgot
to remove the comment.

> 
>> +      int pos = orig->spanned_rank_interval ()[LEFT];
>> +      Real spanner_placement = min (1.0,
>> +                                    max (robust_scm2double
>> (me->get_property ("spanner-placement"), -1.0),
>> +                                         -1.0));
>> +
>> +      spanner_placement = (spanner_placement + 1.0) / 2.0;
>> +      int rpos = orig->spanned_rank_interval ()[RIGHT];
>> +      pos = (int)((rpos - pos) * spanner_placement + pos + 0.5);
>> +
>> +      if (pos < me->spanned_rank_interval () [LEFT])
>> +        return SCM_EOL;
> 
> Can you rethink this? The rank numbers are not good measures of
> anything.  In particular, adding a new voice/staff with different
> rhythmic texture will cause these numbers to fluctuate, and mess up
> your positioning.  I still think this is a overall bad idea, and would
> love to see examples where users need placement of footnotes, but
> placement so inexact that they do not want to use per-item
> positioning.  One possibility is to use column->when() as a measure,
> as it is least somewhat invariant, but preferably I'd like to scrap
> this (especially given the code dup and overall ugliness) until we
> have proof that it's really needed.

Done.

http://codereview.appspot.com/4286042/

Cheers,
MS