Issue 3942: Scale slurs and ties when using \magnifyMusic.

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

[Mark Polesky, 2014-06-06 09:29:03]

Please review this new patch.  I'm not entirely sure this is the right approach,
so I may need some advice here.

Thank you!
- Mark

[Mark Polesky, 2014-06-13 17:05:21]

On 2014/06/06 09:29:03, Mark Polesky wrote:
> Please review this new patch.  I'm not entirely sure this
> is the right approach, so I may need some advice here.

This patch has gone through the review/countdown process without a single
comment or review, but I'm not comfortable pushing it without any feedback.  In
the definition of \magnifyMusic (in music-functions-init.ly), I've used about 50
temporary overrides, manually entering scaled values based on the normal default
values for each grob-property I'm overriding.  Ideally, if the user has already
modified some of those values, I'd like to base the new scaled values on the
user's choices, and not base them on the LilyPond default values.  If that's
possible at all, I don't know how to do that.  Also, if anyone has any
suggestions for a more elegant approach, please let me know.

Thanks.
- Mark

[dak, 2014-06-14 06:58:52]

markpolesky@gmail.com writes:

> On 2014/06/06 09:29:03, Mark Polesky wrote:
>> Please review this new patch.  I'm not entirely sure this
>> is the right approach, so I may need some advice here.
>
> This patch has gone through the review/countdown process without a
> single comment or review, but I'm not comfortable pushing it without any
> feedback.  In the definition of \magnifyMusic (in
> music-functions-init.ly), I've used about 50 temporary overrides,
> manually entering scaled values based on the normal default values for
> each grob-property I'm overriding.  Ideally, if the user has already
> modified some of those values, I'd like to base the new scaled values on
> the user's choices, and not base them on the LilyPond default values.
> If that's possible at all, I don't know how to do that.

If you do a read-modify-write on properties, you have to use \applyMusic
for that.

> https://codereview.appspot.com/103890046/

-- 
David Kastrup

[Mark Polesky, 2014-06-15 03:41:58]

On 2014/06/14 06:58:52, dak wrote:
>> On 2014/06/06 09:29:03, Mark Polesky wrote:
>> ... Ideally, if the user has already
>> modified some of those values, I'd like to base the new scaled values on
>> the user's choices, and not base them on the LilyPond default values.
>> If that's possible at all, I don't know how to do that.
> 
> If you do a read-modify-write on properties, you have to use \applyMusic
> for that.

David, 
I'm sorry to say that I'm unable to figure this out on my own.  \applyMusic is
completely undocumented and the few occurrences in the source code are not
illustrative enough for me.  Within an \applyMusic construct, how do I access a
given grob-property value, and then override it -- temporarily?

Thanks.
- Mark

[dak, 2014-06-15 05:25:27]

markpolesky@gmail.com writes:

> On 2014/06/14 06:58:52, dak wrote:
>>> On 2014/06/06 09:29:03, Mark Polesky wrote:
>>> ... Ideally, if the user has already
>>> modified some of those values, I'd like to base the new scaled values
> on
>>> the user's choices, and not base them on the LilyPond default values.
>>> If that's possible at all, I don't know how to do that.
>
>> If you do a read-modify-write on properties, you have to use
> \applyMusic
>> for that.
>
> David,
> I'm sorry to say that I'm unable to figure this out on my own.
> \applyMusic is completely undocumented and the few occurrences in the
> source code are not illustrative enough for me.  Within an \applyMusic
> construct, how do I access a given grob-property value, and then
> override it -- temporarily?

It would have helped if I had written the correct command to use.  It
is, of course, \applyContext.

The documentation does not help much, but one illustrative
read/modify/write use is in scm/music-functions.scm in
add-grace-property and remove-grace-property.

> https://codereview.appspot.com/103890046/

-- 
David Kastrup

[Mark Polesky, 2014-06-20 09:29:45]

On 2014/06/15 05:25:27, dak wrote:
> It would have helped if I had written the correct command to use.  It
> is, of course, \applyContext.
> 
> The documentation does not help much, but one illustrative
> read/modify/write use is in scm/music-functions.scm in
> add-grace-property and remove-grace-property.

Well, I finally understand this much more than before (thank you David), but I
still need someone to check my work here.  I've completely overhauled the
\magnifyMusic function, so now if the user has overridden any default values,
those adjustments will still be respected as the size increases or decreases.

I tried to code as clearly and concisely as possible, but it ended up longer
than I imagined.  I'm wondering if I should be defining some of the longer
scheme chunks in some other file, to make the magnifyMusic definition easier to
read?

Also, my (scale-prop) procedure isn't the most generic thing around, though I
think it fits the bill for what it is.

Looking forward to getting some feedback.
- Mark

[Mark Polesky, 2014-06-30 22:55:24]

This new patch went through a countdown cycle without any 
reviews.  I'm putting it through another cycle in case
anyone would like to make comments before I push it. 

Thanks.
- Mark

[pwm, 2014-07-04 14:04:35]

I happened to notice one little thing while I was skimming through this (i.e.
not a full review, for which I'm not really qualified).

https://codereview.appspot.com/103890046/diff/280001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/103890046/diff/280001/scm/music-functions.scm#...
scm/music-functions.scm:2469: props))))
This can be simplified to: (for-each scale-prop props)

[dak, 2014-07-06 14:04:07]

Yeah, I know.  Late again.  I was really having problems catching up with
various things.

https://codereview.appspot.com/103890046/diff/360001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/103890046/diff/360001/ly/music-functions-init....
ly/music-functions-init.ly:645: Stem.thickness
This is madness.  Stem.thickness and its ilk don't make sense as symbols at all:
unique symbols come at a cost.  You cannot use those here anyway without
converting the symbol to a string, splitting the string at ".", and convert back
to symbols again.  There is no point in not writing
   (Stem thickness)
   (Slur line-thickness)
   (Slur thickness)
and so on in the first place and it's not like it would be more verbose.

The LilyPond expression Stem.thickness _is_ represented in Scheme by '(Stem
thickness).  You can write

$(define-void-function (parser location sl) (symbol-list?) (write sl))
Stem.thickness

and you can also write

\override #'(Stem thickness) = 5

So there is no point in creating some LilyPond/Scheme hybrid in between.

https://codereview.appspot.com/103890046/diff/360001/ly/music-functions-init....
ly/music-functions-init.ly:679: Slur.details.region-size
I doubt you even have a chance of making this work.  Overriding and reverting
bulks of *nested* properties does not work reliably.

[Mark Polesky, 2014-07-07 02:21:59]

On 2014/07/06 14:04:07, dak wrote:
> ly/music-functions-init.ly:645: Stem.thickness
> This is madness.  Stem.thickness and its ilk don't make sense as symbols at
all

Okay

> ly/music-functions-init.ly:679: Slur.details.region-size
> I doubt you even have a chance of making this work.  Overriding and reverting
> bulks of *nested* properties does not work reliably.

Okay

Since this patch has already been pushed, I'm moving the discussion to the new
patch here:
http://codereview.appspot.com/110840044

[dak, 2014-07-07 18:22:09]

https://codereview.appspot.com/103890046/diff/250001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/103890046/diff/250001/ly/music-functions-init....
ly/music-functions-init.ly:718: \context Voice {
This would create a new Staff when using
\new TabStaff { \magnifyMusic ...

It would seem safer to use \context Bottom instead.