Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(73)

Issue 39047: Implement framework for post-fix text (de)cresc spanners (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years ago by Reinhold
Modified:
14 years, 7 months ago
Reviewers:
Neil Puttock
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Implement framework for post-fix text (de)cresc spanners The main problem why \decr or \dim were prefix operators was that the new dynamic engraver was reading only the properties of the Engraver itself, so there was no way to create a CrescendoEvent and at the same time specify the hairpin/text behavior. This patch makes the New_dynamic_engraver also look at the properties of the (Dec|C)recendoEvent and prefer a setting from there. If the event does not have a property (which will be the case most of the time), the Engraver's settings are used (i.e. same behavior as now). With this change, e.g. \dim can be defined as dim = #(make-music 'DecrescendoEvent 'span-direction START 'decrescendoSpanner 'text 'decrescendoText "dim.") and used as postfix like a4\dim without the need of any overrides before the affected note.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Add regtest, use property-name instead of PropertyName #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -31 lines) Patch
A input/regression/dynamics-custom-text-spanner-postfix.ly View 1 chunk +25 lines, -0 lines 0 comments Download
A input/regression/dynamics-text-spanner-postfix.ly View 1 chunk +16 lines, -0 lines 0 comments Download
M lily/new-dynamic-engraver.cc View 1 5 chunks +17 lines, -2 lines 0 comments Download
M ly/spanners-init.ly View 1 chunk +6 lines, -29 lines 0 comments Download

Messages

Total messages: 1
Neil Puttock
15 years ago (2009-04-11 18:33:47 UTC) #1
I never realized this would be so simple, but it strikes me as a bit of a hack.

In your sample text dynamic spanners, there seems to be an element of redundancy
in the event properties; you could easily junk '(de)crescendoSpanner and
'(de)crescendoText, using 'text only to trigger the change, though I'm still not
sure I like this either:

+      SCM event_text = current_span_event_->get_property ("dynamic-text");
       SCM cresc_type = get_property ((start_type + "Spanner").c_str ());
 
-      if (cresc_type == ly_symbol2scm ("text"))
+      if (cresc_type == ly_symbol2scm ("text")
+	  || Text_interface::is_markup (event_text))
 	{
 	  current_spanner_
 	    = make_spanner ("DynamicTextSpanner",
 			    accepted_spanevents_drul_[START]->self_scm ());
 
-	  SCM text = get_property ((start_type + "Text").c_str ());
+	  SCM text = ((event_text != SCM_EOL)
+		      ? event_text
+		      : get_property ((start_type + "Text").c_str ()));
 	  if (Text_interface::is_markup (text))
 	    {
 	      current_spanner_->set_property ("text", text);
 	    }
 	}

http://codereview.appspot.com/39047/diff/1/2
File lily/new-dynamic-engraver.cc (right):

http://codereview.appspot.com/39047/diff/1/2#newcode76
Line 76: SCM spanner_type = evt->get_property (name);
This looks weird to me, since you're taking advantage of the fact that the event
properties you're setting look like context properties.  You'd need a method
which does the reverse of camel_case_to_lisp_identifier () so that the events
would be converted from e.g. crescendo-spanner -> crescendoSpanner.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b