rewrite Self_alignment_interface

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
I strongly recommend to view changes in a git branch,
not here (you'll be able to see individual commits and
follow the logic of changes instead of viewing one big
diff).  To do that, go to your lilypond git repository
and type
  git log --patch --reverse --color=always \
  master..origin/dev/janek/cleaned-alignment

thanks!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

66a8ff7 use X-parents to align MMRNumbers, MMRTexts and PercentRepeatCounters
12ab158 Remove x_centered_on_y_parent from Self_alignment_interface.
7d793a9 Self_alignment_interface: introduce align_grob (issue 3239).
4d7318f If a grob's parent is a PaperColumn, align using NoteColumn extent.
03b1173 Allow separate alignment "factors" for me and parent.
8424e5b Calculate dynamics alignment in a sane, configurable way.
20a9f06 Calculate fingerings alignment in a sane, configurable way.
2f5b7b3 Remove add_offset_callback, set_center_parent and set_align_self.
c626752 Remove special case in DynamicText alignment.
10e4327 Remove centered_on_note_columns from Self_alignment_interface.
Use align-grob in define-grobs.scm: [...] (several commits)
eafb4ee Remove centered_on_._parent from Self_alignment_interface.
Use align-grob in define-grobs.scm: [...] (several commits)
15de696 allow to use nonstandard extents for alignment

[janek, 2013-03-12 22:09:10]

Please review.
You'll find an illustration in the issue tracker:
http://code.google.com/p/lilypond/issues/detail?id=3239#c2

[janek, 2013-03-14 20:05:55]

return 0 when him is a PaperColumn to avoid regressions

[janek, 2013-03-14 20:07:12]

return 0 if him is a PaperColumn to avoid regressions

[janek, 2013-03-17 18:01:20]

ok, now i see that i've unnecessarily mixed two different things: rewriting
self-alignment-interface (this patch) and changing behaviour of unassociated
LyricTexts
(http://lists.gnu.org/archive/html/lilypond-devel/2013-03/msg00231.html).  These
changes are orthogonal, and i'll keep them separated in next patchsets.

JAnek

[janek, 2013-03-17 23:31:16]

rewrite. replaces all three methods in self-alignment-interface with one

[janek, 2013-03-18 07:44:06]

change explanation comment

[janek, 2013-03-18 12:43:18]

don't report additional programming errors

[MikeSol, 2013-03-19 15:28:45]

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

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:128: /* LilyPond positions every grob relative
to its parent in respective axis.
In general, try to avoid using the word LilyPond in comments, as it is in the
LilyPond code base.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:133: 
I don't see staff space being looked up anywhere in the function.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:144: 
With respect to my last review, this function should accept a pointer to a
vector of grob pointers:

vector<Grob *> &hims

so that you can do the note column stuff for lyrics.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:144: 
I don't have the energy to trigger another comment flame war, but a comment like
this may wind up diverging from what the code is doing (it already does with the
staff space thing).  Can you write the bare minimum and comment the code below
in places?  The code is already clear: I can tell what you're doing and why
you're doing it, so it speaks for itself.

[janek, 2013-03-19 21:04:16]

Hi Mike & all,

On 2013/03/19 15:21:37, MikeSol wrote:
> I like where you're going with this.

cool! :)

> I would recommend creating a function algined_on_grobs that accepts a grob, an
> vector of grobs to align to, and an axis.  Then algined_on_parent becomes a
> subset of this where the vector is of length one and is the parent.
> Then, you can create a function
> Self_alignment_interface::lyrics_algined_on_note_columns that invokes
> aligned_on_grobs with a vector of note columns returned using the search
method
> in this function. 

On 2013/03/19 15:28:45, MikeSol wrote:
> With respect to my last review, this function should accept a pointer to a
> vector of grob pointers:
> 
> vector<Grob *> &hims
> 
> so that you can do the note column stuff for lyrics.

I don't think this (defining special lyrics_algined_on_note_columns callback) is
optimal approach.

With your suggestion, here's how i expect alignment stuff would look like:
- there'd be a method Self_alignment_interface::aligned_on_grobs that can align
a grob on another grob(s)
- we'd have two callbacks: 
  Self_alignment_interface::lyrics_aligned_on_note_columns
  and
  Self_alignment_interface::lyrics_aligned_on_parent_notehead
- we'd have to decide when which callback should be used.  How should LyricText
definition in define-grobs.scm look like?  We should use
lyrics_aligned_on_note_columns when lyrics are unassociated, and
lyrics_aligned_on_parent_notehead when there is an associatedVoice - how to
write that in define-grobs?

And the same thing will be repeated for many other grobs, for example
DynamicTexts: we'll have to specify different callback for DynamicTexts attached
to NoteHeads, and different one for standalone Dynamics (e.g. in Dynamic
Context).

How alignment stuff would look like with my approach:
- aligned_on_grobs would be more complicated, because it'd have to grab
NoteColumn extent if 'him' turns out to be a PaperColumn
- we'd have one callback for everything
- we won't have to think about anything when defining grobs.  We'd just write
X-offset = ly:self-alignment-interface::align-grob, and it would be
aligned_on_grobs' job to align on appropriate grob if the default one (parent)
is inappropriate.

The difference is conceptual: aligning on NoteColumn is an exception, an
emergency exit needed only when grob doesn't have an ordinary parent.  As far as
i know, if grob's parent is a NoteHead or some other "normal" grob, we always
want to align on that; and when grob's parent is a PaperColumn, we always want
to align grob on an appropriate NoteColumn.

As for using a vector 'hims' instead of single 'him', maybe it's a good idea,
but i don't see any uses for it (besides your above suggestion, which seems to
be unnecessary).

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

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:128: /* LilyPond positions every grob relative
to its parent in respective axis.
On 2013/03/19 15:28:45, MikeSol wrote:
> In general, try to avoid using the word LilyPond in comments, as it is in the
> LilyPond code base.

Done.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:133: 
On 2013/03/19 15:28:45, MikeSol wrote:
> I don't see staff space being looked up anywhere in the function.

Well, staffspace is the unit in which XY-offsets are measured, so it's implied
by LilyPond herself.  I've reworded the comment to make it clearer.

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:144: 
On 2013/03/19 15:28:45, MikeSol wrote:
> I don't have the energy to trigger another comment flame war,
> but a comment like this may wind up diverging from what the code
> is doing (it already does with the staff space thing).
> Can you write the bare minimum and comment the code below in places?
> The code is already clear: I can tell what you're doing and why
> you're doing it, so it speaks for itself.

I've trimmed the comments and tried to make them less implementation-specific.
However, as a rule, i will continue to write as much comments as is reasonable
(of course, let me know whenever i write *too much* comments!), because of two
things:
- i really want to make reading the code /as easy as possible/ for a newcomer (i
know that i'd benefit from these comments if they were written when i was first
reading the code)
- i want to make review process as easy as possible, to encourage everyone
(including non-c++ people) to review my code ;)  My rule is "reading commit
message should make it clear what the patch does and why, and after first
reading of the code&comments it should be clear how it's done, at least at the
concept level".

[janek, 2013-03-19 21:04:48]

rewrite comments and interface description

[janek, 2013-03-19 21:33:45]

fix silly typo

[mike7, 2013-03-19 22:41:32]

On 19 mars 2013, at 22:04, janek.lilypond@gmail.com wrote:

> Hi Mike & all,
> 
> On 2013/03/19 15:21:37, MikeSol wrote:
>> I like where you're going with this.
> 
> cool! :)
> 
>> I would recommend creating a function algined_on_grobs that accepts a
> grob, an
>> vector of grobs to align to, and an axis.  Then algined_on_parent
> becomes a
>> subset of this where the vector is of length one and is the parent.
>> Then, you can create a function
>> Self_alignment_interface::lyrics_algined_on_note_columns that invokes
>> aligned_on_grobs with a vector of note columns returned using the
> search method
>> in this function.
> 
> On 2013/03/19 15:28:45, MikeSol wrote:
>> With respect to my last review, this function should accept a pointer
> to a
>> vector of grob pointers:
> 
>> vector<Grob *> &hims
> 
>> so that you can do the note column stuff for lyrics.
> 
> I don't think this (defining special lyrics_algined_on_note_columns
> callback) is optimal approach.
> 
> With your suggestion, here's how i expect alignment stuff would look
> like:
> - there'd be a method Self_alignment_interface::aligned_on_grobs that
> can align a grob on another grob(s)
> - we'd have two callbacks:
>  Self_alignment_interface::lyrics_aligned_on_note_columns
>  and
>  Self_alignment_interface::lyrics_aligned_on_parent_notehead
> - we'd have to decide when which callback should be used.  How should
> LyricText definition in define-grobs.scm look like?  We should use
> lyrics_aligned_on_note_columns when lyrics are unassociated, and
> lyrics_aligned_on_parent_notehead when there is an associatedVoice - how
> to write that in define-grobs?
> 
> And the same thing will be repeated for many other grobs, for example
> DynamicTexts: we'll have to specify different callback for DynamicTexts
> attached to NoteHeads, and different one for standalone Dynamics (e.g.
> in Dynamic Context).
> 
> How alignment stuff would look like with my approach:
> - aligned_on_grobs would be more complicated, because it'd have to grab
> NoteColumn extent if 'him' turns out to be a PaperColumn
> - we'd have one callback for everything
> - we won't have to think about anything when defining grobs.  We'd just
> write X-offset = ly:self-alignment-interface::align-grob, and it would
> be aligned_on_grobs' job to align on appropriate grob if the default one
> (parent) is inappropriate.
> 
> The difference is conceptual: aligning on NoteColumn is an exception, an
> emergency exit needed only when grob doesn't have an ordinary parent.
> As far as i know, if grob's parent is a NoteHead or some other "normal"
> grob, we always want to align on that; and when grob's parent is a
> PaperColumn, we always want to align grob on an appropriate NoteColumn.
> 
> As for using a vector 'hims' instead of single 'him', maybe it's a good
> idea, but i don't see any uses for it (besides your above suggestion,
> which seems to be unnecessary).

The main reason that I suggest this is that exceptions in the code base lead to
harder maintenance.  I am guilty of adding some from time to time (i.e. in
stencil integral) but I try to get rid of them as fast as possible.  One example
in the code base that I don't like: check out line 136.  Granted, whoever
created it doesn't like it either from the looks of the comment.  It makes the
code harder to read, maintain and understand.  The worst offender by far is the
metronome mark - there are so many exceptions concerning who its parent is that
it leads to actual problems with layout in certain cases.

I definitely like your idea of coming up with a single function, though. 
Perhaps try the following:

-) always decompose axis-groups into elements (recursively if need be)
-) create a non_musical flag that, if set, weeds out all grobs where non-musical
is set to #t

This may achieve the same effect but it is a lot more general, as the
distinction is between musical and non-musical rather than between note column
and everything else.

Cheers,
MS

[janek, 2013-03-19 23:28:14]

Hi,

On Tue, Mar 19, 2013 at 11:40 PM, mike@mikesolomon.org
<mike@mikesolomon.org> wrote:
> The main reason that I suggest this is that exceptions in the code base lead
to harder maintenance.
> I am guilty of adding some from time to time (i.e. in stencil integral) but I
try to get rid of them as fast as possible.
> One example in the code base that I don't like: check out line 136.

In which file?  line 136 of self-alignment-interface.cc from master is empty.

> Granted, whoever created it doesn't like it either from the looks of the
comment.
> It makes the code harder to read, maintain and understand.
> The worst offender by far is the metronome mark - there are so many
exceptions
> concerning who its parent is that it leads to actual problems with layout in
certain cases.

Sure, that's no good at all.

> I definitely like your idea of coming up with a single function, though. 
Perhaps try the following:
>
> -) always decompose axis-groups into elements (recursively if need be)

What do you mean by axis-groups?  'hims' (a vector of grobs that me
shoulld be aligned to)?

> -) create a non_musical flag that, if set, weeds out all grobs where
non-musical is set to #t

I'm not quite surehow non-musical grobs are related to alignment code.
 Do you mean that this flag would be responsible for "getting rid of
PaperColumn parent"?

cheers,
JAenk

[mike7, 2013-03-20 04:24:17]

On 20 mars 2013, at 00:27, Janek Warchoł <janek.lilypond@gmail.com> wrote:

> Hi,
> 
> On Tue, Mar 19, 2013 at 11:40 PM, mike@mikesolomon.org
> <mike@mikesolomon.org> wrote:
>> The main reason that I suggest this is that exceptions in the code base lead
to harder maintenance.
>> I am guilty of adding some from time to time (i.e. in stencil integral) but I
try to get rid of them as fast as possible.
>> One example in the code base that I don't like: check out line 136.
> 
> In which file?  line 136 of self-alignment-interface.cc from master is empty.

separation-item.cc - sorry for the omission.

> 
>> Granted, whoever created it doesn't like it either from the looks of the
comment.
>> It makes the code harder to read, maintain and understand.
>> The worst offender by far is the metronome mark - there are so many
exceptions
>> concerning who its parent is that it leads to actual problems with layout in
certain cases.
> 
> Sure, that's no good at all.
> 
>> I definitely like your idea of coming up with a single function, though. 
Perhaps try the following:
>> 
>> -) always decompose axis-groups into elements (recursively if need be)
> 
> What do you mean by axis-groups?  'hims' (a vector of grobs that me
> shoulld be aligned to)?
> 
>> -) create a non_musical flag that, if set, weeds out all grobs where
non-musical is set to #t
> 
> I'm not quite surehow non-musical grobs are related to alignment code.
> Do you mean that this flag would be responsible for "getting rid of
> PaperColumn parent"?
> 

I'll try to propose a patch later today - it's easier to do that than explaining
with words.

Cheers,
MS

[janek, 2013-03-20 22:18:40]

Hi all,

after some thinking i've come to a conclusion that we should keep this patch as
is - just a simple rewrite of self-alignment-interface, without lots of new
features and generalizations: they should be done in another patch.  Doing so
should make git history more logical: there will be a commit that just changes
code organization, but not it's functionality, and then the following commits
will build on top of that new code.

[janek, 2013-03-30 18:06:29]

lots of stuff. Please review in dev/janek-alignment branch

[MikeSol, 2013-03-30 18:15:11]

In general, I see a lot of reassigning of parents.  What is the goal with this
(sorry if you've explained this already)?

https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc
File lily/fingering-engraver.cc (right):

https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc#...
lily/fingering-engraver.cc:142: Self_alignment_interface::aligned_on_x_parent
(fingerings_[i]->self_scm ()));
This needs to be a chained offset procedure. See chain_offset_callback elsewhere
in the code.

https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc
File lily/paper-column.cc (right):

https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc#newcod...
lily/paper-column.cc:222: /*
Good work!

[janek, 2013-03-30 18:25:23]

On 2013/03/30 18:15:11, MikeSol wrote:
> In general, I see a lot of reassigning of parents.  What is the goal with this
> (sorry if you've explained this already)?

What do you mean by "reassigning of parents"?  I fetch grobs' parents often, but
i don't give grobs new parents.
 
>
https://codereview.appspot.com/7768043/diff/38001/lily/fingering-engraver.cc#...
> lily/fingering-engraver.cc:142: Self_alignment_interface::aligned_on_x_parent
> (fingerings_[i]->self_scm ()));
> This needs to be a chained offset procedure.
> See chain_offset_callback elsewhere
> in the code.

ok, will investigate.

>
https://codereview.appspot.com/7768043/diff/38001/lily/paper-column.cc#newcod...
> lily/paper-column.cc:222: /*
> Good work!

Thanks :)

Janek

[janek, 2013-03-31 22:26:30]

some progress.

[MikeSol, 2013-04-01 05:52:00]

I'd recommend adding a sort of padding property to the self-alignment-interface
to get it completely there.  That is, imagine that we right align to a grob and
we want to be padded by 0.1  We should be able to do that.  That'd allow
self-alignment-interface to be used for grobs like InstrumentName.

https://codereview.appspot.com/7768043/diff/46001/lily/new-fingering-engraver.cc
File lily/new-fingering-engraver.cc (right):

https://codereview.appspot.com/7768043/diff/46001/lily/new-fingering-engraver...
lily/new-fingering-engraver.cc:291: // don't overwrite offset property if it was
overridden by the user
Consider chaining an offset callback here.  See chain_callback and
chain_offset_callback.

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

https://codereview.appspot.com/7768043/diff/46001/lily/self-alignment-interfa...
lily/self-alignment-interface.cc:152: 
This would be the place to tack on padding.

https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:143: (X-extent . (-0.1 . 0.1)) ;UGH! how to get ambitusline
extent from its stencil?
Why can't you use the standard X extent function given in grob.cc?

[janek, 2013-04-01 11:13:38]

Hi all,

On Mon, Apr 1, 2013 at 7:52 AM,  <mtsolo@gmail.com> wrote:
> I'd recommend adding a sort of padding property to the
> self-alignment-interface to get it completely there.
> That is, imagine that we right align to a grob and we want
> to be padded by 0.1  We should be able to do that.
> That'd allow self-alignment-interface to be used
> for grobs like InstrumentName.

I think that this should be done by manipulating extents:
if i want an InstrumentName to be "horizontally-aligned-
with-padding", i add desired padding value to its X-extent,
and voilà!  LilyPond thinks that InstrumentName is slightly bigger,
aligns it as if it was slightly bigger, and thus it ends up padded.
What do you think?

As for chain_offset_callback, i've tried it (see attached patch), but
i get a strange error:

Processing `input/regression/add-stem-support.ly'
Parsing...
Interpreting music...
Preprocessing graphical objects...ERROR:
In procedure ly:self-alignment-interface::x-align-grob:
ERROR: Wrong number of arguments to
#<primitive-procedure ly:self-alignment-interface::x-align-grob>

quite frankly, i don't know what's wrong.
I've looked at other uses of chain_offset_callback
and it seems that i've written my code correctly.

PS i'll look into Ambitus extent later - right now i'm getting some
unexpected regressions after doing some additional modifications.

[mike7, 2013-04-01 11:20:40]

On 1 avr. 2013, at 14:13, Janek Warchoł <janek.lilypond@gmail.com> wrote:

> Hi all,
> 
> On Mon, Apr 1, 2013 at 7:52 AM,  <mtsolo@gmail.com> wrote:
>> I'd recommend adding a sort of padding property to the
>> self-alignment-interface to get it completely there.
>> That is, imagine that we right align to a grob and we want
>> to be padded by 0.1  We should be able to do that.
>> That'd allow self-alignment-interface to be used
>> for grobs like InstrumentName.
> 
> I think that this should be done by manipulating extents:
> if i want an InstrumentName to be "horizontally-aligned-
> with-padding", i add desired padding value to its X-extent,
> and voilà!  LilyPond thinks that InstrumentName is slightly bigger,
> aligns it as if it was slightly bigger, and thus it ends up padded.
> What do you think?

X-extent is used in many things other than self alignment positioning, so u
don't want to change extents. Every positioning in LilyPond has some notion of
padding save this interface. It would be a good addition, but could easily be in
a follow-up patch.

> 
> As for chain_offset_callback, i've tried it (see attached patch), but
> i get a strange error:
> 
> Processing `input/regression/add-stem-support.ly'
> Parsing...
> Interpreting music...
> Preprocessing graphical objects...ERROR:
> In procedure ly:self-alignment-interface::x-align-grob:
> ERROR: Wrong number of arguments to
> #<primitive-procedure ly:self-alignment-interface::x-align-grob>
> 
> quite frankly, i don't know what's wrong.
> I've looked at other uses of chain_offset_callback
> and it seems that i've written my code correctly.
> 

I'll look into it when I get home.

> PS i'll look into Ambitus extent later - right now i'm getting some
> unexpected regressions after doing some additional modifications.
> <0001-use-chain-offsets-instead-of-setting-properties.patch>

[janek, 2013-04-01 21:51:18]

On Mon, Apr 1, 2013 at 1:20 PM, mike@mikesolomon.org
<mike@mikesolomon.org> wrote:
> X-extent is used in many things other than self alignment positioning,
> so u don't want to change extents. Every positioning in LilyPond has
> some notion of padding save this interface. It would be a good addition,
> but could easily be in a follow-up patch.

Ok, i agree that some padding would be desirable.  What worries me is
the multitude of paddings we have - they give me a headache; i quite
frankly am lost in their different meanings.  As of now i'm unsure how
to implement such padding without making things more complicated, so i
suggest to wait until other design questions are answered.

>> As for chain_offset_callback, i've tried it (see attached patch),
>> but i get a strange error:
>>
>> Processing `input/regression/add-stem-support.ly'
>> Parsing...
>> Interpreting music...
>> Preprocessing graphical objects...ERROR:
>> In procedure ly:self-alignment-interface::x-align-grob:
>> ERROR: Wrong number of arguments to
>> #<primitive-procedure ly:self-alignment-interface::x-align-grob>
>>
>> quite frankly, i don't know what's wrong.
>> I've looked at other uses of chain_offset_callback
>> and it seems that i've written my code correctly.
>
> I'll look into it when I get home.

did you have any sucess with this stuff?

thanks,
janek

[mike7, 2013-04-02 04:26:29]

On 2 avr. 2013, at 00:51, janek.lilypond@gmail.com wrote:

> On Mon, Apr 1, 2013 at 1:20 PM, mike@mikesolomon.org
> <mike@mikesolomon.org> wrote:
>> X-extent is used in many things other than self alignment positioning,
>> so u don't want to change extents. Every positioning in LilyPond has
>> some notion of padding save this interface. It would be a good
> addition,
>> but could easily be in a follow-up patch.
> 
> Ok, i agree that some padding would be desirable.  What worries me is
> the multitude of paddings we have - they give me a headache; i quite
> frankly am lost in their different meanings.  As of now i'm unsure how
> to implement such padding without making things more complicated, so i
> suggest to wait until other design questions are answered.
> 
>>> As for chain_offset_callback, i've tried it (see attached patch),
>>> but i get a strange error:
> 
>>> Processing `input/regression/add-stem-support.ly'
>>> Parsing...
>>> Interpreting music...
>>> Preprocessing graphical objects...ERROR:
>>> In procedure ly:self-alignment-interface::x-align-grob:
>>> ERROR: Wrong number of arguments to
>>> #<primitive-procedure ly:self-alignment-interface::x-align-grob>
> 
>>> quite frankly, i don't know what's wrong.
>>> I've looked at other uses of chain_offset_callback
>>> and it seems that i've written my code correctly.
> 
>> I'll look into it when I get home.
> 
> did you have any sucess with this stuff?
> 

Try making the function accept a second optional argument with a previous offset
(check out the offset functions in side position interface)

Cheers,
MS

> thanks,
> janek
> 
> https://codereview.appspot.com/7768043/

[janek, 2013-04-02 11:42:14]

On 2013/04/02 04:26:29, mike7 wrote:
> On 2 avr. 2013, at 00:51, mailto:janek.lilypond@gmail.com wrote:
> 
> >>> As for chain_offset_callback, i've tried it (see attached patch),
> >>> but i get a strange error:
> >>>
> >>> Processing `input/regression/add-stem-support.ly'
> >>> Parsing...
> >>> Interpreting music...
> >>> Preprocessing graphical objects...ERROR:
> >>> In procedure ly:self-alignment-interface::x-align-grob:
> >>> ERROR: Wrong number of arguments to
> >>> #<primitive-procedure ly:self-alignment-interface::x-align-grob>
> >>>
> >>> quite frankly, i don't know what's wrong.
> >>> I've looked at other uses of chain_offset_callback
> >>> and it seems that i've written my code correctly.
> > 
> >> I'll look into it when I get home.
> > 
> > did you have any sucess with this stuff?
> 
> Try making the function accept a second optional argument with a previous
offset
> (check out the offset functions in side position interface)

I've tried that and it works, but the code got quite ugly - you can see results
in dev/janek-chain-optarg (i can abstract some of the duplicated code to a
function, but it will still be quite ugly because of an additional level of
abstraction).

I don't quite see what we gain from using chain_offset_callback - could you
explain why we shouldn't just set the property and be done?

Janek

https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/7768043/diff/46001/scm/define-grobs.scm#newcod...
scm/define-grobs.scm:143: (X-extent . (-0.1 . 0.1)) ;UGH! how to get ambitusline
extent from its stencil?
On 2013/04/01 05:52:00, MikeSol wrote:
> Why can't you use the standard X extent function given in grob.cc?

well, i've tried 
  (X-extent . ,ly:grob::stencil-width)
but i keep on getting the same "warning: AmbitusLine
has empty extent and non-empty stencil."

Besides, wouldn't Lily use that on her own if there was 
something there?  I mean, X-extent should be automatically 
derived from stencil anyway.  So, i think that the problem 
is in getting extent of a line stencil.

[janek, 2013-04-02 21:43:55]

simplify things a bit.

[janek, 2013-04-02 22:48:52]

On 2013/04/02 21:43:55, janek wrote:
> simplify things a bit.

Oops, sorry, wrong description.
This patch is just an intermediate state; it should work (my computer tells
there are no unwanted regtest changes), but there will be more changes, for
example centered_on_foo will get deleted.  Before that i'd like to solve
http://lists.gnu.org/archive/html/lilypond-devel/2013-04/msg00010.html - after
that things should get a bit cleaner.

reviews welcome!
Janek

[Trevor Daniels, 2013-04-03 17:12:14]

Code not tested or even eye-balled.

https://codereview.appspot.com/7768043/diff/76001/input/regression/text-spann...
File input/regression/text-spanner-attachment-alignment.ly (right):

https://codereview.appspot.com/7768043/diff/76001/input/regression/text-spann...
input/regression/text-spanner-attachment-alignment.ly:21: \repeat unfold 2 {c'4
_ \markup { "FAT" } }
What does (1 . -1) do?  I saw nothing in any of the
commit messages about changing the user interface
(but they are so long maybe I missed it)

https://codereview.appspot.com/7768043/diff/76001/input/regression/unassociat...
File input/regression/unassociated-lyrics-alignment.ly (right):

https://codereview.appspot.com/7768043/diff/76001/input/regression/unassociat...
input/regression/unassociated-lyrics-alignment.ly:12: \new Lyrics { \lyricmode 
{ foo2 bar aaa1 mmm1 } }
I see the durations on the syllables are the same as those
on the notes.  What happens if they differ?  In particular
if a syllable is placed between two notes?

https://codereview.appspot.com/7768043/diff/76001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/7768043/diff/76001/scm/define-grob-properties....
scm/define-grob-properties.scm:807: @code{'(-1 . 1)} will result in grobs' left
edge being aligned to
OK, I understand now.

[janek, 2013-04-03 18:00:37]

Thanks for the review!

On 2013/04/03 17:12:14, Trevor Daniels wrote:
> Code not tested or even eye-balled.

If you're considering eye-balling it, i strongly recommend viewing it in a
branch (dev/janek-alignment) - hopefully commit descriptions will help
understand respective changes in the code.

https://codereview.appspot.com/7768043/diff/76001/input/regression/unassociat...
File input/regression/unassociated-lyrics-alignment.ly (right):

https://codereview.appspot.com/7768043/diff/76001/input/regression/unassociat...
input/regression/unassociated-lyrics-alignment.ly:12: \new Lyrics { \lyricmode 
{ foo2 bar aaa1 mmm1 } }
On 2013/04/03 17:12:14, Trevor Daniels wrote:
> I see the durations on the syllables are the same as those
> on the notes.  What happens if they differ?  In particular
> if a syllable is placed between two notes?

Then the center of the syllable will be aligned to the appropriate point between
these notes, much like Dynamics attached to spacer rests:

\paper {
  ragged-right = ##f
}
{
  c'4\< c'\! d'\> e'\!
  << f'1 { s4 s4\< s4\> s4\! } >>
}

<<
  { c'4 c' d' e' f'1 }
  \new Lyrics \lyricmode {
    <4 !4 >4 !4
    \skip4 <4 >4 !4
  }
>>

(output attached to tracker issue)
Do you think that this should be part of the regtest?

[janek, 2013-04-04 14:23:34]

add regtest suggested by Trevor

[janek, 2013-04-08 23:46:31]

align MMR and perc.rep. stuff using xparents