Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

Thomas Morley-2
Reviewers: ,

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

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

Before thickness based on line-thickness from layout

Please review this at https://codereview.appspot.com/326970043/

Affected files (+2, -2 lines):
   M lily/arpeggio.cc


Index: lily/arpeggio.cc
diff --git a/lily/arpeggio.cc b/lily/arpeggio.cc
index  
95da047a067bd5ced57ba29f53a0a2ee93405bbe..e38f554c7bdd6425181a800870a196824aac1065  
100644
--- a/lily/arpeggio.cc
+++ b/lily/arpeggio.cc
@@ -189,7 +189,7 @@ Arpeggio::brew_chord_bracket (SCM smob)
                                          Interval ())
                     * Staff_symbol_referencer::staff_space (me);

-  Real lt = me->layout ()->get_dimension (ly_symbol2scm  
("line-thickness"));
+  Real lt = robust_scm2double (me->get_property ("thickness"), 0.1);
    Real sp = 1.5 * Staff_symbol_referencer::staff_space (me);
    Real dy = heads.length () + sp;
    Real x = robust_scm2double (me->get_property ("protrusion"), 0.4);
@@ -209,7 +209,7 @@ Arpeggio::brew_chord_slur (SCM smob)
                                          Interval ())
                     * Staff_symbol_referencer::staff_space (me);

-  Real lt = me->layout ()->get_dimension (ly_symbol2scm  
("line-thickness"));
+  Real lt = robust_scm2double (me->get_property ("thickness"), 0.1);
    Real dy = heads.length ();

    Real height_limit = 1.5;



_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

nine.fierce.ballads

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.

https://codereview.appspot.com/326970043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

Thomas Morley-2
In reply to this post by Thomas Morley-2

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.

https://codereview.appspot.com/326970043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

nine.fierce.ballads
In reply to this post by Thomas Morley-2
I'm no expert, but LGTM.

https://codereview.appspot.com/326970043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

Carl Sorensen
In reply to this post by Thomas Morley-2
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#newcode175
scm/define-grobs.scm:175: (line-thickness . 1)
I don't think line-thickness should be added here; you should just use
thickness

https://codereview.appspot.com/326970043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

Carl Sorensen
In reply to this post by Thomas Morley-2
After reviewing the slur code, I remove my objections to using
grob.line-thickness in this patch.


https://codereview.appspot.com/326970043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Let arpeggio-bracket/slur depend on grob-property thickness (issue 326970043 by thomasmorley65@gmail.com)

Thomas Morley-2
In reply to this post by Thomas Morley-2
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

https://codereview.appspot.com/326970043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Loading...