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

[address@hidden]

On Jan 6, 2012, at 2:35 PM, address@hidden 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