Don't report a programming error when trying to align grob with an empty extent

Don't report a programming error when trying to align grob with an empty extent
    
If a grob's extent is empty (undefined), alignment procedures from
Self_alignment_interface don't have any information about grob's
dimensions, so they cannot calculate the offset required to achieve
specified alignment of such grob.
    
However, this isn't actually a problem: if a grob's extent is empty,
this usually means that the grob itself is empty, and in that case
there is nothing to align at all.  Therefore, no programming error
should be reported.
    
For example, a user may remove some grob from output by overriding
its stencil to #f.  This will result in an empty extent, and Lily
shouldn't complain about being unable to align such grob, because
there is nothing to align.

[janek, 2013-03-18 14:18:34]

This is the part of Issue 3239 that introduced additional programming errors.  I
hope that the explanation is clear.

Of course there is one fundamental question: maybe we shouldn't complain about
grobs with empty extents, and just silently assume that an empty extent is
equivalent to (0 . 0)?

In other words, maybe this fragment of code should never report any programming
errors?

[janek, 2013-03-18 16:48:36]

report a programming error when trying to align on empty self

[MikeSol, 2013-03-19 15:14:49]

LGTM

[dak, 2013-03-21 06:57:46]

https://codereview.appspot.com/7533046/diff/3001/lily/self-alignment-interfac...
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7533046/diff/3001/lily/self-alignment-interfac...
lily/self-alignment-interface.cc:63: programming_error ("cannot align on self:
empty element");
"cannot align on self: empty element" seems like a nonsensical message.  An
empty element is more or less by definition aligned to anything.  If you want to
flag problems, it might make more sense to address them in those callers that
depend on non-emptiness than here.

[janek, 2013-03-21 08:04:42]

Thanks for review!

https://codereview.appspot.com/7533046/diff/3001/lily/self-alignment-interfac...
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7533046/diff/3001/lily/self-alignment-interfac...
lily/self-alignment-interface.cc:63: programming_error ("cannot align on self:
empty element");
On 2013/03/21 06:57:46, dak wrote:
> "cannot align on self: empty element" seems like a nonsensical message.  An
> empty element is more or less by definition aligned to anything. 

I agree that the sentence is weird.  What about "element shouldn't have empty
extent"?

> If you want to
> flag problems, it might make more sense to address them in those callers that
> depend on non-emptiness than here.

So, you think that we shouldn't report any programming error in these alignment
funcitons since an empty extent doesn't prevent us from doing our job (i.e.
aligning)?
(i'm just asking to make sure that i understood you correctly)
If so, i'll just delete all these error reports - fair enough for me.

[dak, 2013-03-21 08:14:51]

On 2013/03/21 08:04:42, janek wrote:

> So, you think that we shouldn't report any programming error in these
alignment
> funcitons since an empty extent doesn't prevent us from doing our job (i.e.
> aligning)?

Well, with an empty extent it would appear our job of aligning _self_ is already
done before we even start.

> (i'm just asking to make sure that i understood you correctly)
> If so, i'll just delete all these error reports - fair enough for me.

For me the problem only starts once one tries aligning a non-empty construct to
an empty one.  For _that_ we need a coping strategy.

That strategy might in some cases warrant flagging an error, in other cases it
might warrant escalating to aligning on the empty construct's parent instead.

I don't know the logic of the code and its uses, so this is basically my
unqualified gut feeling and nothing else.  Flagging and reporting a "problem"
without any user-visible consequences does not appear helpful to me.  What is
the user or programmer supposed to do?

Whether my characterization is accurate, I don't know either.  If it isn't,
function name and warning message could possibly be made more informative.

[janek, 2013-03-21 12:22:00]

On Thu, Mar 21, 2013 at 9:14 AM,  <dak@gnu.org> wrote:
> On 2013/03/21 08:04:42, janek wrote:
>
>> So, you think that we shouldn't report any programming error in these
>> alignment funcitons since an empty extent doesn't prevent us from
>> doing our job i.e. aligning)?
>
> Well, with an empty extent it would appear our job of aligning
> _self_ is already done before we even start.

yes, in a way.

>> (i'm just asking to make sure that i understood you correctly)
>> If so, i'll just delete all these error reports - fair enough for me.
>
> For me the problem only starts once one tries aligning a non-empty
> construct to an empty one.  For _that_ we need a coping strategy.

Not quite.  I mean, grob's parent must exist (even if its extent is empty),
and if the extent is empty, we can just return 0 offset.

> That strategy might in some cases warrant flagging an error, in other
> cases it might warrant escalating to aligning on the empty construct's
> parent instead.

Hmm.  Maybe, but no example comes to my mind.

> I don't know the logic of the code and its uses, so this is basically my
> unqualified gut feeling and nothing else.  Flagging and reporting a
> "problem" without any user-visible consequences does not appear helpful
> to me.  What is the user or programmer supposed to do?
>
> Whether my characterization is accurate, I don't know either.  If it
> isn't, function name and warning message could possibly be made more
> informative.

It is telling that (presumably after looking at the code at least
once) you don't know what the logic of the code is.  I find the naming
quite confusing indeed.
Please take a look at this comment with which i hope to explain the
idea of the code and its uses (you can see an older and newer version
here):
https://codereview.appspot.com/7768043/diff2/19001:25001/lily/self-alignment-...
maybe it'll help you.
I'll also write some more complete explanation and send it to the list.

best,
janek

[dak, 2013-03-21 12:37:50]

Janek Warchoł <janek.lilypond@gmail.com> writes:

> On Thu, Mar 21, 2013 at 9:14 AM,  <dak@gnu.org> wrote:
>
>> I don't know the logic of the code and its uses, so this is basically
>> my unqualified gut feeling and nothing else.  Flagging and reporting
>> a "problem" without any user-visible consequences does not appear
>> helpful to me.  What is the user or programmer supposed to do?
>>
>> Whether my characterization is accurate, I don't know either.  If it
>> isn't, function name and warning message could possibly be made more
>> informative.
>
> It is telling that (presumably after looking at the code at least
> once) you don't know what the logic of the code is.

No, in this case I actually did not bother to even look.  I looked at
the function name and the programming error message, and that was more
or less it.  For better or worse, comments putting a function into
perspective are almost universally absent in LilyPond, so understanding
how a function is supposed to be used involves searching through a
larger body of code.

Your patch is not responsible for this sad state of affairs, but of
course it does not help with reviewing it.

-- 
David Kastrup

[janek, 2013-03-23 15:44:02]

On Thu, Mar 21, 2013 at 1:21 PM, Janek Warchoł <janek.lilypond@gmail.com> wrote:
> On Thu, Mar 21, 2013 at 9:14 AM,  <dak@gnu.org> wrote:
>> I don't know the logic of the code and its uses, so this is basically my
>> unqualified gut feeling and nothing else. [...]
>
> It is telling that (presumably after looking at the code at least
> once) you don't know what the logic of the code is. [...]
> I'll write some more complete explanation and send it to the list.

It took a lot of time and became much longer than i expected, but eventually
i've written a detailed explanation of how alignment in Lilypond works - this
should "put [this] function into perspective":
http://lists.gnu.org/archive/html/lilypond-user/2013-03/msg00956.html

If i got everything right, you'll see in that explanation that an empty extent
isn't harmful (at least in self-alignment) - it just results in offset being 0.
So, the question is: in general, is it ok if a grob has empty extent?  Or should
extent always be some Interval (possibly zero)?
In other words: if an alignment method is called on an empty grob (see example
below), should it just say "well, i received an empty object, so what?  I can't
do anything with it, so i'll move on", or shout "hey, there are empty grobs
flying around!  Do something about it!".

Example: if we override grob's stencil to #f, the extent will be #f as well:

\version "2.17.12"
{ c' e' f' d' }
\addlyrics { la le \override LyricText #'stencil = ##f li lo }

- the above code reports "programming error: cannot align on self: empty
element".  Do we consider above code (and the use of \omit as currently defined)
a good practice?  Or do we want users to write

\version "2.17.12"
{ c' e' f' d' }
\addlyrics {
  la le
  \override LyricText #'stencil = ##f
  \override LyricText #'X-extent = #'(0 . 0)
  li lo
}

Bottom line: this patch is about consistency.  If we care about grobs having
non-empty extents, we should report programming errors in all self-alignment
methods.  If we don't care, we should not report any programming errors about
this.

I hope this makes this issue clear.  I'd really like to move forward with this.
best,
Janek

[t.daniels_treda.co.uk, 2013-03-23 16:38:21]

janek.lilypond@gmail.com wrote Saturday, March 23, 2013 3:44 PM

> If i got everything right, you'll see in that explanation that an empty
> extent isn't harmful (at least in self-alignment) - it just results in
> offset being 0.
> So, the question is: in general, is it ok if a grob has empty extent?
> Or should extent always be some Interval (possibly zero)?
> In other words: if an alignment method is called on an empty grob (see
> example below), should it just say "well, i received an empty object, so
> what?  I can't do anything with it, so i'll move on", or shout "hey,
> there are empty grobs flying around!  Do something about it!".
> 
> Example: if we override grob's stencil to #f, the extent will be #f as
> well:
> 
> \version "2.17.12"
> { c' e' f' d' }
> \addlyrics { la le \override LyricText #'stencil = ##f li lo }
> 
> - the above code reports "programming error: cannot align on self: empty
> element".  Do we consider above code (and the use of \omit as currently
> defined) a good practice?  

A 'programming error' should be issued only when an internal inconsistency
in LilyPond's code or data structures is detected.  Any such message should
always result in a bug being reported and tracked.  If the problem is due to
suspect user code a warning or error should be issued.  In this case maybe
a warning is appropriate.

Trevor

[janek, 2013-03-24 11:22:08]

Don't report a programming error

[janek, 2013-03-24 11:26:16]

On 2013/03/23 16:38:21, t.daniels_treda.co.uk wrote:
> A 'programming error' should be issued only when an internal inconsistency
> in LilyPond's code or data structures is detected.  Any such message should
> always result in a bug being reported and tracked.  If the problem is due to
> suspect user code a warning or error should be issued.  In this case maybe
> a warning is appropriate.

Thanks, Trevor!  This sheds some light on this situation.
After rereading David's comments i've come to the conclusion that there should
be no programming errors reported when extents are empty, so i've sent a new
patchset which makes Lily behaviour consistent in this regard.

thanks,
Janek

[Trevor Daniels, 2013-03-24 12:37:14]

I was a little concerned that problems might
result when a non-empty stencil was given an
empty extent, but as this passes all tests it
looks like this fear was unfounded.

So LGTM apart from a nitpick.

Trevor

https://codereview.appspot.com/7533046/diff/15001/lily/self-alignment-interfa...
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7533046/diff/15001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:153: // empty extent isn't a problem - we
simply don't align such grobs.
I think this comment would be better associated
with the if condition so it covers the entire
conditional clause.

[dak, 2013-03-24 12:45:38]

On 2013/03/24 12:37:14, Trevor Daniels wrote:
> I was a little concerned that problems might
> result when a non-empty stencil was given an
> empty extent, but as this passes all tests it
> looks like this fear was unfounded.

I don't think your fear is unfounded: there just does not seem to be any
sensible behavior when we have a non-empty stencil with an empty extent: you
can't align on an empty extent.

If we can detect this case reliably (non-empty stencil more or less being
defined as anything putting ink to paper) it may well be worth to warn about
this _particular_ variant.

[janek, 2013-03-24 14:26:36]

On 2013/03/24 12:45:38, dak wrote:
> On 2013/03/24 12:37:14, Trevor Daniels wrote:
> > I was a little concerned that problems might
> > result when a non-empty stencil was given an
> > empty extent, but as this passes all tests it
> > looks like this fear was unfounded.
> 
> I don't think your fear is unfounded: there just does not seem to be any
> sensible behavior when we have a non-empty stencil with an empty extent: you
> can't align on an empty extent.

I think there may be use-cases when someone could want to keep the stencil but
make its extent empty for some spacing purposes.  However, i didn't found any
example yet.

> If we can detect this case reliably (non-empty stencil more or less being
> defined as anything putting ink to paper) it may well be worth to warn about
> this _particular_ variant.

I'll look into this.

https://codereview.appspot.com/7533046/diff/15001/lily/self-alignment-interfa...
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7533046/diff/15001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:153: // empty extent isn't a problem - we
simply don't align such grobs.
On 2013/03/24 12:37:14, Trevor Daniels wrote:
> I think this comment would be better associated
> with the if condition so it covers the entire
> conditional clause.

Done.

[janek, 2013-03-24 14:27:19]

reword and move comment according to Trevor

[janek, 2013-03-24 15:00:39]

add a waning when stencil isn't empty.\

[janek, 2013-03-24 15:09:05]

On 2013/03/24 12:45:38, dak wrote:
> On 2013/03/24 12:37:14, Trevor Daniels wrote:
> > I was a little concerned that problems might
> > result when a non-empty stencil was given an
> > empty extent, but as this passes all tests it
> > looks like this fear was unfounded.
> 
> I don't think your fear is unfounded: there just does not seem to be any
> sensible behavior when we have a non-empty stencil with an empty extent: you
> can't align on an empty extent.
> 
> If we can detect this case reliably (non-empty stencil more or less being
> defined as anything putting ink to paper) it may well be worth to warn about
> this _particular_ variant.

Patchset 5 warns the user when the extent is empty and stencil isn't.
However, i'm not sure if we should warn about such situations at all;
i haven't found any similar warnings in the codebase. Also, detecting
non-empty stencils with empty extents is quite unrelated to alignment
code.

Please choose between patchsets 4 and 5.

thanks,
Janek

[Trevor Daniels, 2013-03-24 16:39:03]

> Patchset 5 warns the user when the extent is empty and stencil isn't.
> However, i'm not sure if we should warn about such situations at all;
> i haven't found any similar warnings in the codebase. Also, detecting
> non-empty stencils with empty extents is quite unrelated to alignment
> code.
> 
> Please choose between patchsets 4 and 5.

I prefer 5.  The warning does no harm if it is
never triggered, but is there in case some future
change elsewhere assumes a non-empty stencil will
be aligned even if its extent is empty.  It is
relevant to these alignment routines as they cannot
calculate the offset required to achieve the specified
alignment of such grobs, so should warn if a caller
appears to be expecting them to do that.

Trevor

[janek, 2013-03-26 21:49:40]

On 2013/03/24 16:39:03, Trevor Daniels wrote:
>
> > Please choose between patchsets 4 and 5.
> 
> I prefer 5.  The warning does no harm if it is
> never triggered, but is there in case some future
> change elsewhere assumes a non-empty stencil will
> be aligned even if its extent is empty.  It is
> relevant to these alignment routines as they cannot
> calculate the offset required to achieve the specified
> alignment of such grobs, so should warn if a caller
> appears to be expecting them to do that.

Aaaand the winner is... patchset 5!
Seriously though, Trevor's argument convinced me.

pushed as 1656075e24330067ff1dfa4d8253e5f3e5783014, closed.