Re: [PATCH] Optionally add text to metronome marks (i.e. allow \tempo "Allegro" 4=120)

[Han-Wen Nienhuys]

On Thu, Jun 19, 2008 at 3:16 PM, Reinhold Kainhofer
<address@hidden> wrote:

>> Oops, you really got me there. If there is a text, I set the tempoText
>> property, but I forgot that if that property is already set, I need to
>> unset it there... Fixed with the attached patch.
>
> Since that last mail didn't get any responses, I'll ask again: Is the patch
> now okay to be pushed to master?

sorry for the delay, feel free to apply after taking these comments
into consideration.


> One thing that I don't like about it is that in the parser I haven't found a
> way to match a string or markup, so I had to duplicate the cases:
>
> +       | TEMPO string steno_duration '=' bare_unsigned {
> +               $$ = MAKE_SYNTAX ("tempo", @$, make_simple_markup($2), $3,
> scm_int2num ($5));
> +       }
> +       | TEMPO full_markup steno_duration '=' bare_unsigned    {
> +               $$ = MAKE_SYNTAX ("tempo", @$, $2, $3, scm_int2num ($5));
> +       }
>
> Did I miss some pattern that matches a string and a full markup (and can be
> used equivalently in the tempo and tempoText calls)?

No. Please add a rule.

Thinking about this a little more... I think the formatting support
for this is definitely the worth the effort, but would it not be a
better idea to use a standard music function/properties-sets on the
input side?  How big is the necessity for supporting 4 different
variants of input directly in the parser ?

+    (set! props (append props (list
+            (if text (make-property-set 'tempoText text)
+                     (make-property-unset 'tempoText)))))

it usually is shorter and more efficient to prepend (ie. to cons) the
extra elements onto the front of the list

+      )
+    )
+  )
+)

don't put a single paren on a line (ie. move to previous line)

+          (make-concat-markup (list (make-simple-markup "(")
+                                    note-markup
+                                    (make-simple-markup ")")))))

that surprises me, I'd think you have to switch off spaces, otherwise you get

  (<SPACE>NOTE = xx <SPACE>)

did I miss the part that sets wordspace to 0 or does it look better
with the spaces?
-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen