Re: reorganize self_alignment_interface (issue 7768043)

[janek . lilypond]

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-interface.cc
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/7768043/diff/19001/lily/self-alignment-interface.cc#newcode128
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-interface.cc#newcode133
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-interface.cc#newcode144
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".

https://codereview.appspot.com/7768043/