Use aligned-on-x-parent instead of other callbacks for some grobs

Fix bugs with aligning on main part of the NoteColumn
    
    NoteColumns may contain either noteheads or rests, so the function
    was changed to handle the case when NoteColumn contains only rest.
    
    ly_scm2bool returns true when the argument is unset - that's not
    what we want: unset properties should evaluate to false.



    Use aligned-on-x-parent instead of other callbacks for some grobs
    
    Now that we have an easy way of specifying different alignments for
    grob and its parent (see 0c4f221e5d), we can use aligned-on-.-parent
    in places that we previously used .-aligned-on-self.  With this change,
    the users have more control over the placement of grobs.
    
    Affected grobs:
    - InstrumentSwitch
    - Script
    - SostenutoPedal
    - SustainPedal
    - UnaCordaPedal

[Keith, 2014-08-10 19:38:02]

https://codereview.appspot.com/127860043/diff/1/lily/flag.cc
File lily/flag.cc (left):

https://codereview.appspot.com/127860043/diff/1/lily/flag.cc#oldcode40
lily/flag.cc:40: DECLARE_SCHEME_CALLBACK (calc_x_offset, (SCM));
The docs use this, so we need either convert-ly, or a change by hand as
appropriate.
In general, we need to search for use of functions in the docs, LSR, and
mutopiaproject, to know how to best handle their redefinition or removal.

[janek, 2014-08-10 21:10:57]

don't mess with Flag

[janek, 2014-08-17 15:08:13]

fix problems with TabNoteHeads and scripts attached to NoteColumns

[Keith, 2014-08-18 04:57:09]

Looks fine; 
just a minor unrelated change in code-organization looks unwise.

https://codereview.appspot.com/127860043/diff/40001/lily/lily-guile.cc
File lily/lily-guile.cc (right):

https://codereview.appspot.com/127860043/diff/40001/lily/lily-guile.cc#newcod...
lily/lily-guile.cc:201: robust_scm2bool (SCM b, bool def)
I am not sure, but I think this is a bad idea.
to_boolean() is robust to undefined symbols, returning #f, and a lot of
define-grobs.scm depends on that convention.
If you add this variant, and people start writing C++ that treats undefined
symbols as #t, we might get confused.

[janek, 2014-08-18 06:09:44]

https://codereview.appspot.com/127860043/diff/40001/lily/lily-guile.cc
File lily/lily-guile.cc (right):

https://codereview.appspot.com/127860043/diff/40001/lily/lily-guile.cc#newcod...
lily/lily-guile.cc:201: robust_scm2bool (SCM b, bool def)
On 2014/08/18 04:57:08, Keith wrote:
> I am not sure, but I think this is a bad idea.
> to_boolean() is robust to undefined symbols, returning #f, and a lot of
> define-grobs.scm depends on that convention.
> If you add this variant, and people start writing C++ that treats undefined
> symbols as #t, we might get confused.

Hmm.  Good point.  However, to me the situation without robust_scm2bool is also
confusing: in fact, i was confused enough to write buggy code :(
As far as i can see, the code for getting non-boolean properties  follows the
convention:

Type value = robust_scm2type (grob->get_property("prop-name"), default)

When writing code for issue 2245, I have initially tried to use robust_scm2bool
- which didn't exist.  As i was not aware of to_boolean, i ended up with
ly_scm2bool, which turned out to be a bad choice.

So, my question is: can we do something to make this more intuitive?

What about writing utility functions for getting and converting properties in
one step?  The current situation is not very elegant - the code sometimes gets
so long that it's hard to fit it in 80 chars limit.

[janek, 2014-08-19 06:53:06]

add missing interface to TabNoteHead

[dak, 2014-08-20 08:50:55]

On 2014/08/18 06:09:44, janek wrote:

> As far as i can see, the code for getting non-boolean properties  follows the
> convention:
> 
> Type value = robust_scm2type (grob->get_property("prop-name"), default)
> 
> When writing code for issue 2245, I have initially tried to use
robust_scm2bool
> - which didn't exist.

And it's pointless since unset options are #f by default and convention. 
Somewhat irritatingly, separately to to_boolean there _does_ exist a ly_scm2bool
(which is equivalent to scm_is_true and thus defaults to #t (!) when given a
non-boolean).  It is used a few times in our code base.  Most of the uses should
likely be replaced by to_boolean, some conceivably by scm_is_true.  And
afterwards, all instances of to_boolean should be renamed to ly_scm2bool and the
behavior of _that_ changed to be that of to_boolean.

> As i was not aware of to_boolean, i ended up with
> ly_scm2bool, which turned out to be a bad choice.
> 
> So, my question is: can we do something to make this more intuitive?

The most important answer to that question in this context is: not as part of
*this* issue.  Please use to_boolean here.  I remember being tripped up by
ly_scm2bool at some point of time in the past.  I'm definitely sympathetic to
changing its behavior to that of to_boolean and getting rid of to_boolean
altogether (after making sure that all current uses of ly_scm2bool are mapped to
the most appropriate of either to_boolean behavior under the guise of "new
ly_scm2bool" or to scm_is_true).  But not intermingled with this issue, and most
certainly not half-intermingled with this issue.

[janek, 2014-08-23 13:58:36]

Leave TabNoteHeads alone (opened issue 4073 for them)

[janek, 2014-08-23 14:01:57]

2014-08-20 10:50 GMT+02:00  <dak@gnu.org>:
>  Somewhat irritatingly, separately to to_boolean there _does_ exist a
> ly_scm2bool (which is equivalent to scm_is_true and thus defaults to #t
> (!) when given a non-boolean).

Yes, this is very unintuitive.

> It is used a few times in our code base.
>  Most of the uses should likely be replaced by to_boolean, some
> conceivably by scm_is_true.  And afterwards, all instances of to_boolean
> should be renamed to ly_scm2bool and the behavior of _that_ changed to
> be that of to_boolean.
>
>> So, my question is: can we do something to make this more intuitive?
>
> The most important answer to that question in this context is: not as
> part of *this* issue.  Please use to_boolean here.  I remember being
> tripped up by ly_scm2bool at some point of time in the past.  I'm
> definitely sympathetic to changing its behavior to that of to_boolean
> and getting rid of to_boolean altogether (after making sure that all
> current uses of ly_scm2bool are mapped to the most appropriate of either
> to_boolean behavior under the guise of "new ly_scm2bool" or to
> scm_is_true).  But not intermingled with this issue, and most certainly
> not half-intermingled with this issue.

You're right - patch updated accordingly.

thanks,
Janek

[janek, 2014-08-27 07:45:02]

Thanks for reviewing!  I have pushed this as

commit 48678617b169957433c562612151f2a71be50b59
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Sun Aug 10 21:09:42 2014 +0200

    Use aligned-on-x-parent instead of other callbacks for some grobs
    
    Now that we have an easy way of specifying different alignments for
    grob and its parent (see 0c4f221e5d), we can use aligned-on-.-parent
    in places that we previously used .-aligned-on-self.  With this change,
    the users have more control over the placement of grobs.
    
    Affected grobs:
    - InstrumentSwitch
    - Script
    - SostenutoPedal
    - SustainPedal
    - UnaCordaPedal

commit 2c279ba4a1602f72afa2940b51c5e50d8c5a4b60
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Sun Aug 17 16:14:11 2014 +0200

    Fix bugs with aligning on main part of the NoteColumn
    
    NoteColumns may contain either noteheads or rests, so the function
    was changed to handle the case when NoteColumn contains only rest.
    
    ly_scm2bool returns true when the argument is unset - that's not
    what we want: unset properties should evaluate to false.

[dak, 2014-08-29 22:21:31]

https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc
File lily/note-column.cc (right):

https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc#newco...
lily/note-column.cc:167: Grob *main_head;
This is a catastrophic error.  If get_stem (me) returns 0 and there are no grobs
in note-heads, main_head is not initialized at all but gets a random value.  The
rest of the code more likely than not then calls Grob::extent on this random
value.

It would have been easy to notice this error: GCC puts out a quite accurate
warning.

Ignoring GCC's warnings is rarely a good idea.

[pkx166h, 2014-08-30 09:35:26]

On 2014/08/29 22:21:31, dak wrote:
> https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc
> File lily/note-column.cc (right):
> 
>
https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc#newco...
> lily/note-column.cc:167: Grob *main_head;
> This is a catastrophic error.  If get_stem (me) returns 0 and there are no
grobs
> in note-heads, main_head is not initialized at all but gets a random value. 
The
> rest of the code more likely than not then calls Grob::extent on this random
> value.
> 
> It would have been easy to notice this error: GCC puts out a quite accurate
> warning.
> 
> Ignoring GCC's warnings is rarely a good idea.

Oh sorry. I would have thought perhaps Patchy would have 'borked' at some point?

[dak, 2014-08-30 09:45:02]

On 2014/08/30 09:35:26, J_lowe wrote:
> On 2014/08/29 22:21:31, dak wrote:
> > https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc
> > File lily/note-column.cc (right):
> > 
> >
>
https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc#newco...
> > lily/note-column.cc:167: Grob *main_head;
> > This is a catastrophic error.  If get_stem (me) returns 0 and there are no
> grobs
> > in note-heads, main_head is not initialized at all but gets a random value. 
> The
> > rest of the code more likely than not then calls Grob::extent on this random
> > value.
> > 
> > It would have been easy to notice this error: GCC puts out a quite accurate
> > warning.
> > 
> > Ignoring GCC's warnings is rarely a good idea.
> 
> Oh sorry. I would have thought perhaps Patchy would have 'borked' at some
point?

Warnings don't stop the compilation and it would be a bit much to ask for
testers to carefully read through all the log diffs.  It would probably make
sense if warnings stopped the compilation (GCC has an option for that), but
there is one warning from the Bison-generated parser.cc that would be very hard
to get rid of (sort of a Bison bug or at least annoyance).  Maybe we should just
add an exception for compiling that file and let g++ bomb out on all other
warnings.  People keep writing code triggering warnings without noticing, and I
estimate that probably half of them are not harmless and the others are usually
easy to get rid of, more often than not improving readability.

[janek, 2014-08-30 11:43:39]

2014-08-30 0:21 GMT+02:00  <dak@gnu.org>:
>
> https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc
> File lily/note-column.cc (right):
>
>
https://codereview.appspot.com/127860043/diff/80001/lily/note-column.cc#newco...
> lily/note-column.cc:167: Grob *main_head;
> This is a catastrophic error.  If get_stem (me) returns 0 and there are
> no grobs in note-heads, main_head is not initialized at all but gets a
> random value.  The rest of the code more likely than not then calls
> Grob::extent on this random value.

Ooops, i'm sorry for that.  I have limited internet access for the
weekend; i'll upload a fix for review when i'm back home (feel free to
make a fix yourself if you want).

> It would have been easy to notice this error: GCC puts out a quite
> accurate warning.
>
> Ignoring GCC's warnings is rarely a good idea.

I didn't yet manage to find this warning in make output.  It's really
hard to see valuable information there... :/

best,
Janek

[janek, 2014-09-03 22:20:00]

initialize pointer to 0

[janek, 2014-09-03 22:31:37]

Hi David,

i apologize for the delay - i'm loosing grip on my lilypond stuff :(

On 2014/08/30 09:45:02, dak wrote:
> On 2014/08/30 09:35:26, J_lowe wrote:
> > Oh sorry. I would have thought perhaps Patchy would have 'borked' at some
> point?
> 
> Warnings don't stop the compilation and it would be a bit much to ask for
> testers to carefully read through all the log diffs.  It would probably make
> sense if warnings stopped the compilation (GCC has an option for that), but
> there is one warning from the Bison-generated parser.cc that would be very
hard
> to get rid of (sort of a Bison bug or at least annoyance).  Maybe we should
just
> add an exception for compiling that file and let g++ bomb out on all other
> warnings.  People keep writing code triggering warnings without noticing, and
I
> estimate that probably half of them are not harmless and the others are
usually
> easy to get rid of, more often than not improving readability.

I think this makes sense.

best,
Janek

[janek, 2014-09-08 06:51:35]

pushed as 

commit dc067afa0915bb0ecbd34d2f7f413f616d893c32
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Thu Sep 4 00:17:55 2014 +0200

    Initialize main_head to 0 (in Note_column::calc_main_extent)
    
    Make sure that the pointer won't be assigned a random value.

[janek, 2014-09-08 06:51:36]

pushed as 

commit dc067afa0915bb0ecbd34d2f7f413f616d893c32
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Thu Sep 4 00:17:55 2014 +0200

    Initialize main_head to 0 (in Note_column::calc_main_extent)
    
    Make sure that the pointer won't be assigned a random value.