Code review - Issue 1579041: Align metronome mark at time signature or first musical element. Fixes #684.https://codereview.appspot.com/2011-06-27T01:37:33+00:00rietveld
Message from unknown
2010-06-07T10:05:48+00:00jannekeurn:md5:7f1787a9590c3e1af66a309d5e604c7d
Message from unknown
2010-06-07T10:40:42+00:00jannekeurn:md5:a3f4bfd75b66395d3029276b32355921
Message from n.puttock@gmail.com
2010-06-07T16:59:14+00:00Neil Puttockurn:md5:bdb36f664a6d4e7b2fabb91091e32f4c
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)
Message from unknown
2010-06-07T21:51:15+00:00jannekeurn:md5:12ba1ed9add1ce8d71f6d3f62eab8eeb
Message from janneke-list@xs4all.nl
2010-06-07T21:54:05+00:00janneke-list_xs4all.nlurn:md5:ce5ae1252b352bc17a827e0453d67ee0
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
Message from n.puttock@gmail.com
2010-06-08T22:22:43+00:00Neil Puttockurn:md5:5fd536a5be234aaf77180a781323a509
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.
Message from unknown
2010-06-14T15:16:22+00:00jannekeurn:md5:24f6f9385fd20f5dbbb4e62b248137af
Message from unknown
2010-06-14T19:16:55+00:00jannekeurn:md5:8f6a9bf893a3c785d6c5571f7a2c8232
Message from jan.nieuwenhuizen@gmail.com
2010-06-16T12:08:06+00:00jannekeurn:md5:405e7813c38352838a8d3651ab3d5a1f
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.
Message from n.puttock@gmail.com
2010-06-16T22:37:14+00:00Neil Puttockurn:md5:ee5c11981d38475d15252ee97630fa0f
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
Message from n.puttock@gmail.com
2010-06-20T21:57:20+00:00Neil Puttockurn:md5:16fbc55699505ad0bf0b20e57a02e4d6
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)
Message from jan.nieuwenhuizen@gmail.com
2010-06-22T18:51:57+00:00jannekeurn:md5:c0a608f965911793d0bee1de4d54ae0e
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.
Message from jan.nieuwenhuizen@gmail.com
2010-06-22T19:01:58+00:00jannekeurn:md5:c4675353ea023a1789361c078abf179e
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.
Message from c_sorensen@byu.edu
2010-06-22T19:04:14+00:00c_sorensenurn:md5:a7851bd858ac164c4c3bd10d217b2b5c
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
Message from unknown
2010-06-22T19:05:09+00:00jannekeurn:md5:682646bf0eeda73679eb25bc67b9fea6
Message from c_sorensen@byu.edu
2010-06-22T19:05:29+00:00c_sorensenurn:md5:0956a6abe50514efd663d62d63978246
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
Message from jan.nieuwenhuizen@gmail.com
2010-07-01T08:08:36+00:00jannekeurn:md5:4eb79edb11fc4fbcc98f8b71e30039bd
So, how are we doing here? Ready to commit & close
http://code.google.com/p/lilypond/issues/detail?id=684
Message from n.puttock@gmail.com
2010-07-01T21:34:01+00:00Neil Puttockurn:md5:95eef2bc1188686084cef43fd4e8d329
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
Message from n.puttock@gmail.com
2010-07-04T19:54:54+00:00Neil Puttockurn:md5:7b71d584decd293de3d4b64f70d60b54
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.
Message from janneke-list@xs4all.nl
2010-07-04T20:36:28+00:00janneke-list_xs4all.nlurn:md5:42f7e01174b10562e2e9bfb3b9d8b9a0
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
Message from n.puttock@gmail.com
2010-07-04T21:14:52+00:00Neil Puttockurn:md5:80f1541b58a980d2cdafec6db758881a
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.
Message from n.puttock@gmail.com
2010-07-04T21:15:10+00:00Neil Puttockurn:md5:3e76f299282ff7f50de05914c38cc79c
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)
Message from reinhold@kainhofer.com
2010-07-27T13:42:01+00:00reinhold_kainhofer.comurn:md5:01438d5cd236d843d56828bb0a874052
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
Message from janneke-list@xs4all.nl
2010-07-27T14:14:12+00:00janneke-list_xs4all.nlurn:md5:ac3ad743a9d3dd117a1fac7397fc7a65
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
Message from n.puttock@gmail.com
2010-08-10T21:36:28+00:00Neil Puttockurn:md5:ac778db989084f86bb18145e655c021b
On 2010/07/27 14:14:12, janneke-list_xs4all.nl wrote:
> I'm "waiting" for an ack.
From whom?
Message from unknown
2010-08-24T13:25:45+00:00jannekeurn:md5:7a816f3c71c077ffcd76cc61be82ae10
Message from jan.nieuwenhuizen@gmail.com
2010-08-24T13:26:14+00:00jannekeurn:md5:096f8f7b70072bb1d4b2b5240ea44c73
> 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.
Message from n.puttock@gmail.com
2010-08-25T20:54:24+00:00Neil Puttockurn:md5:5873add24192661f43e6bc81f4511357
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.
Message from n.puttock@gmail.com
2010-08-26T22:49:10+00:00Neil Puttockurn:md5:032c42a2020c028253d9e455b5dac12d
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
Message from jan.nieuwenhuizen@gmail.com
2010-08-27T09:50:47+00:00jannekeurn:md5:919bd2001da40cd035c8273183e7b56b
Thanks. Applied and set to Fixed.
Message from hanwenn@gmail.com
2010-08-29T18:04:00+00:00hanwennurn:md5:737d921fb7cfb3ef65163f3c19a1c883
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
Message from janneke-list@xs4all.nl
2010-08-29T18:54:25+00:00janneke-list_xs4all.nlurn:md5:152c9ccef6b0bad9861cb41300acfa07
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
Message from hanwenn@gmail.com
2010-08-29T18:59:31+00:00hanwennurn:md5:fcdf76fb7b04e720dffb9c6f6b11cb00
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
Message from reinhold.kainhofer@gmail.com
2010-08-29T19:54:28+00:00Reinholdurn:md5:a6417aca34cf083d27156775ee927005
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...
Message from hanwenn@gmail.com
2010-08-29T20:09:55+00:00hanwennurn:md5:c5b3bdd426b578c23d9e5513b0096655
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.
Message from janneke-list@xs4all.nl
2010-08-29T20:21:05+00:00janneke-list_xs4all.nlurn:md5:c3a1103ee2da5661e4c6d04dd6552d0c
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
Message from hanwenn@gmail.com
2010-08-29T20:46:09+00:00hanwennurn:md5:c78175735594072d99872a6366b3be1d
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
Message from n.puttock@gmail.com
2010-08-29T22:02:18+00:00Neil Puttockurn:md5:ff6cf34fff026ef046689070fc8f63f2
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).
Message from n.puttock@gmail.com
2010-08-29T23:27:40+00:00Neil Puttockurn:md5:bdcbd2ef8a2a2601f38caeb0792de730
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/
Message from janneke-list@xs4all.nl
2010-08-30T07:41:55+00:00janneke-list_xs4all.nlurn:md5:2a28376808cddaa5bd01e89f816a9525
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
Message from n.puttock@gmail.com
2010-09-01T22:12:32+00:00Neil Puttockurn:md5:0b34943b6ca6128e0486084420a35673
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
Message from ColinPKCampbell@gmail.com
2011-06-27T01:37:33+00:00Colin Campbellurn:md5:4ff152d514e0dc66e7b5f203619f6de5
I gather this is connected to issue 684, so it should probably be marked closed, Jan.
Thanks,
Colin