layout.cc: do not draw empty boxes

layout.cc: do not draw empty boxes;
warn about, but do not draw, boxed with reversed dimensions

[joeneeman, 2012-08-12 10:11:43]

lgtm

[Graham Percival, 2012-08-12 14:32:17]

LGTM

[Keith, 2012-09-03 07:25:53]

http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc
File lily/lookup.cc (right):

http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc#newcode172
lily/lookup.cc:172: if (b.x ().length () < blotdiameter)
It is arguably valid to request a box running from right to left, for which
length() returns zero.  In this application, the Interval class abstraction is
not helping us.  We could defeat the abstraction with 
 if ( abs( b.x().delta() ) < blotdiameter)
    blotdiameter = abs( b.x().delta() );

[dak, 2012-09-03 08:16:27]

http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc
File lily/lookup.cc (right):

http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc#newcode172
lily/lookup.cc:172: if (b.x ().length () < blotdiameter)
On 2012/09/03 07:25:53, Keith wrote:
> It is arguably valid to request a box running from right to left, for which
> length() returns zero.  In this application, the Interval class abstraction is
> not helping us.  We could defeat the abstraction with 
>  if ( abs( b.x().delta() ) < blotdiameter)
>     blotdiameter = abs( b.x().delta() );

Well, "arguably valid" does not mean that we can let some of our code argue one
way, some of our code the other.

If we argue it is invalid, we should deliver a programming error here in order
to make people fix the callers.  If we argue it is valid, the code needs to be
able to deal with it.

Both the code in this function as well as the code in any function it calls.  Of
course, this becomes easier to guarantee if the code in this function will, when
in doubt, transform its input into a "proper" left-to-right box, giving the
followup code better guarantees than those it contends itself to work with.

[Keith, 2012-09-03 22:35:20]

http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc
File lily/lookup.cc (right):

http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc#newcode172
lily/lookup.cc:172: if (b.x ().length () < blotdiameter)
On 2012/09/03 08:16:27, dak wrote:

> Well, "arguably valid" does not mean that we can 
> let some of our code argue one way, some of our 
> code the other.
 
That is a nice description of the situation to date, in this area.  It has been,
though, an exceptionally quiet argument.

New patch up; I'll think about it.

[joeneeman, 2012-09-03 22:54:24]

On 2012/09/03 08:16:27, dak wrote:
> http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc
> File lily/lookup.cc (right):
> 
> http://codereview.appspot.com/6450113/diff/10001/lily/lookup.cc#newcode172
> lily/lookup.cc:172: if (b.x ().length () < blotdiameter)
> On 2012/09/03 07:25:53, Keith wrote:
> > It is arguably valid to request a box running from right to left, for which
> > length() returns zero.  In this application, the Interval class abstraction
is
> > not helping us.  We could defeat the abstraction with 
> >  if ( abs( b.x().delta() ) < blotdiameter)
> >     blotdiameter = abs( b.x().delta() );
> 
> Well, "arguably valid" does not mean that we can let some of our code argue
one
> way, some of our code the other.
> 
> If we argue it is invalid, we should deliver a programming error here in order
> to make people fix the callers.  If we argue it is valid, the code needs to be
> able to deal with it.

I argue that it is invalid. If you interpret a "right-to-left box" as being
non-empty, then the implementation of Box needs to be rewritten, since functions
such as add_point and widen don't act as expected.

[Keith, 2012-09-04 01:08:01]

On 2012/09/03 22:54:24, joeneeman wrote:
> > On 2012/09/03 07:25:53, Keith wrote:
> > > It is arguably valid to request a box running from right to left, 
> > > for which length() returns zero. 

> I argue that it is invalid. If you interpret a "right-to-left box" 
> as being non-empty, then the implementation of Box needs to be 
> rewritten, since functions such as add_point and widen don't act 
> as expected.

Surprisingly, only 'draw-thick-zero-fret tried to draw a box using an Interval
with negative length.  Since Intervals with negative length are regarded as
"empty" in LilyPond, we could refuse to draw them, but warn if they are
non-infinite, that is, something somebody might have wanted to draw.

New patch up.

[dak, 2012-09-17 09:59:52]

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm
File scm/fret-diagrams.scm (right):

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm#newcode649
scm/fret-diagrams.scm:649: (ordered-cons (car corner1) (car corner2))
I was wondering whether it would not make more sense to draw the whole thing in
one standard orientation first and then turn the result as a whole rather than
single coordinates.

However, that might make it harder to have lettering be always upright, and
might make the change more invasive overall.  So just food for thought, and
nothing that should make anybody except possibly the patch author himself stop
the countdown.

[Graham Percival, 2012-09-18 04:52:05]

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm
File scm/fret-diagrams.scm (right):

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm#newcode642
scm/fret-diagrams.scm:642: (corner1
from the context, I gather than corner1 is not guaranteed to be lower-left or
upper-right?  Could we get a comment stating that -- the fact that corner1 and
corner2 will be bottom-left and top-right, but not necessarily in that order?

[Keith, 2012-09-18 05:24:38]

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm
File scm/fret-diagrams.scm (right):

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm#newcode642
scm/fret-diagrams.scm:642: (corner1
On 2012/09/18 04:52:05, Graham Percival wrote:
>  Could we get a comment stating that -- the fact that corner1 and
> corner2 will be bottom-left and top-right, but not necessarily in that order?

After 'stencil-coordinates transforms them to coordinates on the page, they
could be top-left and bottom right.  I renamed them because thinking about which
corners they are is merely distraction.

  ;; corner1 and corner 2 are opposite corners of the thick fret

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm#newcode649
scm/fret-diagrams.scm:649: (ordered-cons (car corner1) (car corner2))
On 2012/09/17 09:59:52, dak wrote:
> I was wondering whether it would not make more sense to draw the whole thing
in
> one standard orientation first and then turn the result as a whole rather
than
> single coordinates.

That would be a complete rewrite.

> However, that might make it harder to have lettering be always upright,

It would seem to.

http://codereview.appspot.com/6450113/diff/8003/scm/fret-diagrams.scm#newcode650
scm/fret-diagrams.scm:650: (ordered-cons (cdr corner1) (cdr corner2))
 ;; Intervals should have limits increasing order (else they are considered
empty)