Re: PATCH: Lyrics break estimation of vertical spacing

[Boris Shingarov]

Hi Joe,
 
> You'll need to do something better here, rather than just leaving all
 > the extent information out for markup blocks.
 
Right, this is exactly why that oldest version of the patch was segfaulting.
The second version (attached to the bug and formatted
on codreview.appspot, also linked to from the bug) has this corrected.

 > >   * NB!!! These hangings are artificial in that they do not take into
 > >   * account any padding/spacing.
> hangings need to take padding (but not spacing) into account, because
 > padding is non-stretchable space. See (the old version of)
 > Page_breaking::min_page_count, which uses padding.
 
Correct; the code does do it, the meaning of the comment is
to warn that it happens not in the hanging method but in its caller;
this is ok because the hangings are only used to calculate the rod heights.
 
> >  void Line_details::compute_hangings (double previous_begin, double
 > > previous_rest)
 > we use Real rather than double (not quite sure why, but let's be
 > consistent).
 
I did put some thought into this;
it appears that in some places we actually use double,
and I *think* the pattern is, Real for allowing  /-Inf,
double when we want to specifilally allow proper finite values only.
I this case, the method demands the value of the argument to be
finite, this is why I chose double.
 
> Also, the code style is
 >
 > void
 > Line_details::compute_hangings ...
 
Thanks, corrected.
 
> >    double y_extent_; /* Y-extent, adjusted according to
> When do you set this?
 
In min_page_count().
 
> Doesn't full_height() remove the need for this
 > anyway?
 
No, full_height() is different.
full_height() is similar to the old unify() of the begin/rest;
y_extent_ is the fine-adjusted depending on the surrounding lines
(well, the previous line to speak correctly).
 
> >        if (i > 0)
 > >        {
 > indent the brace:
 > if (i > 0)
 >   {
 >     foo;
 >   }
 
Thanks.
 
> > -  rod_height_  = line.extent_.length ();
 > >    rod_height_  = line.y_extent_;
 > You'll need to use the same hanging logic here as you do in
 > min_page_count. Otherwise, min_page_count will be able to pack pages
 > tighter than the page-spacer can space them, which will cause problems.
 > Same for space_systems_on_2_pages and space_systems_on_1_page.
 
Thanks, I missed this!
 
> >    Grob *alignment = get_vertical_alignment (); //TODO check for null
> please check for null
 
But what do we do if it's null?
 
Boris