Issue 2245: always align dynamics and lyrics on "main" notehead

NOTE: ===============================================
Since this patch depends on patch for https://code.google.com/p/lilypond/issues/detail?id=3978 (https://codereview.appspot.com/105410046), i have uploaded that code as "Patchset 1".  So, the actual patch will be in the following patchsets, starting with 2.
=====================================================

Issue 2245: always align dynamics and lyrics on "main" notehead

Until now, LyricTexts and DynamicTexts had their X-parents set to
the first NoteHead in the NoteColumn.  This resulted in inconsistent
alignment - placement of lyrics and dynamics depended on the order
of notes in the input:

% this was aligned differently
{ <f' g'>1\p <g' f'>\p }
\addlyrics { la la }

By using NoteColumns themselves as the X-parents, we make sure that
the input order won't matter.  Since the NoteColumn contains all NoteHeads
(including suspended ones, which usually should be ignored when aligning),
we add a function for calculating X-extent of the "main" part of the
NoteColumn, i.e. without suspended NoteHeads.

[janek, 2014-06-29 23:25:50]

Since this patch depends on patch for
https://code.google.com/p/lilypond/issues/detail?id=3978
(https://codereview.appspot.com/105410046), i have uploaded that code as
"Patchset 1".  So, the actual patch will be in the Patchset 2.

[janek, 2014-06-29 23:27:16]

Actual patch - always align dynamics and lyrics on "main" notehead

[janek, 2014-06-29 23:40:25]

Hi folks,

please review.

@Han-Wen: in Script_engraver, you wrote (commit 286fcad779) that aligning stuff
like Scripts on NoteColumns is "not correct" (lines 256-261).  However, to me it
seems that using NoteColumns as X-parents for Lyrics, Dynamics etc. is exactly
the way to solve issue 2245
(https://code.google.com/p/lilypond/issues/detail?id=2245), and that's what this
patch does.  It deals with suspended noteheads using a flag that tells
self-alignment-interface to ignore their extents.

As far as I can see this is a sound solution (especially that if anyone wanted
to include suspended notes in alignment for whatever reason, doing so is
trivial).  Do you agree?

best,
Janek

[janek, 2014-06-30 18:41:21]

don't segfault when there are no stems.

[Mark Polesky, 2014-07-01 19:35:38]

This patch is problematic for me.

Firstly, when a lyric syllable extends to the next note (as with a slur or tie),
it is my understanding that the lyric text should be left-aligned with the
left-most note of the chord.  See Issue 3521:
http://code.google.com/p/lilypond/issues/detail?id=3521

This patch prevents the 3521 workaround from working, which is bad IMO.

Secondly, to my eye, I think the ideal alignment for a center-aligned syllable
on a harmonic second ("suspended") dyad would be center-aligned with the stem,
and not with the NoteColumn.  No idea how easy or hard that would be to code,
but that's what I would vote for.  The "corrected" example given in comment #1
here looks too far to the right to me, and so do the dynamics in the following
comment:
http://code.google.com/p/lilypond/issues/detail?id=2245

- Mark

https://codereview.appspot.com/108270044/diff/40001/input/regression/text-spa...
File input/regression/text-spanner-attachment-alignment.ly (right):

https://codereview.appspot.com/108270044/diff/40001/input/regression/text-spa...
input/regression/text-spanner-attachment-alignment.ly:21: \repeat unfold 2 {c'4
_ \markup { "LONG" } }
git complains about trailing whitespace here.

[janek, 2014-07-02 06:27:15]

Hi,

2014-07-01 21:35 GMT+02:00  <markpolesky@gmail.com>:
> This patch is problematic for me.
>
> Firstly, when a lyric syllable extends to the next note (as with a slur
> or tie), it is my understanding that the lyric text should be
> left-aligned with the left-most note of the chord.  See Issue 3521:
> http://code.google.com/p/lilypond/issues/detail?id=3521
>
> This patch prevents the 3521 workaround from working, which is bad IMO.
>
> Secondly, to my eye, I think the ideal alignment for a center-aligned
> syllable on a harmonic second ("suspended") dyad would be center-aligned
> with the stem, and not with the NoteColumn.

Actually, NoteColumn contains all noteheads (including suspended
ones), so aligning to NoteColumn is the same as aligning to stem (in
case of chords with seconds).

> No idea how easy or hard
> that would be to code,

With my patch that you are reviewing?  Trivial - it's already there ;-)

While I think that it's better to always align lyrics to the "main
notehead", I knew that some people woul prefer to do this otherwise,
so the patch allows to choose how to align LyricTexts (and
DynamicTexts):

\markup "Aligned on main notehead (default):"

\new Staff {
  <g' f'>2~ q
  <e'' d''>2~ q
  <g' f'>1
}
\addlyrics { la __ la __ la }

\markup "Aligned on whole notecolumn:"

\new Staff {
  <g' f'>2~ q
  <e'' d''>2~ q
  <g' f'>1
}
\addlyrics {
  \override LyricText.X-align-on-main-noteheads = ##f
  la __ la __ la
}

Tadaaaaa!! (see the output here:
https://lilypond.googlecode.com/issues/attachment?aid=22450016000&name=2245-e...)
And the alignment will be consistent regardless of input order -
either aligned on main notehead, or on the whole notecolumn, depending
on property X-align-on-main-noteheads.

As i didn't know that this problem affects LyricExtenders, this
version of my patch isn't changing their behaviour at all.  I'll look
into the code and try to make them consistent as well.

How do you like this? ;-)

>
https://codereview.appspot.com/108270044/diff/40001/input/regression/text-spa...
> input/regression/text-spanner-attachment-alignment.ly:21: \repeat unfold
> 2 {c'4 _ \markup { "LONG" } }
> git complains about trailing whitespace here.

It was already there before, but i'll remove it in the next patchset.

best,
Janek

[Mark Polesky, 2014-07-02 18:58:11]

Janek Warchoł wrote:
> While I think that it's better to always align lyrics to
> the "main notehead", I knew that some people would prefer
> to do this otherwise, so the patch allows to choose how to
> align LyricTexts (and DynamicTexts)

Okay, this is less problematic than I thought it was, and 
I'm slowly convincing myself that for LyricText,
main-notehead-alignment is better than NoteColumn-alignment.
However, for DynamicText, I think NoteColumn-alignment is
preferable, especially when the main notehead is on the 
right of the stem.  Gould (p.103) writes:

  "When vertical space is limited, move a dynamic to the 
  left of the note -- never to the right, since the note has 
  already started!"

> \override LyricText.X-align-on-main-noteheads = ##f 

I think this interface (with booleans) is confusing; I would
prefer a choice of symbols, something like this:

  \override LyricText.X-alignment-anchor = #'NoteHead
  \override LyricText.X-alignment-anchor = #'NoteColumn

>> git complains about trailing whitespace here.
> 
> It was already there before, but i'll remove it in the 
> next patchset.

I have this in my .vimrc:

  " Remove trailing whitespace on write
  autocmd BufWritePre * :%s/\s\+$//e

- Mark

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

https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm#newc...
scm/define-grobs.scm:589: (X-offset .
,ly:self-alignment-interface::aligned-on-x-parent)
Not a requirement, but you might as well alphabetize the prop-list while you're
there (case-insensitive, so 'X-extent comes after 'vertical-skylines).  Same for
the other prop-lists you modify below.

[janek, 2014-07-02 19:22:30]

Hi,

2014-07-02 20:58 GMT+02:00  <markpolesky@gmail.com>:
> Janek Warchoł wrote:
>>
>> While I think that it's better to always align lyrics to
>> the "main notehead", I knew that some people would prefer
>> to do this otherwise, so the patch allows to choose how to
>> align LyricTexts (and DynamicTexts)
>
>
> Okay, this is less problematic than I thought it was, and
> I'm slowly convincing myself that for LyricText,
> main-notehead-alignment is better than NoteColumn-alignment.
> However, for DynamicText, I think NoteColumn-alignment is
> preferable, especially when the main notehead is on the
> right of the stem.  Gould (p.103) writes:
>
>   "When vertical space is limited, move a dynamic to the
>   left of the note -- never to the right, since the note has
>   already started!"

Hm.  Sounds reasonable.  On the other hand, if you have many staves
with dynamics, it's better if these dynamics are aligned - and with
alignment on NoteColumns, this would not happen.

>> \override LyricText.X-align-on-main-noteheads = ##f
>
>
> I think this interface (with booleans) is confusing; I would
> prefer a choice of symbols, something like this:
>
>   \override LyricText.X-alignment-anchor = #'NoteHead
>   \override LyricText.X-alignment-anchor = #'NoteColumn

Yeah, i'm also thinking about providing an interface like this in the
future iterations of the alignment rewrite.  However, can we push this
patch as-is, and make such changes later?  The thing is, there will be
so many changes in the alignment code that trying to come up with a
generic interface now (in the middle of work) is IMO a waste of
effort, because i'd continue to rewrite such code with every next
patch.  I think it will be much more efficient to first get all the
features in (using simple solutions), and then look at the resulting
code and generalize that - when we'll know what exactly we need.

>>> git complains about trailing whitespace here.
>> It was already there before, but i'll remove it in the
>> next patchset.
>
> I have this in my .vimrc:
>
>   " Remove trailing whitespace on write
>   autocmd BufWritePre * :%s/\s\+$//e

bad luck that i'm not using vim :)

> https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
>
>
https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm#newc...
> scm/define-grobs.scm:589: (X-offset .
> ,ly:self-alignment-interface::aligned-on-x-parent)
> Not a requirement, but you might as well alphabetize the prop-list while
> you're there (case-insensitive, so 'X-extent comes after
> 'vertical-skylines).  Same for the other prop-lists you modify below.

Ok, i'll remember this.

best,
Janek

[janek, 2014-07-02 19:26:11]

PS

2014-07-02 20:58 GMT+02:00  <markpolesky@gmail.com>:
> Okay, this is less problematic than I thought it was, and
> I'm slowly convincing myself that for LyricText,
> main-notehead-alignment is better than NoteColumn-alignment.
> However, for DynamicText, I think NoteColumn-alignment is
> preferable, especially when the main notehead is on the
> right of the stem.

The "ultimate goal" that i have in my mind is to have an interface
that will allow to easily say things like "align this object on the
main column in such cases, on the stem in other cases, and on the
whole notecolumn in yet other cases.  And in some cases find the
average center of the noteheads and align on that.".

[janek, 2014-07-04 20:47:24]

Hi Mark,

do you have any objections against putting this patch on countdown?
As i wrote, this is a transitional phase: the patch improves the
situation, but i intend to make more changes when it becomes clear how
they should look like.

best,
Janek

PS i've fixed whitespace/ordering issues in the patch for issue 3978,
where they belong.

2014-07-02 21:22 GMT+02:00 Janek Warchoł <janek.lilypond@gmail.com>:
> Hi,
>
> 2014-07-02 20:58 GMT+02:00  <markpolesky@gmail.com>:
>> Janek Warchoł wrote:
>>>
>>> While I think that it's better to always align lyrics to
>>> the "main notehead", I knew that some people would prefer
>>> to do this otherwise, so the patch allows to choose how to
>>> align LyricTexts (and DynamicTexts)
>>
>>
>> Okay, this is less problematic than I thought it was, and
>> I'm slowly convincing myself that for LyricText,
>> main-notehead-alignment is better than NoteColumn-alignment.
>> However, for DynamicText, I think NoteColumn-alignment is
>> preferable, especially when the main notehead is on the
>> right of the stem.  Gould (p.103) writes:
>>
>>   "When vertical space is limited, move a dynamic to the
>>   left of the note -- never to the right, since the note has
>>   already started!"
>
> Hm.  Sounds reasonable.  On the other hand, if you have many staves
> with dynamics, it's better if these dynamics are aligned - and with
> alignment on NoteColumns, this would not happen.
>
>>> \override LyricText.X-align-on-main-noteheads = ##f
>>
>>
>> I think this interface (with booleans) is confusing; I would
>> prefer a choice of symbols, something like this:
>>
>>   \override LyricText.X-alignment-anchor = #'NoteHead
>>   \override LyricText.X-alignment-anchor = #'NoteColumn
>
> Yeah, i'm also thinking about providing an interface like this in the
> future iterations of the alignment rewrite.  However, can we push this
> patch as-is, and make such changes later?  The thing is, there will be
> so many changes in the alignment code that trying to come up with a
> generic interface now (in the middle of work) is IMO a waste of
> effort, because i'd continue to rewrite such code with every next
> patch.  I think it will be much more efficient to first get all the
> features in (using simple solutions), and then look at the resulting
> code and generalize that - when we'll know what exactly we need.
>
>>>> git complains about trailing whitespace here.
>>> It was already there before, but i'll remove it in the
>>> next patchset.
>>
>> I have this in my .vimrc:
>>
>>   " Remove trailing whitespace on write
>>   autocmd BufWritePre * :%s/\s\+$//e
>
> bad luck that i'm not using vim :)
>
>> https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm
>> File scm/define-grobs.scm (right):
>>
>>
https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm#newc...
>> scm/define-grobs.scm:589: (X-offset .
>> ,ly:self-alignment-interface::aligned-on-x-parent)
>> Not a requirement, but you might as well alphabetize the prop-list while
>> you're there (case-insensitive, so 'X-extent comes after
>> 'vertical-skylines).  Same for the other prop-lists you modify below.
>
> Ok, i'll remember this.
>
> best,
> Janek

[Mark Polesky, 2014-07-04 21:21:47]

On 2014/07/04 20:47:24, janek wrote:
> Hi Mark,
> 
> do you have any objections against putting this patch on countdown?

No objections, but I'll leave it to someone else to give the green light since
I'm not qualified to evaluate the C++ code.

> As i wrote, this is a transitional phase: the patch improves the
> situation, but i intend to make more changes when it becomes clear how
> they should look like.

Sounds good.

[janek, 2014-07-04 21:40:00]

2014-07-04 23:21 GMT+02:00  <markpolesky@gmail.com>:
> On 2014/07/04 20:47:24, janek wrote:
>> Hi Mark,
>> do you have any objections against putting this patch on countdown?
>
> No objections, but I'll leave it to someone else to give the green light
> since I'm not qualified to evaluate the C++ code.

ok, thanks!

[david.nalesnik, 2014-07-05 12:24:36]

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

https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm#newc...
scm/define-grobs.scm:87: (X-extent . ,ly:accidental-interface::width)
The old code is used as an example of ly:make-grob-closure in Extending
(http://www.lilypond.org/doc/v2.19/Documentation/extending/callback-functions). 
I don't think this needs to be fixed with this patch, but eventually a new
example should be found for the documentation.  (Or some note to the effect that
the example uses a previous setting.)

[Keith, 2014-07-05 18:26:10]

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

https://codereview.appspot.com/108270044/diff/40001/scm/define-grobs.scm#newc...
scm/define-grobs.scm:87: (X-extent . ,ly:accidental-interface::width)
I opened a tracker item
http://code.google.com/p/lilypond/issues/detail?id=3993
suggesting we simplify this bit of documentation

[janek, 2014-07-08 07:22:29]

Pushed as 

commit 59a842eba0f7ad78289a58a7dfa8fa786cdf11ed
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Wed Mar 27 17:20:03 2013 +0100

    Issue 2245: always align dynamics and lyrics on "main" notehead
    
    Until now, LyricTexts and DynamicTexts had their X-parents set to
    the first NoteHead in the NoteColumn.  This resulted in inconsistent
    alignment - placement of lyrics and dynamics depended on the order
    of notes in the input:
    
    % this was aligned differently
    { <f' g'>1\p <g' f'>\p }
    \addlyrics { la la }
    
    By using NoteColumns themselves as the X-parents, we make sure that
    the input order won't matter.  Since the NoteColumn contains all NoteHeads
    (including suspended ones, which usually should be ignored when aligning),
    as well as Flags and some other objects, we cannot use its X-extent directly
-
    instead, we add a function for calculating X-extent of the "main" part of
the
    NoteColumn, i.e. X-extent of the non-suspended NoteHeads (represented by the
    NoteHead furthest away from the stem).


Thanks for review!