Re: Implements DOM-id property for grobs. (issue 5504106)

[janek . lilypond]

Hi Mike,

i apologize for the delay; i focused on other things that seemed more
urgent to me.

On 2012/01/11 12:27:10, mike_apollinemike.com wrote:
> [explanation of the patch]

> I'm not sure how/where to include this info in the source: if you can
think of a
> good way to phrase it that would make sense to other people, I'd be
happy to
> include it in the patch.

I'm thinking about it (as a matter of fact, the new docstring looks
nice!), but i need to understand your code better.  Below are my
questions.


http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc
File lily/grob.cc (right):

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode128
lily/grob.cc:128: retval = *m;
so retval is the current stencil, but only when it's not empty?

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode175
lily/grob.cc:175: if (scm_is_string (id))
I understand that we're doing something if the grob already has an id
set.

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode177
lily/grob.cc:177: SCM expr = scm_list_3 (ly_symbol2scm ("id"),
isn't 'id' a scheme-thingy already?  I mean, in line 174, it is declared
as SCM, so why convert it with ly_symbol2scm?

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode178
lily/grob.cc:178: id,
why store id two times?

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode179
lily/grob.cc:179: retval.expr ());
do i understand correcly that retval stands for "return value" and is
some kind of an object?

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode181
lily/grob.cc:181: retval = Stencil (retval.extent_box (), expr);
Do we overwrite the retval that was set earlier (in other if's)?  Why?

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc
File lily/stencil-interpret.cc (right):

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#newcode81
lily/stencil-interpret.cc:81: (*func) (func_arg, scm_list_2
(ly_symbol2scm ("start-enclosing-id-node"), id));
this line is longer than 80 chars, do we care about it?

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#newcode82
lily/stencil-interpret.cc:82: interpret_stencil_expression (scm_caddr
(expr), func, func_arg, o);
i understand that we extract actual stencil here and interpret it again,
but what these func's do?

http://codereview.appspot.com/5504106/