Fix 1120 in a way to avoid issues 1472, 1474

Fix note spacing with melismata
by receding the extra-spacing-height of LyricText and similar items.
Create property skyline-vertical-padding for the vertical padding of left- and right-skylines used in note spacing.

[Keith, 2011-01-21 04:22:58]

upload.py is now skipping over the base files in scm for me - the problem
Valentin had a month ago.  Looking into that.

Minor shifts, as expected, in the regression tests.

I said that I would look into LyricExtenders and Hyphens.  No need for any
adjustment there; they are not consulted in the note-spacing; we don't know at
that stage if extenders and hyphens will even be needed.

I checked that issues 1472 and 1474 are back at their 2.12.3 status, when they
did not bother us.  Surprisingly, issue 1229 is not returned to its 2.12.3
status.

Collisions of the type 
{ g''( c''-> geses'' a'') } 
are now much more rare, back to 2.12.3 status.

[Keith, 2011-01-21 05:18:25]

On 2011/01/21 04:22:58, Keith wrote:
> upload.py is now skipping over the base files in scm for me 
>   - the problem Valentin had a month ago. 
Solved. Whichever database Python uses for mime types now associates the
extention .scm with "application/vnd.lotus-screencam"
For now, I added that string to the white list in my copy of upload.py, so that
upload.py knows it is a text, not binary file, and uploads it.

[Graham Percival (old account), 2011-01-21 13:13:43]

Regtest comparison shows nothing obviously wrong, and it compiles the docs from
scratch.

[Keith, 2011-01-23 04:48:19]

Needs adjustments as marked, to handle all the other issues that commit ee0488
fixed : 
886 (=819) 1138 and 1137(already handled)
and some exactly analogous situations not yet reported.

New patch within a day, to come along with reg tests.

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode261
scm/define-grobs.scm:261: (stencil . ,ly:text-interface::print)
extra-spacing-height . (-0.5  . 0.5) for issue 1138

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode495
scm/define-grobs.scm:495: (stencil . ,ly:text-interface::print)
extra-spacing-height for issue 886 (and 819)

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode856
scm/define-grobs.scm:856: (stencil . ,fret-board::calc-stencil)
extra-spacing-height to avoid analog of issue 886

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode967
scm/define-grobs.scm:967: (stencil . ,system-start-text::print)
extra-spacing-height analogous to Lyrics?

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1183: (stencil . ,ly:measure-grouping::print)
extra-spacing-height analogous to Lyrics?

[Keith, 2011-01-24 05:15:07]

Extended to cover the other issues that were fixed along with 1120.  The
regression test that /could/ have caught the breakage of issue 1120 is revised
so it will (more likely) catch any future breakage.

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode261
scm/define-grobs.scm:261: (stencil . ,ly:text-interface::print)
On 2011/01/23 04:48:19, Keith wrote:
> extra-spacing-height . (-0.5  . 0.5) for issue 1138
.. is not required for the regtest that raised issue 1138
(figured-bass-extenders-markup) Also, if the line above is added to FiguredBass,
it spaces complicated basso continuo too tightly.  Figured Bass is more similar
to NoteHeads than it is to Lyrics.

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode586
scm/define-grobs.scm:586: (extra-spacing-height . (-0.5 . 0.5))
CueClef and CueEndClef were added after ee00488 so a simple revert missed these.
 They should match Clef.

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode967
scm/define-grobs.scm:967: (stencil . ,system-start-text::print)
InstrumentName goes left of the staff.  No analogy with the melismata issue 1120

http://codereview.appspot.com/4095041/diff/32001/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1183: (stencil . ,ly:measure-grouping::print)
On 2011/01/23 04:48:19, Keith wrote:
> extra-spacing-height analogous to Lyrics?
MeasureGrouping does not work analogously to Lyrics.  No need to extend a fix
for 1120 here.

http://codereview.appspot.com/4095041/diff/42001/input/regression/lyrics-meli...
File input/regression/lyrics-melisma-beam.ly (right):

http://codereview.appspot.com/4095041/diff/42001/input/regression/lyrics-meli...
input/regression/lyrics-melisma-beam.ly:17: g4 d8[ b8 d8 g8]  g4
Moved some note heads so their stems interfere with lyrics, so that these notes
will move should something like issue 1120 recur.

http://codereview.appspot.com/4095041/diff/42001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4095041/diff/42001/scm/define-grobs.scm#newcode178
scm/define-grobs.scm:178: (BalloonTextItem
Similar to Lyrics in its spacing needs

[Carl, 2011-01-25 04:47:23]

This patch also resolves the problem in issue 1229 of notes extending beyond the
right-hand bar line.

Adding additional extra-spacing-height to the TimeSignature grob resolves the
1229 issue of overlapping the time signature.

This patch seems to have some very good benefits.

[Graham Percival (old account), 2011-01-26 18:18:21]

On 2011/01/25 04:47:23, Carl wrote:
> This patch seems to have some very good benefits.

Neil has identified a potential downside to this patch.

Some kind of additional work is required -- maybe adding some special case to
avoid changing clefs in this way, or possibly just an argument that the change
is good.

[Keith, 2011-01-29 21:34:13]

On 2011/01/26 18:18:21, Graham Percival wrote:
> Neil has identified a potential downside to this patch.
> 
That was a restoration of 2.12.3 behavior, which I had mentioned in the original
email thread but didn't explicitly handle until now.

Status of this patch is 
1)revert ee0488, which was a global change in effective extra-spacing-height,
/except/ keep from ee0488 the addition of beat-slash on the print-callbacks list

2)reviewed the entire list of Layout Objects to specify extra-spacing-height to
restore any possible positive effect of ee0488

3)regression test and see only expected changes

[Carl, 2011-01-29 23:00:09]

This patch solves Neil's problem with clef spacing.

LGTM.

Carl

[Keith, 2011-01-30 19:29:04]

On 2011/01/29 21:34:13, Keith wrote:
> Status of this patch is 
[...]
> 3)regression test and see only expected changes

There are a lot of changes.  When I say 'expected' I mean after having studied
the changes made by the original ee0488.  The comparison made soon after that
commit is available at
http://lilypond.org/test/v2.13.25-1/compare-v2.13.24-1/index.html

I hope this patch picks the more-desirable version in cases where there is a
noticeable difference in the comparison linked above.

[hanwenn, 2011-01-31 11:20:39]

2 issues:

* this hardcodes 0.1 in several places. Make a property, so we can override this

* rewrite the commit description so it explains what you are doing (ie. what
issues 1120, 1472, 1474 are).  The desc. should be understandable without access
to the bug database.

[Keith, 2011-01-31 19:36:16]

On 2011/01/31 11:20:39, hanwenn wrote:
> * this hardcodes 0.1 in several places. Make a property, 
> so we can override this

Agreed in principle; the relevant property extra-spacing-height should absorb
these magic numbers, but I am not willing to do so in this patch.

The 0.1s were in the previous stable release; commit ee0488 tried to remove
them, but had side effects.  This patch reverts ee0488 (thus bringing back the
ugly 0.1s) then adjusts a few existing properties to have the desired effects of
ee0488.

I began looking into absorbing the 0.1s into the extra-spacing-height of all the
objects who need it, thus completing the job of ee0488. I concluded that
negative extra-spacing-height for Lyrics was necessary for that task, so
proposed this patch first.

> * rewrite the commit description so it explains what you are doing > (ie. what
issues 1120, 1472, 1474 are). 

I gave words to the issue numbers on Rietveld; the git commit text will be a
shortened version in that style.

[Carl, 2011-01-31 19:50:38]

On 2011/01/31 19:36:16, Keith wrote:
> On 2011/01/31 11:20:39, hanwenn wrote:
> > * this hardcodes 0.1 in several places. Make a property, 
> > so we can override this
> 
> Agreed in principle; the relevant property extra-spacing-height should absorb
> these magic numbers, but I am not willing to do so in this patch.
> 
> The 0.1s were in the previous stable release; commit ee0488 tried to remove
> them, but had side effects.  This patch reverts ee0488 (thus bringing back the
> ugly 0.1s) then adjusts a few existing properties to have the desired effects
of
> ee0488.
> 

Since the 0.1s are all part of a Separation_item, what if we defined a new
grob-property default-separation, and set the value to 0.1 for
NonMusicalPaperColumn, NoteColumn, and PaperColumn, since these are the grobs
having separation_item_interface?

We could then get the value of default-separation from the paper column, and use
that value throughout separation-item.cc

I agree that it seems like a bit of a pain to do this when you're just wanting
to revert a patch that now appears inappropriate.  But if this property were
added, no recompilation would be required in order to resolve the issues you've
been fighting with here.

[Keith, 2011-01-31 20:20:47]

On 2011/01/31 19:50:38, Carl wrote:
> 
> We could then get the value of default-separation from the paper column, and
use
> that value throughout separation-item.cc
> 
I had discounted the new property idea because of all the extra property
lookups, but it seems your idea costs only one lookup per Column.  
For a name, default-clearance might better indicate that this is a vertical
clearance maintained while choosing horizontal separation between columns.

> I agree that it seems like a bit of a pain to do [...]
Pain only makes us stronger.  However, I decided decades ago that I should avoid
learning programming languages where one can redefine the + operator.  I did not
manage to escape them entirely, but might get stuck trying to pass a new
property through all the layers.  Let's see.

[Neil Puttock, 2011-01-31 23:28:06]

On 31 January 2011 20:20,  <k-ohara5a5a@oco.net> wrote:

> I had discounted the new property idea because of all the extra property
> lookups, but it seems your idea costs only one lookup per Column.
> For a name, default-clearance might better indicate that this is a
> vertical clearance maintained while choosing horizontal separation
> between columns.

Since we already have skyline-horizontal-padding, why not call it
skyline-vertical-padding?

Cheers,
Neil

[Keith, 2011-02-01 09:33:22]

On 2011/01/31 19:50:38, Carl wrote:
> Since the 0.1s are all part of a Separation_item, what if we 
> defined a new grob-property default-separation, and set the value
> to 0.1 for NonMusicalPaperColumn, NoteColumn, and PaperColumn, 
> since these are the grobs having separation_item_interface?
> 
Done; and I like it.

Only NoteColumn needed the padding to restore 2.12.3s good note spacing.
I gave a nonzero padding to NonMusicalPaperColumn because the grobs in such
columns all seem to want the same padding.

[Keith, 2011-02-01 09:33:48]

http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1309: (skyline-vertical-padding . 0.15)
The starting value 0.15 matches version 2.12.3 behavior.
The old 0.1s applied once as extra-spacing-height, and once as a vertical
horizon_padding which has double effect due to the shape of this padding.  The
new 0.15 is a straightforward horizon padding.

[Graham Percival (old account), 2011-02-01 21:49:02]

LGTM

[hanwenn, 2011-02-02 01:23:10]

LGTM

On Tue, Feb 1, 2011 at 7:49 PM,  <percival.music.ca@gmail.com> wrote:
> LGTM
>
> http://codereview.appspot.com/4095041/
>



-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[Neil Puttock, 2011-02-02 23:14:39]

LGTM.

http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc
File lily/separation-item.cc (right):

http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newc...
lily/separation-item.cc:83: double horizon_padding = robust_scm2double
(me->get_property ("skyline-vertical-padding"), 0.0);
Real horizon_padding

http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newc...
lily/separation-item.cc:94: double horizon_padding = robust_scm2double
(me->get_property ("skyline-vertical-padding"), 0.0);
Real horizon_padding

http://codereview.appspot.com/4095041/diff/52002/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4095041/diff/52002/scm/define-grob-properties.s...
scm/define-grob-properties.scm:727: Y-extents and extra-spacing-heights of the
constituent grobs in the
@code{Y-extent}s and @code{extra-spacing-height}s

http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1081: (extra-spacing-height . (+inf.0 . -inf.0))
fix indent (tab)

http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1161: ; Recede in height for purposes of note spacing,
;;

http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1162: ; so notes in melismata can be freely spaced above
lyrics
;;

[Keith, 2011-02-03 07:29:15]

On 2011/02/02 23:14:39, Neil Puttock wrote:
> LGTM.
> 
> http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc
> File lily/separation-item.cc (right):
> 
>
http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newc...
> lily/separation-item.cc:83: double horizon_padding = robust_scm2double
> (me->get_property ("skyline-vertical-padding"), 0.0);
> Real horizon_padding
> [...]
Thanks.  All done.

[Graham Percival (old account), 2011-02-03 18:24:33]

Latest version LGTM.

Keith, could you send me the git-format patch privately, so that I can push it
in 24 hours if nobody complains?