Re: Implements DOM-id property for grobs. (issue 5504106)
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 . http://codereview.appspot.com/5504106/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
Reviewers: Patrick McCarty, dak, mike_apollinemike.com, janek,
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#newcode81
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#newcode82
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.
Description:
Implements DOM-id property for grobs.
Please review this at http://codereview.appspot.com/5504106/
Affected files:
A input/regression/id.ly
M lily/grob.cc
M lily/stencil-interpret.cc
M scm/define-grob-properties.scm
M scm/define-stencil-commands.scm
M scm/output-ps.scm
M scm/output-svg.scm
Index: input/regression/id.ly
diff --git a/input/regression/id.ly b/input/regression/id.ly
new file mode 100644
index
..10c628f3a8a549c286c94d50e2deaf32660e7fee
--- /dev/null
+++ b/input/regression/id.ly
@@ -0,0 +1,9 @@
+\version 2.15.27
+
+\header {
+ texidoc = Shows the id property of a grob being set. This should have
+no effect in the PS backend.
+
+}
+
+{ \override NoteHead #'id = #foo c }
Index: lily/grob.cc
diff --git a/lily/grob.cc b/lily/grob.cc
index
6c2eba1710fb15a283f5ecd50249f3ee7f11320f..ed96f1d13a9b1386a4c64912c2d823684b397097
100644
--- a/lily/grob.cc
+++ b/lily/grob.cc
@@ -170,6 +170,17 @@ Grob::get_print_stencil () const
= *unsmob_stencil (scm_call_1 (ly_lily_module_constant
(stencil-whiteout),
retval.smobbed_copy ()));
}
+
+ SCM id = get_property (id);
+ if (scm_is_string (id))
+{
+ SCM expr = scm_list_3 (ly_symbol2scm (id),
+ id,
+ retval.expr ());
+
+ retval = Stencil (retval.extent_box (), expr);
+}
+
}
return retval;
@@ -784,6 +795,7 @@ ADD_INTERFACE (Grob,
cause
color
cross-staff
+ id
extra-X-extent
extra-Y-extent
extra-offset
Index: lily/stencil-interpret.cc
diff --git a/lily/stencil-interpret.cc b/lily/stencil-interpret.cc
index
1d89e032ba2559051bc8d489fc339eacc7450df2..8214af5dcc8e40a427dcd80212a0931a74bcef6f
100644
--- a/lily/stencil-interpret.cc
+++ b/lily/stencil-interpret.cc
@@ -74,6 +74,16 @@ interpret_stencil_expression (SCM expr,
return;
}
+ else if (head == ly_symbol2scm (id))
+{
+ SCM id = scm_cadr (expr);
+
+ (*func) (func_arg, scm_list_2 (ly_symbol2scm
(start-enclosing-id-node), id));
+ interpret_stencil_expression (scm_caddr (expr), func, func_arg,
o);
+ (*func) (func_arg, scm_list_1
Re: Implements DOM-id property for grobs. (issue 5504106)
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/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
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 http://codereview.appspot.com/5504106/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
On Jan 11, 2012, at 12:53 PM, janek.lilyp...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
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? http://codereview.appspot.com/5504106/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
On Jan 6, 2012, at 2:35 PM, d...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Implements DOM-id property for grobs. (issue 5504106)
m...@apollinemike.com m...@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 ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Implements DOM-id property for grobs. (issue 5504106)
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. https://codereview.appspot.com/5504106/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
