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

Issue 4095041: Fix 1120 in a way to avoid issues 1472, 1474 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 2 months ago by Keith
Modified:
13 years, 1 month ago
Reviewers:
hanwenn, carl.d.sorensen, Neil Puttock, Graham Percival (old account)
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix note spacing with melismata by receding the extra-spacing-height of LyricText and similar items. Create property skyline-vertical-padding for the vertical padding of left- and right-skylines used in note spacing.

Patch Set 1 #

Patch Set 2 : revert ee0488 #

Patch Set 3 : recede extra-spacing-height #

Total comments: 9

Patch Set 4 : Handle all issues ee0488 did, update regtest #

Total comments: 2

Patch Set 5 : recede LeftEdge #

Patch Set 6 : new property skyline-vertical-padding #

Total comments: 7

Patch Set 7 : formatting #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -11 lines) Patch
M input/regression/lyrics-melisma-beam.ly View 1 2 3 2 chunks +5 lines, -4 lines 0 comments Download
M lily/separation-item.cc View 1 2 3 4 5 6 3 chunks +5 lines, -3 lines 0 comments Download
M scm/define-grob-properties.scm View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M scm/define-grobs.scm View 1 2 3 4 5 6 10 chunks +9 lines, -4 lines 0 comments Download

Messages

Total messages: 22
Keith
upload.py is now skipping over the base files in scm for me - the problem ...
13 years, 2 months ago (2011-01-21 04:22:58 UTC) #1
Keith
On 2011/01/21 04:22:58, Keith wrote: > upload.py is now skipping over the base files in ...
13 years, 2 months ago (2011-01-21 05:18:25 UTC) #2
Graham Percival (old account)
Regtest comparison shows nothing obviously wrong, and it compiles the docs from scratch.
13 years, 2 months ago (2011-01-21 13:13:43 UTC) #3
Keith
Needs adjustments as marked, to handle all the other issues that commit ee0488 fixed : ...
13 years, 1 month ago (2011-01-23 04:48:19 UTC) #4
Keith
Extended to cover the other issues that were fixed along with 1120. The regression test ...
13 years, 1 month ago (2011-01-24 05:15:07 UTC) #5
Carl
This patch also resolves the problem in issue 1229 of notes extending beyond the right-hand ...
13 years, 1 month ago (2011-01-25 04:47:23 UTC) #6
Graham Percival (old account)
On 2011/01/25 04:47:23, Carl wrote: > This patch seems to have some very good benefits. ...
13 years, 1 month ago (2011-01-26 18:18:21 UTC) #7
Keith
On 2011/01/26 18:18:21, Graham Percival wrote: > Neil has identified a potential downside to this ...
13 years, 1 month ago (2011-01-29 21:34:13 UTC) #8
Carl
This patch solves Neil's problem with clef spacing. LGTM. Carl
13 years, 1 month ago (2011-01-29 23:00:09 UTC) #9
Keith
On 2011/01/29 21:34:13, Keith wrote: > Status of this patch is [...] > 3)regression test ...
13 years, 1 month ago (2011-01-30 19:29:04 UTC) #10
hanwenn
2 issues: * this hardcodes 0.1 in several places. Make a property, so we can ...
13 years, 1 month ago (2011-01-31 11:20:39 UTC) #11
Keith
On 2011/01/31 11:20:39, hanwenn wrote: > * this hardcodes 0.1 in several places. Make a ...
13 years, 1 month ago (2011-01-31 19:36:16 UTC) #12
Carl
On 2011/01/31 19:36:16, Keith wrote: > On 2011/01/31 11:20:39, hanwenn wrote: > > * this ...
13 years, 1 month ago (2011-01-31 19:50:38 UTC) #13
Keith
On 2011/01/31 19:50:38, Carl wrote: > > We could then get the value of default-separation ...
13 years, 1 month ago (2011-01-31 20:20:47 UTC) #14
Neil Puttock
On 31 January 2011 20:20, <k-ohara5a5a@oco.net> wrote: > I had discounted the new property idea ...
13 years, 1 month ago (2011-01-31 23:28:06 UTC) #15
Keith
On 2011/01/31 19:50:38, Carl wrote: > Since the 0.1s are all part of a Separation_item, ...
13 years, 1 month ago (2011-02-01 09:33:22 UTC) #16
Keith
http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/4095041/diff/52002/scm/define-grobs.scm#newcode1309 scm/define-grobs.scm:1309: (skyline-vertical-padding . 0.15) The starting value 0.15 matches version ...
13 years, 1 month ago (2011-02-01 09:33:48 UTC) #17
Graham Percival (old account)
LGTM
13 years, 1 month ago (2011-02-01 21:49:02 UTC) #18
hanwenn
LGTM On Tue, Feb 1, 2011 at 7:49 PM, <percival.music.ca@gmail.com> wrote: > LGTM > > ...
13 years, 1 month ago (2011-02-02 01:23:10 UTC) #19
Neil Puttock
LGTM. http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc File lily/separation-item.cc (right): http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc#newcode83 lily/separation-item.cc:83: double horizon_padding = robust_scm2double (me->get_property ("skyline-vertical-padding"), 0.0); Real ...
13 years, 1 month ago (2011-02-02 23:14:39 UTC) #20
Keith
On 2011/02/02 23:14:39, Neil Puttock wrote: > LGTM. > > http://codereview.appspot.com/4095041/diff/52002/lily/separation-item.cc > File lily/separation-item.cc (right): ...
13 years, 1 month ago (2011-02-03 07:29:15 UTC) #21
Graham Percival (old account)
13 years, 1 month ago (2011-02-03 18:24:33 UTC) #22
Latest version LGTM.

Keith, could you send me the git-format patch privately, so that I can push it
in 24 hours if nobody complains?
Sign in to reply to this message.

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