Let arpeggio-bracket/slur depend on grob-property thickness

Let arpeggio-bracket/slur depend on grob-property thickness

Before thickness based on line-thickness from layout

[thomasmorley651, 2017-07-28 21:16:46]

Please review.
(It's my first patch changing operative c++-code on my own)

[Dan Eble, 2017-07-28 23:12:37]

https://codereview.appspot.com/326970043/diff/1/lily/arpeggio.cc
File lily/arpeggio.cc (right):

https://codereview.appspot.com/326970043/diff/1/lily/arpeggio.cc#newcode192
lily/arpeggio.cc:192: Real lt = robust_scm2double (me->get_property
("thickness"), 0.1);
1. The "thickness" property is documented in define-grob-properties.scm as being
a multiple of the current staff line thickness.  I'm having trouble figuring out
if that was also the case for what it is replacing.  Please confirm whether
using the raw thickness, rather than multiplying it by something, is correct
here.
2. Add the default thickness to define-grobs.scm?

https://codereview.appspot.com/326970043/diff/1/lily/arpeggio.cc#newcode251
lily/arpeggio.cc:251: /* properties */
Update this list.

[thomasmorley651, 2017-07-29 09:16:58]

Dan's comments

[thomasmorley651, 2017-07-29 09:25:11]

https://codereview.appspot.com/326970043/diff/1/lily/arpeggio.cc
File lily/arpeggio.cc (right):

https://codereview.appspot.com/326970043/diff/1/lily/arpeggio.cc#newcode192
lily/arpeggio.cc:192: Real lt = robust_scm2double (me->get_property
("thickness"), 0.1);
On 2017/07/28 23:12:37, Dan Eble wrote:
> 1. The "thickness" property is documented in define-grob-properties.scm as
being
> a multiple of the current staff line thickness. I'm having trouble figuring
out
> if that was also the case for what it is replacing.  Please confirm whether
> using the raw thickness, rather than multiplying it by something, is correct
> here.

As far as I can tell the printed thickness relied _only_ on staff line
thickness, without any possibility for the user to change it. Thus this patch.
I now changed the used thickness to the multiplication of staff line thickness
and grob thickness.

For the slur-style arpeggio I'm not sure. One could change it to use
grob.line-thickness _and_ grob.thickness as it is done for Slur.

Opinions?

> 2. Add the default thickness to define-grobs.scm?

Done.

https://codereview.appspot.com/326970043/diff/1/lily/arpeggio.cc#newcode251
lily/arpeggio.cc:251: /* properties */
On 2017/07/28 23:12:37, Dan Eble wrote:
> Update this list.

Done.

[Dan Eble, 2017-07-29 13:51:20]

I'm no expert, but LGTM.

[thomasmorley651, 2017-08-01 10:54:42]

let arpeggio-slur rely on grob-line-thickness as well

[Carl, 2017-08-01 12:45:59]

I believe that line-thickness should not be changed for the grob; just
thickness.  line-thickness should be the staff line-thickness, not changed per
grob, IMO.

https://codereview.appspot.com/326970043/diff/40001/lily/arpeggio.cc
File lily/arpeggio.cc (right):

https://codereview.appspot.com/326970043/diff/40001/lily/arpeggio.cc#newcode214
lily/arpeggio.cc:214: Real lt
I think lt should just be line-thickness, and th should be
thickness*line-thickness

https://codereview.appspot.com/326970043/diff/40001/lily/arpeggio.cc#newcode261
lily/arpeggio.cc:261: "line-thickness "
I think that only thickness should be added

https://codereview.appspot.com/326970043/diff/40001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/326970043/diff/40001/scm/define-grobs.scm#newc...
scm/define-grobs.scm:175: (line-thickness . 1)
I don't think line-thickness should be added here; you should just use thickness

[Carl, 2017-08-01 12:53:25]

After reviewing the slur code, I remove my objections to using
grob.line-thickness in this patch.

[thomasmorley651, 2017-08-01 16:15:00]

On 2017/08/01 12:53:25, Carl wrote:
> After reviewing the slur code, I remove my objections to using
> grob.line-thickness in this patch.

ok, so I'll go for patch set 3