Re: Issue 3330: redo much of the stencil stacking/spacing/empty-check (issue 8869044)

[dak]

On 2013/04/28 21:59:37, janek wrote:
> changes i see generally LGTM (but i may be missing something, in
particular
> Keith seems to make a good point).

> > Let Stencil::is_empty use Box::is_empty [...]

> I think the desription of this commit could use some more detail, for
example
> that we need (1 . -1) stencils for backspacing.

I disagree with that reasoning since it is backwards.  It's just simpler
if Stencil and Box use the same criteria (actually, I wanted to throw
out the check for the stencil expression being '() in Stencil::is_empty
() as well, but that tripped up something in the page breaker and it
looked like it would take a lot of time finding the cause).

This simplicity was part of the reason to change Box's is_empty tests.

> https://codereview.appspot.com/8869044/diff/9001/lily/box.cc
> File lily/box.cc (right):

> https://codereview.appspot.com/8869044/diff/9001/lily/box.cc#newcode66
> lily/box.cc:66: return interval_a_[a][LEFT] == empty[LEFT]
> Do we need to define 'empty'?  I.e. why not just write
> interval_a_[a][LEFT] == infinity_f ?
> Are we expecting that the definition of empty extent will change
further?

I am more worried that infinity_f might be defined with a different
type/precision than that stored in Interval, making the comparison fail.
 It's not like the C++ compiler would generate a lot of code here, and I
can probably even leave off the set_empty as it should be the default.

> https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc
> File lily/stencil.cc (right):

> https://codereview.appspot.com/8869044/diff/9001


https://codereview.appspot.com/8869044/diff/9001/lily/stencil.cc#newcode259
> lily/stencil.cc:259: offset += d * padding;
> I think this is worth a comment with an example.  It's not obvious why
sometimes
> we want padding and sometimes not.

The current version has a bit more here.

> https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm
> File scm/markup.scm (right):


https://codereview.appspot.com/8869044/diff/9001/scm/markup.scm#newcode67
> scm/markup.scm:67: between the end of each stencil and the reference
point of
> the following
> Hmm, shouldn't this read "between the end of the stencil and the
*beginning* of
> the following stencil"?

Depends on the version.  In the reviewed version, this is pretty
accurate, but now I've changed behavior again.

https://codereview.appspot.com/8869044/