Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
Hi Chris, Thanks for working on these issues. I've applied your patch to master. Cheers, Neil http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
lgtm
http://codereview.appspot.com/150067/diff/3018/4006
File lily/extender-engraver.cc (right):
http://codereview.appspot.com/150067/diff/3018/4006#newcode118
lily/extender-engraver.cc:118: Moment *start_mom = column ?
unsmob_moment (column->get_property ("when") ) : 0;
no space between ) )
http://codereview.appspot.com/150067
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
The latest patch looks to me to be a proper fix for issue 800. http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
I've uploaded a patch that reverts the "fix" for issue 800, as it reintroduced the neverending-extender bug. I also added some comments in the code as requested. http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
On Tue, 2009-11-10 at 21:57 +, csny...@mvpsoft.com wrote: > On 2009/11/10 20:00:00, joeneeman wrote: > > http://codereview.appspot.com/150067/diff/2003/3005#newcode113 > > lily/extender-engraver.cc:113: if (!melisma_busy (voice) && > > !current_lyric_is_skip_ && lyric_acknowledged_) > > Could you please add a sentence or two explaining why this test is > here? > > > Also, please add a comment explaining (at a high level) why we need to > pass > > empty lyrics to the hyphen and extender engravers. > > I'll try to explain both of these together. Here are the cases I'm > addressing: I appreciate the explanation, but I'd really like to have a comment in the code. Basically, the problem was confusing enough that it required a few tries to get it right; therefore, it will be confusing to whomever looks at that bit of code in a few years. It would be nice to have a comment which gives the four cases you listed along with the desired behaviour for each case. It would be even better if the comment were self-contained, rather than referring to the history of the code. Thanks, Joe ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
On 2009/11/10 20:00:00, joeneeman wrote: http://codereview.appspot.com/150067/diff/2003/3005#newcode113 lily/extender-engraver.cc:113: if (!melisma_busy (voice) && !current_lyric_is_skip_ && lyric_acknowledged_) Could you please add a sentence or two explaining why this test is here? Also, please add a comment explaining (at a high level) why we need to pass empty lyrics to the hyphen and extender engravers. I'll try to explain both of these together. Here are the cases I'm addressing: 1) "La __ _ _ la la" (issue 786) Extender followed by manual melismata (worked before commit 7531ea6b3, broken after) 2) Same as above, but with an additional voice with a different rhythm (issue 800) (worked before 7531ea6b3, broken after) 3) "La -- _ _ la la" (worked both before and after 7531ea6b3, but needed to be addressed while fixing 786) 4) "La __" with music continuing after the end of the lyrics (broken in some cases before 7531ea6b3, fixed after) Pre-7531ea6b3, extenders were only finished at either (a) the next lyrics in the block or (b) the end of the associated voice. This led to problems when the voice continued without additional lyrics (a situation I encountered a couple of times when engraving vocal music with multiple verses and a single refrain, alternate texts, etc.). 7531ea6b3, my previous commit, added a check in Extender_engraver::stop_translation_timestep to check if the associated voice was in a melisma, and to completize the extender if not. This fixed case 4, but broke 1 and 2. The reason is, the extender-engraver had no way of knowing whether the lack of additional lyrics was due to reaching the end of the lyrics block or because there were manual melismata (underscores). By changing the lyrics-engraver to not kill LyricsText objects caused by manual melismata, the extender-engraver is now able to tell the difference between the two cases and react accordingly. The empty LyricsText objects introduced a problem with hyphens, however, since the hyphen's right bound would attach to the empty LyricsText object instead of the next non-empty LyricsText, necessitating some changes to the hyphen-engraver to accomodate this. Regarding issue 800 (case 2 above): I'm starting to second-guess my solution to that. In fact, I might have even completely un-done the fix from my earlier commit. I think the problem is that melisma_busy only works if the current timestep coincides with a note in that voice. It seems to me that melisma_busy should be modified to return true in this case. http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
http://codereview.appspot.com/150067/diff/2003/3005
File lily/extender-engraver.cc (right):
http://codereview.appspot.com/150067/diff/2003/3005#newcode83
lily/extender-engraver.cc:83: }
current_lyric_is_skip_ = ly_is_equal (test, scm_from_locale_string ("
"));
http://codereview.appspot.com/150067/diff/2003/3005#newcode113
lily/extender-engraver.cc:113: if (!melisma_busy (voice) &&
!current_lyric_is_skip_ && lyric_acknowledged_)
Could you please add a sentence or two explaining why this test is here?
http://codereview.appspot.com/150067/diff/2003/3006
File lily/hyphen-engraver.cc (right):
http://codereview.appspot.com/150067/diff/2003/3006#newcode63
lily/hyphen-engraver.cc:63:
current_lyric_is_skip_ = ly_is_equal (text, scm_from_locale_string ("
"))
http://codereview.appspot.com/150067/diff/2003/3007
File lily/lyric-engraver.cc (right):
http://codereview.appspot.com/150067/diff/2003/3007#newcode67
lily/lyric-engraver.cc:67: text_ = make_item ("LyricText",
event_->self_scm ());
Indentation.
Also, please add a comment explaining (at a high level) why we need to
pass empty lyrics to the hyphen and extender engravers.
http://codereview.appspot.com/150067
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
Third time's a charm, hopefully. Patch Set 3, which has just been uploaded, fixes both 786 and 800 without adversely affecting any regression tests. (I spoke too soon about potentially uncovering an underlying bug - all of the bugs were mine) http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
Oops - comment for the 2nd patch set should be "Fixed hyphens..." Not sure what to do about issue 800 - it might be a bug in some other portion of the code that my earlier patch uncovered. http://codereview.appspot.com/150067 ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
> I can't verify #800 here; the extender's still too short. You're right - I assumed 800 would be fixed because it looked like it was the same issue, but it seems unrelated. I've done some investigation, and am stumped so far. > Can you check lyric-melisma-manual.ly? > > I've just run a regression test check and there's a missing hyphen > following "Ky", though the melisma is correct (see attached image). I'm working on it; I fixed the hyphen issue, but now there are a bunch of other lyrics that are spaced wrong. I'll keep investigating. Chris Snyder Adoro Music Publishing 1-616-828-4436 x800 http://www.adoromusicpub.com ___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
2009/11/9 : > This patch fixes 786 (and 800, which I think should be labeled a dupe). I can't verify #800 here; the extender's still too short. > No regression tests were adversely affected, but there could possibly be > some issues with creating "empty" LyricText objects. Can you check lyric-melisma-manual.ly? I've just run a regression test check and there's a missing hyphen following "Ky", though the melisma is correct (see attached image). Regards, Neil <>___ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
Fixes issue 786, "Extenders in lyrics stop prematurely if a single underscore is found."
Reviewers: ,
Message:
This patch fixes 786 (and 800, which I think should be labeled a dupe).
No regression tests were adversely affected, but there could possibly be
some issues with creating "empty" LyricText objects.
Description:
Fixes issue 786, "Extenders in lyrics stop prematurely if a single
underscore is found."
This commit changes the lyrics engraver to create LyricText objects
even for "empty" lyrics (underscores). This change is necessary because
the old behavior
(pre-7531ea6b3 commit) relied on extenders continuing until the presence
of another lyric
syllable, which was not always the case. That commit changed the
behavior to completize
extenders when no more lyrics were present, which fixed the
neverending-extender bug but
introduced the extenders-stopping-prematurely bug.
By adding the "empty" LyricText objects, the extender engraver can now
tell the difference
between melismas and the end of a block of lyrics.
Please review this at http://codereview.appspot.com/150067
Affected files:
M lily/extender-engraver.cc
M lily/lyric-engraver.cc
Index: lily/extender-engraver.cc
diff --git a/lily/extender-engraver.cc b/lily/extender-engraver.cc
index
041f9046ed470e9ee45a6ecbdfc0ff7d7c22784f..467f16ff10977fcc30df5d217574a5a61ba215a6
100644
--- a/lily/extender-engraver.cc
+++ b/lily/extender-engraver.cc
@@ -28,6 +28,7 @@ class Extender_engraver : public Engraver
Stream_event *ev_;
Spanner *extender_;
Spanner *pending_extender_;
+ bool current_lyric_is_skip_;
public:
TRANSLATOR_DECLARATIONS (Extender_engraver);
@@ -44,6 +45,7 @@ protected:
Extender_engraver::Extender_engraver ()
{
+ current_lyric_is_skip_ = false;
extender_ = 0;
pending_extender_ = 0;
ev_ = 0;
@@ -70,7 +72,14 @@ Extender_engraver::acknowledge_lyric_syllable (Grob_info
i)
if (extender_)
extender_->set_bound (LEFT, item);
- if (pending_extender_)
+ SCM text = item->get_property ("text");
+ if (ly_is_equal (text, scm_from_locale_string (" "))) {
+current_lyric_is_skip_ = true;
+ } else {
+current_lyric_is_skip_ = false;
+ }
+
+ if (pending_extender_ && !current_lyric_is_skip_)
{
pending_extender_->set_object ("next", item->self_scm ());
completize_extender (pending_extender_);
@@ -98,7 +107,7 @@ Extender_engraver::stop_translation_timestep ()
{
Pointer_group_interface::add_grob (pending_extender_,
ly_symbol2scm ("heads"), h);
- if (!melisma_busy (voice))
+ if (!melisma_busy (voice) && !current_lyric_is_skip_)
{
completize_extender (pending_extender_);
pending_extender_ = 0;
Index: lily/lyric-engraver.cc
diff --git a/lily/lyric-engraver.cc b/lily/lyric-engraver.cc
index
74d15e4df1faedef65fddc857260b3fe33c46c5e..5a071535777cdac9b27302b6c3826551a4affe89
100644
--- a/lily/lyric-engraver.cc
+++ b/lily/lyric-engraver.cc
@@ -64,7 +64,6 @@ Lyric_engraver::process_music ()
if (last_text_)
last_text_->set_property ("self-alignment-X", scm_from_int (LEFT));
}
- else
text_ = make_item ("LyricText", event_->self_scm ());
}
}
___
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
