Re: Improves horizontal spacing of axis groups that SpanBar grobs traverse. (issue 4917046)

[mtsolo]

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 <address@hidden>
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/