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

Issue 326970043: Let arpeggio-bracket/slur depend on grob-property thickness (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by thomasmorley651
Modified:
4 months, 3 weeks ago
Reviewers:
Dan Eble, carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Let arpeggio-bracket/slur depend on grob-property thickness Before thickness based on line-thickness from layout

Patch Set 1 #

Total comments: 4

Patch Set 2 : Dan's comments #

Patch Set 3 : let arpeggio-slur rely on grob-line-thickness as well #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -4 lines) Patch
M lily/arpeggio.cc View 1 2 4 chunks +13 lines, -4 lines 2 comments Download
M scm/define-grobs.scm View 1 2 2 chunks +2 lines, -0 lines 1 comment Download

Messages

Total messages: 9
thomasmorley651
Please review. (It's my first patch changing operative c++-code on my own)
2 years, 11 months ago (2017-07-28 21:16:46 UTC) #1
Dan Eble
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 ...
2 years, 11 months ago (2017-07-28 23:12:37 UTC) #2
thomasmorley651
Dan's comments
2 years, 11 months ago (2017-07-29 09:16:58 UTC) #3
thomasmorley651
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 ...
2 years, 11 months ago (2017-07-29 09:25:11 UTC) #4
Dan Eble
I'm no expert, but LGTM.
2 years, 11 months ago (2017-07-29 13:51:20 UTC) #5
thomasmorley651
let arpeggio-slur rely on grob-line-thickness as well
2 years, 11 months ago (2017-08-01 10:54:42 UTC) #6
Carl
I believe that line-thickness should not be changed for the grob; just thickness. line-thickness should ...
2 years, 11 months ago (2017-08-01 12:45:59 UTC) #7
Carl
After reviewing the slur code, I remove my objections to using grob.line-thickness in this patch.
2 years, 11 months ago (2017-08-01 12:53:25 UTC) #8
thomasmorley651
2 years, 11 months ago (2017-08-01 16:15:00 UTC) #9
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
Sign in to reply to this message.

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