Align metronome mark at time signature or first musical element. Fixes #684.

Align metronome mark at time signature or first musical element.  Fixes #684.

Only if no TimeSignature is present, align on musical column.

Break-alignable metronome marks.

[Neil Puttock, 2010-06-07 16:59:14]

Hi Jan,

Have you checked what happens with full-bar rests?

I haven't tested your patch, but it's similar to the one I posted, which suffers
from invisible tempo marks at full-bar rests.

Cheers,
Neil

http://codereview.appspot.com/1579041/diff/2001/3001
File lily/metronome-engraver.cc (right):

http://codereview.appspot.com/1579041/diff/2001/3001#newcode78
lily/metronome-engraver.cc:78: == text_->get_property_data
("break-align-symbol")))
should be break-align-symbols (which complicates it a bit)

[janneke-list_xs4all.nl, 2010-06-07 21:54:05]

Op maandag 07-06-2010 om 16:59 uur [tijdzone +0000], schreef
n.puttock@gmail.com:

Hi Neil,

> Have you checked what happens with full-bar rests?

Uhuh, I have now ;-)

> I haven't tested your patch, but it's similar to the one I posted, which
> suffers from invisible tempo marks at full-bar rests.

Good catch, that should also be fixed now.  Thanks!

Greetings,
Jan.

> Cheers,
> Neil
> 
> 
> http://codereview.appspot.com/1579041/diff/2001/3001
> File lily/metronome-engraver.cc (right):
> 
> http://codereview.appspot.com/1579041/diff/2001/3001#newcode78
> lily/metronome-engraver.cc:78: == text_->get_property_data
> ("break-align-symbol")))
> should be break-align-symbols (which complicates it a bit)
> 
> http://codereview.appspot.com/1579041/show
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Neil Puttock, 2010-06-08 22:22:43]

Hi Jan,

I've tested the latest patch, and it looks pretty good so far.

Here are a few thoughts on positioning:

-) A metronome mark at a full-bar rest should be aligned with the barline
instead of the paper column to the left of the rest.

-) If there's a tempo change at a key signature, the metronome mark shouldn't be
aligned with the following note.

Cheers,
Neil

http://codereview.appspot.com/1579041/diff/8001/9001
File lily/metronome-engraver.cc (right):

http://codereview.appspot.com/1579041/diff/8001/9001#newcode88
lily/metronome-engraver.cc:88: Metronome_mark_engraver::acknowledge_grob
(Grob_info info)
Since this always gets called before acknowledge_break_aligned (), you could
fold the code above into this with a check for break-aligned-interface.

http://codereview.appspot.com/1579041/diff/8001/9001#newcode98
lily/metronome-engraver.cc:98: text_->set_parent (g, X_AXIS);
I find this really weird: do you know why this is necessary for the tempo mark
to appear on the full-bar rest even though stop_translation_timestep () resets
the X-parent to currentMusicalColumn?

http://codereview.appspot.com/1579041/diff/8001/9002
File scm/define-grobs.scm (right):

http://codereview.appspot.com/1579041/diff/8001/9002#newcode1150
scm/define-grobs.scm:1150: (self-alignment-X . -1)
(self-alignment-X . ,LEFT)

http://codereview.appspot.com/1579041/diff/8001/9002#newcode1157
scm/define-grobs.scm:1157: break-alignable-interface
+ self-alignment-interface

http://codereview.appspot.com/1579041/diff/8001/9002#newcode1162
scm/define-grobs.scm:1162: (break-align-symbol . multi-measure-rest)
This doesn't really work, since a MultiMeasureRest isn't a break-aligned grob
(same for MetronomeMark).

The tempo mark regtests spit out loads of warnings due to the missing interface
for this property, but you can't add break-aligned-interface to silence them
since the rest then gets picked up in the break-alignment-interface, causing a
segfault.

[janneke, 2010-06-16 12:08:06]

On 2010/06/08 22:22:43, Neil Puttock wrote:

Comments processed in patch 4 & 5.

> Here are a few thoughts on positioning:
> 
> -) A metronome mark at a full-bar rest should be aligned with the barline
> instead of the paper column to the left of the rest.

This is not in Read or #684's description...
 
> -) If there's a tempo change at a key signature, the metronome mark shouldn't
be
> aligned with the following note.

and neither is this.  I added both to the latest patch, although
this may be incorrect and was unexpected work for 684...

Sorry for the terse answer, I tried replying twice only to find
the replies never made it.  I'm probably not trusting web interfaces
enough ;-)

Jan.

[Neil Puttock, 2010-06-16 22:37:14]

Hi Jan,

On 2010/06/16 12:08:06, jan.nieuwenhuizen wrote:
> On 2010/06/08 22:22:43, Neil Puttock wrote:

> > -) A metronome mark at a full-bar rest should be aligned with the barline
> > instead of the paper column to the left of the rest.
> 
> This is not in Read or #684's description...

See http://code.google.com/p/lilypond/issues/detail?id=712

> > -) If there's a tempo change at a key signature, the metronome mark
shouldn't
> be
> > aligned with the following note.
> 
> and neither is this.  I added both to the latest patch, although
> this may be incorrect and was unexpected work for 684...

The key signature counts as the "first notational element" in such cases.

Cheers,
Neil

[Neil Puttock, 2010-06-20 21:57:20]

Hi Jan,

Here are some more comments for you.

Cheers,
Neil

http://codereview.appspot.com/1579041/diff/19001/20003
File lily/metronome-engraver.cc (right):

http://codereview.appspot.com/1579041/diff/19001/20003#newcode81
lily/metronome-engraver.cc:81: && g->get_property_data ("break-align-symbol")
get_property ()

http://codereview.appspot.com/1579041/diff/19001/20003#newcode82
lily/metronome-engraver.cc:82: == ly_symbol2scm ("staff-bar"))
can't this be incorporated into 'break-align-symbols for MetronomeMark?

http://codereview.appspot.com/1579041/diff/19001/20003#newcode86
lily/metronome-engraver.cc:86: && scm_member (g->get_property_data
("break-align-symbol"),
get_property ()

http://codereview.appspot.com/1579041/diff/19001/20003#newcode87
lily/metronome-engraver.cc:87: text_->get_property_data ("break-align-symbols"))
get_property ()

http://codereview.appspot.com/1579041/diff/19001/20003#newcode96
lily/metronome-engraver.cc:96: grob_name_scm (Grob *g)
ly_symbol2scm (g->name ().c_str ());

though I'd prefer more lisp-like syntax for this using
camel_case_to_lisp_identifier ()

http://codereview.appspot.com/1579041/diff/19001/20003#newcode109
lily/metronome-engraver.cc:109: text_->get_property_data
("non-break-align-symbols"))
get_property ()

http://codereview.appspot.com/1579041/diff/19001/20006
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/1579041/diff/19001/20006#newcode610
scm/define-grob-properties.scm:610: (non-break-align-symbols ,list? "A list of
symbols that determine
needs adding to an interface

http://codereview.appspot.com/1579041/diff/19001/20007
File scm/define-grobs.scm (right):

http://codereview.appspot.com/1579041/diff/19001/20007#newcode418
scm/define-grobs.scm:418: metronome-mark
Is this necessary?

IIUC, only break-aligned grobs will be acknowledged by the Break_align_engraver,
so a MetronomeMark will never appear in the list of elements for ordering.

http://codereview.appspot.com/1579041/diff/19001/20007#newcode1161
scm/define-grobs.scm:1161: break-alignable-interface
+ self-alignment-interface

otherwise regtests spit out loads of warnings for missing interface (from
self-alignment-X setting)

[janneke, 2010-06-22 18:51:57]

On 2010/06/20 21:57:20, Neil Puttock wrote:

AARGH.  Again this web interface eats my email.

Is anyone using this, it is so frustrating.  I'm going to
try using plain email response now.

[janneke, 2010-06-22 19:01:58]

Hi Neil,

Not sure how to find the email address to bind this
to http://codereview.appspot.com/1579041 ?

> http://codereview.appspot.com/1579041/diff/19001/20003#newcode82
> lily/metronome-engraver.cc:82: == ly_symbol2scm ("staff-bar"))
> can't this be incorporated into 'break-align-symbols for MetronomeMark?

Alas, it can't.  When adding staff-bar to that list, the broken
marks appear next to the bar numbers, right at the start of the
staves.

> though I'd prefer more lisp-like syntax for this using
> camel_case_to_lisp_identifier ()

Done.

> http://codereview.appspot.com/1579041/diff/19001/20006#newcode610
> scm/define-grob-properties.scm:610: (non-break-align-symbols ,list? "A list of
> symbols that determine
> needs adding to an interface

Added to the break-aligned-interface.

> http://codereview.appspot.com/1579041/diff/19001/20007
> File scm/define-grobs.scm (right):
> 
> http://codereview.appspot.com/1579041/diff/19001/20007#newcode418
> scm/define-grobs.scm:418: metronome-mark
> Is this necessary?
> 
> IIUC, only break-aligned grobs will be acknowledged by the
Break_align_engraver,
> so a MetronomeMark will never appear in the list of elements for ordering.

Okay...so I've added the break-aligned-interface.  It makes sense
to have this position added - this is probably not the only symbol
that we want positioned like this?

> http://codereview.appspot.com/1579041/diff/19001/20007#newcode1161
> scm/define-grobs.scm:1161: break-alignable-interface
> + self-alignment-interface
> 
> otherwise regtests spit out loads of warnings for missing interface (from
> self-alignment-X setting)

Added.

[c_sorensen, 2010-06-22 19:04:14]

On 6/22/10 12:51 PM, "jan.nieuwenhuizen@gmail.com"
<jan.nieuwenhuizen@gmail.com> wrote:

> On 2010/06/20 21:57:20, Neil Puttock wrote:
> 
> AARGH.  Again this web interface eats my email.

It came fine to me, just 10 minutes later.

Carl

[c_sorensen, 2010-06-22 19:05:29]

On 6/22/10 12:51 PM, "jan.nieuwenhuizen@gmail.com"
<jan.nieuwenhuizen@gmail.com> wrote:

> On 2010/06/20 21:57:20, Neil Puttock wrote:
> 
> AARGH.  Again this web interface eats my email.

It came fine to me, just 10 minutes later.

Carl

[janneke, 2010-07-01 08:08:36]

So, how are we doing here?  Ready to commit & close

http://code.google.com/p/lilypond/issues/detail?id=684

[Neil Puttock, 2010-07-01 21:34:01]

Hi Jan,

On 2010/07/01 08:08:36, jan.nieuwenhuizen wrote:
> So, how are we doing here?  Ready to commit & close

I'm testing the latest set at the moment; will report back with more comments.

Cheers,
Neil

[Neil Puttock, 2010-07-04 19:54:54]

Hi Jan,

On 2010/06/22 19:01:58, jan.nieuwenhuizen wrote:

> Alas, it can't.  When adding staff-bar to that list, the broken
> marks appear next to the bar numbers, right at the start of the
> staves.

That's a shame.  It would be great if the symbol order in 'break-align-symbols
determined the priority (like it does for RehearsalMark/BarNumber).
 

> > http://codereview.appspot.com/1579041/diff/19001/20007#newcode418
> > scm/define-grobs.scm:418: metronome-mark
> > Is this necessary?
> > 
> > IIUC, only break-aligned grobs will be acknowledged by the
> Break_align_engraver,
> > so a MetronomeMark will never appear in the list of elements for ordering.
> 
> Okay...so I've added the break-aligned-interface.  It makes sense
> to have this position added - this is probably not the only symbol
> that we want positioned like this?

I'm still confused as to why it's necessary, since it's only useful for ordering
break-aligned grobs inside the stave; ditto for the defaults for
'break-align-symbol and 'break-align-symbols.

[janneke-list_xs4all.nl, 2010-07-04 20:36:28]

Op zondag 04-07-2010 om 19:54 uur [tijdzone +0000], schreef
n.puttock@gmail.com:

> > Alas, it can't.  When adding staff-bar to that list, the broken
> > marks appear next to the bar numbers, right at the start of the
> > staves.
> 
> That's a shame.  It would be great if the symbol order in
> 'break-align-symbols determined the priority (like it does for
> RehearsalMark/BarNumber).

It does.  The symbol order does determine the priority.  Problem is,
at start of a stave the *only* symbol that is present is the staff
bar.

> > Okay...so I've added the break-aligned-interface.  It makes sense
> > to have this position added - this is probably not the only symbol
> > that we want positioned like this?
> 
> I'm still confused as to why it's necessary, since it's only useful for
> ordering break-aligned grobs inside the stave; ditto for the defaults
> for 'break-align-symbol and 'break-align-symbols.

It is not necessary, we could scrap both of these.  I figure however,
that when adding break-aligned-ness to other engravers (text-spanner
etc), it could would be nice if they could make use of mm-rest's
alignment position.  Possibly it's better to remove both and add
when needed -- I don't know.


-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[Neil Puttock, 2010-07-04 21:14:52]

On 2010/07/04 20:36:28, janneke-list_xs4all.nl wrote:

> It does.  The symbol order does determine the priority.

Hmm, this doesn't appear to be the case, since key signatures are preferred over
time signatures for aligning.

> Problem is,
> at start of a stave the *only* symbol that is present is the staff
> bar.

Ah, good point. :)  It would prevent aligning on notes.


> > > Okay...so I've added the break-aligned-interface.  It makes sense
> > > to have this position added - this is probably not the only symbol
> > > that we want positioned like this?
> > 
> > I'm still confused as to why it's necessary, since it's only useful for
> > ordering break-aligned grobs inside the stave; ditto for the defaults
> > for 'break-align-symbol and 'break-align-symbols.
> 
> It is not necessary, we could scrap both of these.  I figure however,
> that when adding break-aligned-ness to other engravers (text-spanner
> etc), it could would be nice if they could make use of mm-rest's
> alignment position.  Possibly it's better to remove both and add
> when needed -- I don't know.

I'd rather remove them, since it's likely to cause confusion among users.

Below, I've suggested adding 'non-musical, since it improves spacing for
full-bar rests: there's currently a bit of extra space added between the
non-musical and musical paper columns.

[Neil Puttock, 2010-07-04 21:15:10]

http://codereview.appspot.com/1579041/diff/30001/31004
File lily/metronome-engraver.cc (right):

http://codereview.appspot.com/1579041/diff/30001/31004#newcode82
lily/metronome-engraver.cc:82: && g->get_property_data ("break-align-symbol")
text_->get_property ("break-align-symbol")

OK, so it's unlikely, but a user might use a callback to set
'break-align-symbols instead of a simple list.

http://codereview.appspot.com/1579041/diff/30001/31004#newcode88
lily/metronome-engraver.cc:88: text_->get_property_data ("break-align-symbols"))
get_property ()

http://codereview.appspot.com/1579041/diff/30001/31004#newcode112
lily/metronome-engraver.cc:112: text_->get_property_data
("non-break-align-symbols"))
get_property ()

http://codereview.appspot.com/1579041/diff/30001/31008
File scm/define-grobs.scm (right):

http://codereview.appspot.com/1579041/diff/30001/31008#newcode1159
scm/define-grobs.scm:1159: (non-break-align-symbols . (multi-measure-rest))
+ (non-musical . #t)

[reinhold_kainhofer.com, 2010-07-27 13:42:01]

Am Sonntag, 20. Juni 2010, 23:57:20 schrieb n.puttock@gmail.com:
> Here are some more comments for you.

What happened to this patch? AFAICS, it has not been pushed to master, right?

I'm just afraid that it might be forgotten, which would be very bad, since I 
need this in LilyPond, too...

Cheers,
Reinhold
-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org

[janneke-list_xs4all.nl, 2010-07-27 14:14:12]

Op dinsdag 27-07-2010 om 15:45 uur [tijdzone +0200], schreef Reinhold
Kainhofer:

> What happened to this patch? AFAICS, it has not been pushed to master, right?

I'm "waiting" for an ack.

Greetings,
Jan.

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[Neil Puttock, 2010-08-10 21:36:28]

On 2010/07/27 14:14:12, janneke-list_xs4all.nl wrote:

> I'm "waiting" for an ack.

From whom?

[janneke, 2010-08-24 13:26:14]

> From whom?

Obviously, I missed your last set of comments.

I removed metronome-mark from the break-aligned lists and
removed the break-aligned-interface.

The self-alignment-interface was already added in a previous
version of the patch, I think.

> OK, so it's unlikely, but a user might use a callback to set
> 'break-align-symbols instead of a simple list.

> lily/metronome-engraver.cc:88: text_->get_property_data
("break-align-symbols"))
> get_property ()

Ah, I see.  All changed.  

I'm curious about other get_property_data () usages
though, in beam, bar-number-engraver, etc?

I also added list_p checks.

> + (non-musical . #t)

Added.

[Neil Puttock, 2010-08-25 20:54:24]

On 2010/08/24 13:26:14, jan.nieuwenhuizen wrote:

> Obviously, I missed your last set of comments.
> 
> I removed metronome-mark from the break-aligned lists and
> removed the break-aligned-interface.
> 
> The self-alignment-interface was already added in a previous
> version of the patch, I think.
> 
> > OK, so it's unlikely, but a user might use a callback to set
> > 'break-align-symbols instead of a simple list.
> 
> > lily/metronome-engraver.cc:88: text_->get_property_data
> ("break-align-symbols"))
> > get_property ()
> 
> Ah, I see.  All changed.

Cheers, looks fine.  I'm testing at the moment; will report back shortly.
 
> I'm curious about other get_property_data () usages
> though, in beam, bar-number-engraver, etc?

I think in the case of beam-engraver.cc it's used to check whether there's been
an explicit setting for staff-position (rather than relying on a callback): if
so, it comes from a manually positioned rest (e.g., c4\rest) so doesn't need to
be chained for beam avoidance.

[Neil Puttock, 2010-08-26 22:49:10]

Hi Jan,

I've tested the latest patch thoroughly, and it seems fine for the most part.

The only niggle I've come across is with full-bar rests at the start of a
system:

\relative c' {
  c1 \break
  \tempo 4 = 60
  R1
}

\paper { ragged-right = ##t }

Ideally, the tempo mark would be positioned after the clef in this case, but I
can't see how we can automate this without breaking the alignment in other
situations.

Cheers,
Neil

[janneke, 2010-08-27 09:50:47]

Thanks.  Applied and set to Fixed.

[hanwenn, 2010-08-29 18:04:00]

http://codereview.appspot.com/1579041/diff/42001/43004
File lily/metronome-engraver.cc (right):

http://codereview.appspot.com/1579041/diff/42001/43004#newcode109
lily/metronome-engraver.cc:109: }
this is out of style with the rest of lilypond code base.

The normal pattern is to use XXx_interface::has_interface (for hard-coded
interfaces), or grob->[internal_]has_interface(), to check for softcoded
interfaces.

The reason for this is that Grob names are only used as keys at creation time,
and for providing context in debugging (grep for ->name() ).  

The rest of the code uses interface names rather grob name, so we can easily
swap in and out implementations of formatting behavior. 

Sorry for coming with this after the fact, but could you fix the implementation
to be in accordance with this?  This engraver should call grob->has_interface()
for each of the symbols from non-break-align-symbols.

thanks

[janneke-list_xs4all.nl, 2010-08-29 18:54:25]

Op zondag 29-08-2010 om 18:04 uur [tijdzone +0000], schreef
hanwenn@gmail.com:

> http://codereview.appspot.com/1579041/diff/42001/43004#newcode109
> lily/metronome-engraver.cc:109: }
> this is out of style with the rest of lilypond code base.
> 
> The normal pattern is to use XXx_interface::has_interface (for
> hard-coded interfaces), or grob->[internal_]has_interface(), to check
> for softcoded interfaces.

Thanks for catching this.  I've applied the patch below.

Jan.

>From 19b37df119ff6ca84421d984fe1d33112ad08299 Mon Sep 17 00:00:00 2001
From: Jan Nieuwenhuizen <janneke@gnu.org>
Date: Sun, 29 Aug 2010 20:51:49 +0200
Subject: [PATCH] Metronome mark: check for interface rather than grob name in
non-break-aligned.

This remove the usage of grob_name so so we can easily swap in and out
implementations of formatting behavior.
---
 lily/metronome-engraver.cc     |   19 ++++++-------------
 scm/define-grob-properties.scm |    2 +-
 scm/define-grobs.scm           |    2 +-
 3 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/lily/metronome-engraver.cc b/lily/metronome-engraver.cc
index 7eaf314..aed0275 100644
--- a/lily/metronome-engraver.cc
+++ b/lily/metronome-engraver.cc
@@ -99,24 +99,17 @@ Metronome_mark_engraver::acknowledge_break_aligned
(Grob_info info)
     }
 }
 
-SCM
-grob_name_scm (Grob *g)
-{
-  SCM name_pair = scm_assq (ly_symbol2scm ("name"), g->get_property ("meta"));
-  return (scm_is_pair (name_pair)
-	  ? ly_camel_case_2_lisp_identifier (scm_cdr (name_pair))
-	  : SCM_EOL);
-}
-
 void
 Metronome_mark_engraver::acknowledge_grob (Grob_info info)
 {
   Grob *g = info.grob ();
 
-  if (text_
-      && safe_is_member (grob_name_scm (g),
-			 text_->get_property ("non-break-align-symbols")))
-    text_->set_parent (g, X_AXIS);
+  if (text_)
+    for (SCM s = text_->get_property ("non-break-align-symbols");
+	 scm_is_pair (s);
+	 s = scm_cdr (s))
+      if (g->internal_has_interface (scm_car (s)))
+	text_->set_parent (g, X_AXIS);
 }
 
 void
diff --git a/scm/define-grob-properties.scm b/scm/define-grob-properties.scm
index d4ee62b..4202b26 100644
--- a/scm/define-grob-properties.scm
+++ b/scm/define-grob-properties.scm
@@ -608,7 +608,7 @@ staff is crucial for @var{padding}).
 @code{VerticalAlignment}; rather, place it using its own
 @code{Y-offset} callback.")
      (non-break-align-symbols ,list? "A list of symbols that determine
-which NON-break-aligned grobs to align this to.")
+which NON-break-aligned interfaces to align this to.")
      (no-ledgers ,boolean? "If set, don't draw ledger lines on this
 object.")
      (no-stem-extend ,boolean? "If set, notes with ledger lines do not
diff --git a/scm/define-grobs.scm b/scm/define-grobs.scm
index 38bd6eb..71a167d 100644
--- a/scm/define-grobs.scm
+++ b/scm/define-grobs.scm
@@ -1171,7 +1171,7 @@
 			  (list ly:self-alignment-interface::x-aligned-on-self)))))
 	(self-alignment-X . ,LEFT)
 	(break-align-symbols . (key-signature time-signature))
-	(non-break-align-symbols . (multi-measure-rest))
+	(non-break-align-symbols . (multi-measure-rest-interface))
 	(non-musical . #t)
 	(meta . ((class . Item)
 		 (interfaces . (break-alignable-interface
-- 
1.7.0.4
1

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[hanwenn, 2010-08-29 18:59:31]

On Sun, Aug 29, 2010 at 3:54 PM, Jan Nieuwenhuizen
<janneke-list@xs4all.nl> wrote:
> Op zondag 29-08-2010 om 18:04 uur [tijdzone +0000], schreef
> hanwenn@gmail.com:
>
>> http://codereview.appspot.com/1579041/diff/42001/43004#newcode109
>> lily/metronome-engraver.cc:109: }
>> this is out of style with the rest of lilypond code base.
>>
>> The normal pattern is to use XXx_interface::has_interface (for
>> hard-coded interfaces), or grob->[internal_]has_interface(), to check
>> for softcoded interfaces.
>
> Thanks for catching this.  I've applied the patch below.

LGTM


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

[Reinhold, 2010-08-29 19:54:28]

First, great to see that this feature has finally been implemented and pushed.
Unfortunately, it seems that it needs some small tweaking, still. The problem is
that the metronome mark is now placed directly above the key signature, while
Gardner Read says that it "is aligned over the meter signature, or -- if none is
present -- over the first notational element of the measure, such as note-heads,
accidentals, repeat signs, and so on." (Gardner Read, p.278)

So, it should never be aligned with the key signature, but rather with the time
signature or the first element after that... Actually, Gardner Read shows an
example with a key signature and a metronome mark, where the metronome mark is
really aligned with the time signature and not with the key signature...

[hanwenn, 2010-08-29 20:09:55]

http://codereview.appspot.com/1579041/diff/42001/43004
File lily/metronome-engraver.cc (right):

http://codereview.appspot.com/1579041/diff/42001/43004#newcode81
lily/metronome-engraver.cc:81: }
is there a reason you are worried about cyclical data structures?  I don't think
we check for them anywhere else.

[janneke-list_xs4all.nl, 2010-08-29 20:21:05]

Op zondag 29-08-2010 om 20:09 uur [tijdzone +0000], schreef
hanwenn@gmail.com:
> http://codereview.appspot.com/1579041/diff/42001/43004
> File lily/metronome-engraver.cc (right):
> 
> http://codereview.appspot.com/1579041/diff/42001/43004#newcode81
> lily/metronome-engraver.cc:81: }
> is there a reason you are worried about cyclical data structures?  I
> don't think we check for them anywhere else.

It's not cyclic, the idea was about not crashing when the data
structure is not a list at all.

I think Neil had a remark about this.

Jan.

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[hanwenn, 2010-08-29 20:46:09]

On Sun, Aug 29, 2010 at 5:21 PM, Jan Nieuwenhuizen
<janneke-list@xs4all.nl> wrote:

> It's not cyclic, the idea was about not crashing when the data
> structure is not a list at all.
>
> I think Neil had a remark about this.

Most of the similar code uses scm_c_memq instead, which skips the
checking.   On closer inspection,  scm_ilength (which does the actual
checking) also catches other errors besides circular lists.

We could make more strict checking a habit, but it's not really a
scalable, since many routines require more complex data structures as
input parameters.

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

[Neil Puttock, 2010-08-29 22:02:18]

On 2010/08/29 19:54:28, Reinhold wrote:
> First, great to see that this feature has finally been implemented and pushed.
> Unfortunately, it seems that it needs some small tweaking, still. The problem
is
> that the metronome mark is now placed directly above the key signature, while
> Gardner Read says that it "is aligned over the meter signature, or -- if none
is
> present -- over the first notational element of the measure, such as
note-heads,
> accidentals, repeat signs, and so on." (Gardner Read, p.278)
> 
> So, it should never be aligned with the key signature, but rather with the
time
> signature or the first element after that... Actually, Gardner Read shows an
> example with a key signature and a metronome mark, where the metronome mark is
> really aligned with the time signature and not with the key signature...

Hmm, I took that to mean key signature as first notational element, but checking
a few scores at random suggests it to be uncommon.

Unfortunately, I didn't do any tests with key signatures present, but it's
actually worse than you've found: the Metronome_engraver acknowledges *all* key
signatures, including those which will be suicided later due to
break-visibility.  This means that if there's a key signature present, it
hijacks the metronome mark's positioning for a note at the start of a bar,
resulting in its being aligned on the barline:

\relative c' {
  \key g \major
  % \override Score.MetronomeMark #'break-align-symbols = #'(time-signature)
  c1
  \tempo Allegro
  c1
}

On a related note, I see now why the Metronome_engraver ignores the ordering of
break-align-symbols: it doesn't acknowledge a BreakAlignment (since it needs to
acknowledge the break-aligned directly).

[Neil Puttock, 2010-08-29 23:27:40]

On 2010/08/29 22:02:18, Neil Puttock wrote:
> On 2010/08/29 19:54:28, Reinhold wrote:
> > First, great to see that this feature has finally been implemented and
pushed.
> > Unfortunately, it seems that it needs some small tweaking, still. The
problem
> is
> > that the metronome mark is now placed directly above the key signature,
while
> > Gardner Read says that it "is aligned over the meter signature, or -- if
none
> is
> > present -- over the first notational element of the measure, such as
> note-heads,
> > accidentals, repeat signs, and so on." (Gardner Read, p.278)
> > 
> > So, it should never be aligned with the key signature, but rather with the
> time
> > signature or the first element after that... Actually, Gardner Read shows an
> > example with a key signature and a metronome mark, where the metronome mark
is
> > really aligned with the time signature and not with the key signature...
> 
> Hmm, I took that to mean key signature as first notational element, but
checking
> a few scores at random suggests it to be uncommon.
> 
> Unfortunately, I didn't do any tests with key signatures present, but it's
> actually worse than you've found: the Metronome_engraver acknowledges *all*
key
> signatures, including those which will be suicided later due to
> break-visibility.  This means that if there's a key signature present, it
> hijacks the metronome mark's positioning for a note at the start of a bar,
> resulting in its being aligned on the barline:
> 
> \relative c' {
>   \key g \major
>   % \override Score.MetronomeMark #'break-align-symbols = #'(time-signature)
>   c1
>   \tempo Allegro
>   c1
> }
> 
> On a related note, I see now why the Metronome_engraver ignores the ordering
of
> break-align-symbols: it doesn't acknowledge a BreakAlignment (since it needs
to
> acknowledge the break-aligned directly).

I've posted a fix for all these issues here:

http://codereview.appspot.com/2042043/

[janneke-list_xs4all.nl, 2010-08-30 07:41:55]

Op zondag 29-08-2010 om 23:27 uur [tijdzone +0000], schreef
n.puttock@gmail.com:

Neil,

> I've posted a fix for all these issues here:
> 
> http://codereview.appspot.com/2042043/

Thanks!

There seems to be no difference in the metronome-marking-break-align
regression test file, you'll want to amend it so that it shows what
is fixed.

Greetings, Jan

> http://codereview.appspot.com/1579041/

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyOfSource.com | Avatar®  http://AvatarAcademy.nl  

[Neil Puttock, 2010-09-01 22:12:32]

On 2010/08/30 07:41:55, janneke-list_xs4all.nl wrote:

> Thanks!
> 
> There seems to be no difference in the metronome-marking-break-align
> regression test file, you'll want to amend it so that it shows what
> is fixed.

Yes, I found that slightly puzzling, since your patch produced verifiable
changes in the existing tests for metronome marks.

Cheers,
Neil

[Colin Campbell, 2011-06-27 01:37:33]

I gather this is connected to issue 684, so it should probably be marked closed,
Jan.

Thanks,
Colin