Hey all,
Responses to Neil and Joe below, and a new patchset posted.
I'd like this patch to go on another countdown so that we can fully sort
out the implementation of get_root_vertical_alignment.
Cheers,
MS
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly
File input/regression/span-bar-allow-span-bar.ly (right):
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly#newcode2
input/regression/span-bar-allow-span-bar.ly:2: \version "2.15.10"
On 2011/09/17 19:01:38, Neil Puttock wrote:
2.15.12
Done.
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly#newcode5
input/regression/span-bar-allow-span-bar.ly:5: texidoc = "The
SpanBarStub grob takes care of horizontal spacing for
On 2011/09/17 19:01:38, Neil Puttock wrote:
@code{SpanBarStub}
Done.
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly#newcode6
input/regression/span-bar-allow-span-bar.ly:6: SpanBar grobs. When the
SpanBar is disallowed, objects in contexts that
On 2011/09/17 19:01:38, Neil Puttock wrote:
@code{SpanBar}
Done.
http://codereview.appspot.com/4917046/diff/18001/input/regression/span-bar-allow-span-bar.ly#newcode14
input/regression/span-bar-allow-span-bar.ly:14: \repeat unfold 64 { c''8
} }
On 2011/09/17 19:01:38, Neil Puttock wrote:
fix indentation
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc
File lily/grob.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode593
lily/grob.cc:593: return get_maybe_root_vertical_alignment (g, 0);
On 2011/09/02 23:27:44, joeneeman wrote:
I still think this should say
return g->get_system ()->get_vertical_alignment ();
because there are several grobs that implement Align_interface and you
want to
make sure you get the right one.
When the grobs' root vertical alignment is accessed, get_system doesn't
work yet for BarLine grobs because their ancestry along the X_AXIS has
not yet been fully established. Grobs' Y_AXIS ancestry is established
earlier in a timestep.
Try replacing this function with:
Grob*
Grob::get_root_vertical_alignment (Grob *g)
{
System *s = g->get_system ();
return s ? s->get_vertical_alignment () : 0;
}
Span bar stubs will cease to work because get_system will, for BarLines,
always return 0.
I'm not saying that your solution is impossible - I can imagine somehow
setting BarLines' X-ancestors earlier in the translation process so that
get_system always yields a system, but my preliminary attempts to
implement this in this patch are coming up short and it seems like it
will be a larger undertaking. I'd rather push this as it is and then
have a discussion about whether get_root_vertical_alignment or
get_system->get_vertical_alignment is a cleaner way to get the root
vertical alignment. I see what you're saying that there are several
grobs that implement the Align_interface, but
get_root_vertical_alignment keeps recursing until it returns the one
that has no Y_AXIS parent. I believe that all grobs save
VerticalAlignment that implement the Align_interface have Y_AXIS
parents.
http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode617
lily/grob.cc:617: g->programming_error ("could not find this grob's
vertical axis group in the vertical alignment.");
On 2011/09/17 19:01:38, Neil Puttock wrote:
remove full stop/period
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode627
lily/grob.cc:627: g1->programming_error ("grob does not belong to a
Vertical Alignment?");
On 2011/09/17 19:01:38, Neil Puttock wrote:
VerticalAlignment
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/grob.cc#newcode643
lily/grob.cc:643: g1->programming_error ("could not situate this grob in
its axis group");
On 2011/09/17 19:01:38, Neil Puttock wrote:
prefer `place' to `situate'
I'm officially losing my English!
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc
File lily/pure-from-neighbor-engraver.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc#newcode46
lily/pure-from-neighbor-engraver.cc:46: pure_relevant_p_ =
ly_lily_module_constant ("pure-relevant?");
On 2011/09/17 19:01:38, Neil Puttock wrote:
you only use this once, so I'd get rid of it
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc#newcode52
lily/pure-from-neighbor-engraver.cc:52: if (to_boolean (scm_apply_1
(pure_relevant_p_, i.item ()->self_scm (), SCM_EOL))
On 2011/09/17 19:01:38, Neil Puttock wrote:
scm_call_1 ?
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc#newcode53
lily/pure-from-neighbor-engraver.cc:53: && !i.grob
()->internal_has_interface (ly_symbol2scm
("pure-from-neighbor-interface")))
On 2011/09/17 19:01:38, Neil Puttock wrote:
swap
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-engraver.cc#newcode57
lily/pure-from-neighbor-engraver.cc:57: // note that this can get outta
hand if there are lots of vertical axis groups...
On 2011/09/17 19:01:38, Neil Puttock wrote:
out of
In spite of the fact that I'm losing my English, my New-Jerseyisms are
not fading away.
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-interface.cc
File lily/pure-from-neighbor-interface.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/pure-from-neighbor-interface.cc#newcode34
lily/pure-from-neighbor-interface.cc:34: MAKE_SCHEME_CALLBACK
(Pure_from_neighbor_interface, elements_callback, 1);
On 2011/09/17 19:01:38, Neil Puttock wrote:
this sounds like it returns elements, whereas you're just processing
the
elements array
Renamed to filter_elements.
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-engraver.cc
File lily/span-bar-engraver.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-engraver.cc#newcode72
lily/span-bar-engraver.cc:72: Grob *vag =
Grob::get_root_vertical_alignment (bars_[0]);
On 2011/09/17 19:01:38, Neil Puttock wrote:
is this safe?
The presence of make_spanbar_ guarantees that bars_[0] will exist.
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc
File lily/span-bar-stub-engraver.cc (right):
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc#newcode4
lily/span-bar-stub-engraver.cc:4: Copyright (C) 1997--2011 Han-Wen
Nienhuys <han...@xs4all.nl>
On 2011/09/02 23:27:44, joeneeman wrote:
You're looking different today, Han-Wen.
Indeed he is...
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc#newcode55
lily/span-bar-stub-engraver.cc:55: // note that this can get outta hand
if there are lots of vertical axis groups...
On 2011/09/17 19:01:38, Neil Puttock wrote:
out of
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc#newcode116
lily/span-bar-stub-engraver.cc:116: //it->set_parent (y_parents[j],
Y_AXIS);
On 2011/09/17 19:01:38, Neil Puttock wrote:
remove?
Done.
http://codereview.appspot.com/4917046/diff/18001/lily/span-bar-stub-engraver.cc#newcode122
lily/span-bar-stub-engraver.cc:122: it->set_property ("Y-extent",
ly_interval2scm (Interval (infinity_f, -infinity_f)));
On 2011/09/17 19:01:38, Neil Puttock wrote:
SCM_BOOL_F
This doesn't work for some reason...
http://codereview.appspot.com/4917046/
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
