Implements DOM-id property for grobs.

Implements DOM-id property for grobs.

[Patrick McCarty, 2012-01-06 07:16:53]

LGTM

https://codereview.appspot.com/5504106/diff/1/scm/output-ps.scm
File scm/output-ps.scm (right):

https://codereview.appspot.com/5504106/diff/1/scm/output-ps.scm#newcode258
scm/output-ps.scm:258: (define (open-node n) n)
I don't see this procedure used anywhere...

https://codereview.appspot.com/5504106/diff/1/scm/output-ps.scm#newcode261
scm/output-ps.scm:261: (define (close-node n) n)
.. or this one.

[dak, 2012-01-06 13:35:03]

Is there a reason you have ignored Patrick's comments?

The issue is missing a description, so is any part of the code.

It needs at least a regtest demonstrating the use of this feature, otherwise
nobody will be able to ever put this code to actual use (or write user
documentation for it) if you were to be hit by a bus.

Of course it would be better if you would write the user documentation yourself,
but without even a regtest for bootstrapping, nobody else can be expected to be
able to do it.

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

http://codereview.appspot.com/5504106/diff/1/lily/grob.cc#newcode174
lily/grob.cc:174: SCM DOM_id = get_property ("DOM-id");
This probably deserves a comment to that effect that this must come last, or the
effect of the DOM-id on the generated XML (whatever that may be) will not
encompass the whole stencil.

http://codereview.appspot.com/5504106/diff/1/scm/framework-svg.scm
File scm/framework-svg.scm (right):

http://codereview.appspot.com/5504106/diff/1/scm/framework-svg.scm#newcode116
scm/framework-svg.scm:116: (define (comment s)
What is this for?  You changed the definition of the routine as well as moving
it, removing the space padding.  Why would that be a good idea?  As a
consequence, you had to change existing callers.

http://codereview.appspot.com/5504106/diff/1/scm/output-svg.scm
File scm/output-svg.scm (right):

http://codereview.appspot.com/5504106/diff/1/scm/output-svg.scm#newcode306
scm/output-svg.scm:306: (comment " FIXME: how to select glyph by name, altglyph
is broken? "))
What is the deal with adding the space here?

[mike_apollinemike.com, 2012-01-06 13:45:26]

On Jan 6, 2012, at 2:35 PM, dak@gnu.org wrote:

> Is there a reason you have ignored Patrick's comments?
> 

They were incorporated into the version that was pushed to staging.

> The issue is missing a description, so is any part of the code.
> 
> It needs at least a regtest demonstrating the use of this feature,
> otherwise nobody will be able to ever put this code to actual use (or
> write user documentation for it) if you were to be hit by a bus.
> 

Will do.

> Of course it would be better if you would write the user documentation
> yourself, but without even a regtest for bootstrapping, nobody else can
> be expected to be able to do it.
> 
> 
> http://codereview.appspot.com/5504106/diff/1/lily/grob.cc
> File lily/grob.cc (right):
> 
> http://codereview.appspot.com/5504106/diff/1/lily/grob.cc#newcode174
> lily/grob.cc:174: SCM DOM_id = get_property ("DOM-id");
> This probably deserves a comment to that effect that this must come
> last, or the effect of the DOM-id on the generated XML (whatever that
> may be) will not encompass the whole stencil.

Will do.

> 
> http://codereview.appspot.com/5504106/diff/1/scm/framework-svg.scm
> File scm/framework-svg.scm (right):
> 
> http://codereview.appspot.com/5504106/diff/1/scm/framework-svg.scm#newcode116
> scm/framework-svg.scm:116: (define (comment s)
> What is this for?  You changed the definition of the routine as well as
> moving it, removing the space padding.  Why would that be a good idea?
> As a consequence, you had to change existing callers.
> 

I also got rid of this in the patch that got reverted from staging.

> http://codereview.appspot.com/5504106/diff/1/scm/output-svg.scm
> File scm/output-svg.scm (right):
> 
> http://codereview.appspot.com/5504106/diff/1/scm/output-svg.scm#newcode306
> scm/output-svg.scm:306: (comment " FIXME: how to select glyph by name,
> altglyph is broken? "))
> What is the deal with adding the space here?
> 

I also fixed this in the reverted patch.  Unfortunately, I did not save the
final pushed version of the patch - do you have a copy of it?  If so, I won't
need to make the changes again.

Thanks for the comments!  I'll post a new patch in the coming days.

Cheers,
MS

[dak, 2012-01-06 13:58:31]

"mike@apollinemike.com" <mike@apollinemike.com> writes:

> I also fixed this in the reverted patch.  Unfortunately, I did not
> save the final pushed version of the patch - do you have a copy of it?
> If so, I won't need to make the changes again.

Unless you scrapped your whole repository,

git cherry-pick 6abea44be29f2de2abbc840507c9184132c8024d

should revive that patch as the last commit, and you can then work with
git commit --amend for changing parts of it.

In case you need to revive stuff that is no longer in any branch
git reflog
will almost always help.

Git starts garbage collecting inaccessible commits after about 3 months
by default.  Plenty of time to get them back if one really messed up.

-- 
David Kastrup

[janek, 2012-01-11 11:53:40]

Hi Mike,

could you add some comments to the code and/or commit message explaining what it
does?  I've read whole patch and i don't understand what happens here, except
that it's some kind of XML identifier.

tia,
Janek

[mike_apollinemike.com, 2012-01-11 12:27:10]

On Jan 11, 2012, at 12:53 PM, janek.lilypond@gmail.com wrote:

> Hi Mike,
> 
> could you add some comments to the code and/or commit message explaining
> what it does?  I've read whole patch and i don't understand what happens
> here, except that it's some kind of XML identifier.
> 
> tia,
> Janek


Hey Janek,

The idea is to have a property of Grob, DOM-id, that results in the creation of
two stencils that enclose the visual representation of the grob in a DOM node. 
These two stencils bookend all other stencils that comprise the grob.

Currently, the only DOM-friendly lilypond backend is svg.  Because a grob's
visual representation may have more than one stencil, all of the stencils need
to be contained in a node that has this id.  In svg speak, this is <g
id="foo"></g>(in html it would be <div id="foo"></div>).

So, your intuition is right - all it is is an XML identifier.  Nothing actually
"happens" as far as layout goes.  It's just useful for people who harvest data
from DOM-friendly documents that lilypond generates to do strange things with
music.

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.

Cheers,
MS

[janek, 2012-01-21 18:15:24]

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#ne...
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#ne...
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?

[MikeSol, 2012-01-23 17:47:25]

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#newcode177
lily/grob.cc:177: SCM expr = scm_list_3 (ly_symbol2scm ("id"),
On 2012/01/21 18:15:25, janek wrote:
> isn't 'id' a scheme-thingy already?  I mean, in line 174, it is declared as
SCM,
> so why convert it with ly_symbol2scm?

We checked previously for a grob property called "id" that needs to be a string.
 Here, we are creating a stencil.  All stencils are represented internally by
lists where the first element is the name of the stencil in symbol form.

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode178
lily/grob.cc:178: id,
On 2012/01/21 18:15:25, janek wrote:
> why store id two times?

The first id is the symbol id.
The second is the variable "id" that contains something else (foobar or
whatever).

http://codereview.appspot.com/5504106/diff/11013/lily/grob.cc#newcode179
lily/grob.cc:179: retval.expr ());
On 2012/01/21 18:15:25, janek wrote:
> do i understand correcly that retval stands for "return value" and is some
kind
> of an object?

Yup.  It should always be another stencil.  This is a convention int he code
base.

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

This is a way of saying "the retval is equal to the old retval wrapped in
something new".  Here, the new wrapping is an id.  Earlier in grob.cc, it's a
color.

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#ne...
lily/stencil-interpret.cc:81: (*func) (func_arg, scm_list_2 (ly_symbol2scm
("start-enclosing-id-node"), id));
On 2012/01/21 18:15:25, janek wrote:
> this line is longer than 80 chars, do we care about it?

I don't think so.

http://codereview.appspot.com/5504106/diff/11013/lily/stencil-interpret.cc#ne...
lily/stencil-interpret.cc:82: interpret_stencil_expression (scm_caddr (expr),
func, func_arg, o);
On 2012/01/21 18:15:25, janek wrote:
> i understand that we extract actual stencil here and interpret it again, but
> what these func's do?

Depends what the func is.  It's a way to pass a function as an argument.  Look
for interpret_stencil_expression calls and you'll see what's passed in & why.

[janek, 2012-02-14 10:55:27]

Mike, 

again i was outrageously slow, i apologize.
Thanks for the explanations, i see now that the code is pretty clear when you're
accustomed to our code base a little.
I think the docstring you wrote is everything that's needed for this, so no need
to do anything more here.

Sorry for delay.  I really appreciate that you are willing to explain all this.

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#newcode178
lily/grob.cc:178: id,
Ah, so expr will be something like
(a-scheme-symbol-saying-"id", the-actual-id-of-the-object, the-stencil)

I see now that i missed "".