Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-25 Thread davidgrant . no 
Patch description in the issue tracker has been updated.


https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition.cc
File lily/vowel-transition.cc (right):

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition.cc#newcode37
lily/vowel-transition.cc:37: SCM num_length = me->get_property
("minimum-length");
On 2020/03/24 21:46:30, hanwenn wrote:
> num suggests a number.
> 
> minimum_length ?

Done.

https://codereview.appspot.com/565750043/diff/569570043/lily/vowel-transition.cc#newcode137
lily/vowel-transition.cc:137: w += -d * r->item_drul_[d]->extent
(r->item_drul_[d], X_AXIS)[-d];
On 2020/03/24 21:46:29, hanwenn wrote:
> this still looks strange, but if it's problem, it'll be contained
within the
> vowel-transition code, which is acceptable.

Acknowledged, thanks.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-20 Thread davidgrant . no 
Revisions following review


https://codereview.appspot.com/565750043/diff/553710043/Documentation/notation/vocal.itely
File Documentation/notation/vocal.itely (right):

https://codereview.appspot.com/565750043/diff/553710043/Documentation/notation/vocal.itely#newcode894
Documentation/notation/vocal.itely:894: ah \vowelTransition _ _ _ _
On 2020/03/15 15:00:49, hanwenn wrote:
> is it vowel transtition or lyric transition?
> 
> make the grob name consistent (grob VowelTransition, or identifier
> \lyricTransition)

Done - all should now be VowelTransition.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc
File lily/spanner.cc (right):

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newcode368
lily/spanner.cc:368: w += -d * r->item_drul_[d]->extent
(r->item_drul_[d], X_AXIS)[-d];
On 2020/03/15 15:00:49, hanwenn wrote:
> this looks suspect. If you translate either items (relative to the
paper-column
> it is attached to), then this will leave the rod alone. Shouldn't the
extent be
> relative to the item' paper column?

This does seem currently to be working as I expect it to, i.e., for long
syllables we find the value to correct Rod::distance_, so that the
_drawn length_ of the vowel transition _doesn't change_ for wide
syllables. Although perhaps I have overlooked situations where this will
be incorrect. I tried the following:
{
  Paper_column *pc = r->item_drul_[d]->get_column ();
  w += -d * r->item_drul_[d]->extent (pc, X_AXIS)[-d];
}

This doesn't work correctly - see e.g. regtest
vowel-transition-offset-syllable.ly. The drawn length of the transitions
change depending on the width of the syllables. The length _shouldn't_
change, rather the width of the syllable should be corrected for.

So I've left this unchanged for now, but any pointers are much
appreciated if it still doesn't look right.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newcode393
lily/spanner.cc:393: SCM min_length_correction = me->get_property
("minimum-length-correction");
On 2020/03/15 15:00:50, hanwenn wrote:
> the behavior you add is specific to your new feature, so I think it
would be
> best to avoid changing spanner.cc (and at the same time, avoiding
wholesale
> copies of this spanner.cc code)
> 
I've put the vowel transition specific spacing code in back into
vowel-transition.cc, so spanner.cc is untouched. This does duplicate a
fair bit from spanner.cc - I don't know how to avoid this.

> Could you summarize for me what the behavior should be? Sorry for
being a little
> dense here.  (And how should they behave across line breaks?)
> 
Certainly: Vowel transitions should never be omitted due to tight
spacing, as their musical meaning would be lost. Space should instead be
added so that the transition can be drawn (at least) at minimum-length.
They extend to the end of the system if the transition continues on the
next system or ends on the first note on the next system. By default
they do not draw on the next system when the transition ends on the
first note on the system.

> It is strange to introduce a minimum-length-correction, when you could
introduce
> a callback for minimum-length that calculates a different value. 
>
The minimum-length-correction property has been removed.

https://codereview.appspot.com/565750043/diff/553710043/lily/spanner.cc#newcode414
lily/spanner.cc:414: r.distance_ -= bounds_protrusion (&r);
On 2020/03/15 15:00:50, hanwenn wrote:
> this is weird.  You're using r.item_drul_ here, but then in the next
line, you
> overwrite r.item_drul_. What's going on?  

I've rewritten this section - hopefully it looks more sensible.

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

https://codereview.appspot.com/565750043/diff/553710043/scm/define-grobs.scm#newcode1471
scm/define-grobs.scm:1471: (minimum-length-correction .
,ly:spanner::calc-padding-correction)
On 2020/03/15 15:00:50, hanwenn wrote:
> The function calc-xxx is usually used for calculating the xxx
property, so the
> naming is off.

Property has been removed.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-13 Thread davidgrant . no 


https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm
File scm/define-music-types.scm (right):

https://codereview.appspot.com/565750043/diff/557620047/scm/define-music-types.scm#newcode685
scm/define-music-types.scm:685: . ((description . "A transition between
lyric syllables.")
On 2020/03/12 17:38:06, lemzwerg wrote:
> Maybe s/transition/vowel transition/  ?

Done.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-12 Thread davidgrant . no 
On 2020/03/12 12:06:06, davidsg wrote:
> Rename to vowel-transition-event

Trying again with the name 'vowel transition' (sustained consonants are
mentioned explicitly in the docs). Perhaps this is an OK compromise?

Updated docs accordingly and added a couple of regtests, including one
documenting properties minimum-length-includes-bounds and
minimum-length-includes-padding.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-11 Thread davidgrant . no 
On 2020/03/11 02:30:04, Dan Eble wrote:
> How about calling it a gradual-vowel-change-event?  Come
> to think of it, is this only for vowels, or would it be appropriate to
use it
> for, say, sh -> ss?

Gould talks specifically about vowels, but I don't see any reason why it
shouldn't apply to sh->ss, or even from vowel to closed mouth. How about
gradual-syllable-change-event?
 
> It sounds (and the examples make
> it look) like something with a specific starting moment and duration,
more like
> a syllable of its own than like a hyphen, as I see it.
The arrow could start from an empty "" syllable, so not directly at a
(visible) syllable. If this is acceptable I'll add a regression test for
this situation.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-10 Thread davidgrant . no 
On 2020/03/11 00:32:26, davidsg wrote:
> Removed transition-engraver.cc

hyphen-engraver now also listens for transition-events, and makes
transitions or hyphens based on event class.

Also changed a couple of mentions of line to arrow in regtests.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-10 Thread davidgrant . no 
On 2020/03/10 09:59:29, davidsg wrote:
> Revision following review

Removed -> as special syntax, and uses instead the command
\vowelTransition.

Transitions are still separated from hyphens, which leaves
transition-engraver _almost_ a duplicate of hyphen-engraver. As
transitions are printed with line-spanner, the properties are quite
different to hyphens. Will it get messy if the properties are merged
into LyricHyphens, or doesn't this matter?

Uses ly:spanner::set-spacing-rods rather than
ly:lyric-transition::set-spacing-rods, so the Lyric_transition struct
has been removed completely.

Added (somewhat clumsily named - any suggestions?) properties to
spanner, for spacing required for transitions: minimum-length-add-bounds
and minimum-length-add-padding. If true, the extent of bounds protrusion
into the spacing rod, or padding, is in effect added to minimum-length
for spacing.

https://codereview.appspot.com/565750043/

Re: Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-08 Thread davidgrant . no 
On 2020/03/08 15:21:22, hanwenn wrote:
> I think it would be better to do this as something that doesn't
require special
> syntax at first, ie. some identifier. 
> 

One of my motivations for introducing '->' is that it keeps the lyrics
clean and easy to read. Music that uses vowel transitions can have a
_lot_ of them.

> Also, it looks like the transition-engraver is almost a literal copy
of the
> hyphen engraver. Wouldn't this be much simpler if you'd implement the
transition
> as a layout tweak to a hyphen?

I thought it might be best to separate transitions from hyphens, and use
the line-spanner-interface to draw the arrow. The requirements for
spacing are slightly different (transitions should never be omitted even
in tight spacing), and also some other properties differ (e.g.
minimum-space). So even though transitions are similar in many ways to
hyphens I wonder if they are distinct enough to be separated. But I will
certainly look into tweaking hyphens instead, as -- as you have pointed
out -- there is currently a lot of duplicated code.

Thank you both for the feedback! I'll have another go at this.

https://codereview.appspot.com/565750043/

Added transition lines for lyrics (issue 565750043 by davidgrant...@gmail.com)

2020-03-08 Thread davidgrant . no 
Reviewers: ,

Message:
I've been working on this feature I would like to propose for LilyPond,
and I wonder how I should proceed (request feedback on users group,
discussion here, or something else?). I'm aware of being very much a
beginner here, but I'm tentatively posting this hoping for feedback
either on the process or the code itself.

I do have some questions:
- Copyright notices at the top of new files: Should I always be adding
my own name (as I have done so far), even though the contents is based
on existing code? I'm assuming the fact that there _is_ a notice in the
files is the main goal, rather than the contents itself. Of course, I
don't want to be taking 'credit' for code written by others, but I also
don't want to be attributing anything to them that they wouldn't want to
touch with a ten foot pole.
- Everything is bundled together here - the feature itself, regtests and
docs. Should this be broken apart into separate commits?

Thanks!

Description:
Added transition lines for lyrics

A transition line (my suggested term) is drawn as an arrow from one
syllable to another, and indicates a gradual change of vowel (see Gould
pp. 452--453).

I propose '->' to be used between lyric syllables (similarly to hyphens
are input).

Please review this at https://codereview.appspot.com/565750043/

Affected files (+466, -59 lines):
  M Documentation/changes.tely
  M Documentation/music-glossary.tely
  M Documentation/notation/vocal.itely
  A input/regression/lyric-transition.ly
  A input/regression/lyric-transition-broken.ly
  A input/regression/lyric-transition-offset-syllable.ly
  A input/regression/lyric-transition-padding.ly
  A input/regression/lyric-transition-right-margin.ly
  A + lily/include/lyric-transition.hh
  M lily/lexer.ll
  A lily/lyric-transition.cc
  M lily/parser.yy
  A + lily/transition-engraver.cc
  M ly/engraver-init.ly
  M scm/define-event-classes.scm
  M scm/define-grobs.scm
  M scm/define-music-display-methods.scm
  M scm/define-music-types.scm
  M scm/safe-lily.scm

Doc: Corrected doc string for ly:dimension? (issue 547470049 by davidgrant...@gmail.com)

2020-01-25 Thread davidgrant . no 
Reviewers: ,

Message:
It isn't clear to me how a dimension is different from a normal number,
but the current doc string seems to be incorrect all the same.

Description:
Doc: Corrected doc string for ly:dimension?

ly:dimension? is a predicate - it does not return a number.

Please review this at https://codereview.appspot.com/547470049/

Affected files (+1, -1 lines):
  M lily/general-scheme.cc


Index: lily/general-scheme.cc
diff --git a/lily/general-scheme.cc b/lily/general-scheme.cc
index 
629c85ccaa7483d542d16d54e548279d2a401483..3cad1775d2754768aa3b39f7608f5b83f0d4316e
 100644
--- a/lily/general-scheme.cc
+++ b/lily/general-scheme.cc
@@ -260,7 +260,7 @@ LY_DEFINE (ly_unit, "ly:unit", 0, 0, 0, (),
 }
 
 LY_DEFINE (ly_dimension_p, "ly:dimension?", 1, 0, 0, (SCM d),
-   "Return @var{d} as a number.  Used to distinguish length"
+   "Is @var{d} a dimension?  Used to distinguish length"
" variables from normal numbers.")
 {
   return scm_number_p (d);

Doc: Added documentation for fill-line line-width (issue 583340043 by davidgrant...@gmail.com)

2020-01-12 Thread davidgrant . no 

Reviewers: ,

Message:
Doc: Added documentation for fill-line line-width

Added short descriptions and examples to Notation Reference.

Suggestions for additions to documentation following this thread on
lilypond-user:
https://lists.gnu.org/archive/html/lilypond-user/2020-01/msg00081.html

Description:
Doc: Added documentation for fill-line line-width

Added short descriptions and examples to Notation Reference.

Please review this at https://codereview.appspot.com/583340043/

Affected files (+23, -0 lines):
  M Documentation/notation/text.itely
  M scm/define-markup-commands.scm


Index: Documentation/notation/text.itely
diff --git a/Documentation/notation/text.itely  
b/Documentation/notation/text.itely
index  
ced0bca8ca2790dd034e493dc5fdafe20bdf414c..507d16aff3b369663960f035e69a6b21c0520013  
100644

--- a/Documentation/notation/text.itely
+++ b/Documentation/notation/text.itely
@@ -1064,6 +1064,24 @@ multi-line text or any other markup expression:
 }
 @end lilypond

+@cindex text, line width
+@cindex markup text, line width
+
+Elements may be spread to fill any specified width by overriding
+the @code{line-width} property.  By default it is set to
+@code{#f} which indicates the entire line:
+
+@lilypond[quote,verbatim]
+\markup {
+  \column {
+\fill-line { left center right }
+\null
+\override #'(line-width . 30)
+\fill-line { left center right }
+  }
+}
+@end lilypond
+
 @cindex wordwrapped text
 @cindex justified text
 @cindex text, justified
Index: scm/define-markup-commands.scm
diff --git a/scm/define-markup-commands.scm b/scm/define-markup-commands.scm
index  
9bd183789a3e13750571ccab2268f17b01d7210a..5463d3b4bba04836638eb4c2c5d3bd6a0db1484d  
100644

--- a/scm/define-markup-commands.scm
+++ b/scm/define-markup-commands.scm
@@ -1569,6 +1569,11 @@ If there are no arguments, return an empty stencil.
   }
   \\line { across the page }
 }
+\\null
+\\override #'(line-width . 50)
+\\fill-line {
+  Width explicitly specified
+}
   }
 }
 @end lilypond"