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

Issue 4489042: Fix calculation of vertical offset when 'staff-padding is set (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years ago by Trevor Daniels
Modified:
8 years ago
Reviewers:
Janek Warchol, carl.d.sorensen, Neil Puttock, t.daniels
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix calculation of vertical offset when 'staff-padding is set - fix 877: Ottava clefs may not look good - the previous code incorrectly calculated the offset from the staff rather than the parent; this happened to coincide with the correct value for most parents but gave an incorrect offset for OctavateEight above the F-clef - add regression test for this case

Patch Set 1 #

Total comments: 6

Patch Set 2 : After dealing with comments from Janek and Neil #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -18 lines) Patch
A input/regression/clef-octavation.ly View 1 1 chunk +18 lines, -0 lines 0 comments Download
M lily/side-position-interface.cc View 1 9 chunks +23 lines, -18 lines 0 comments Download

Messages

Total messages: 10
Trevor Daniels
This is a fix for issue 877, "Ottava clefs may not look good". See http://code.google.com/p/lilypond/issues/detail?id=877 ...
8 years ago (2011-05-06 22:22:19 UTC) #1
Carl
LGTM. I tried fixing this but couldn't track down all the interfaces to figure it ...
8 years ago (2011-05-06 22:26:59 UTC) #2
t.daniels_treda.co.uk
Carl, you wrote Friday, May 06, 2011 11:26 PM > LGTM. Thanks! > I tried ...
8 years ago (2011-05-06 22:45:59 UTC) #3
Janek Warchol
Style nitpick. As for the code, i've read it, but unfortunately i'd have to study ...
8 years ago (2011-05-08 06:17:37 UTC) #4
t.daniels_treda.co.uk
lemniskata.bernoullego@gmail.com wrote Sunday, May 08, 2011 7:17 AM > Style nitpick. > http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc#newcode65 > lily/side-position-interface.cc:65: ...
8 years ago (2011-05-08 07:38:41 UTC) #5
Janek Warchol
2011/5/8 Trevor Daniels <t.daniels@treda.co.uk> > > lemniskata.bernoullego@gmail.com wrote Sunday, May 08, 2011 7:17 AM > ...
8 years ago (2011-05-10 05:11:36 UTC) #6
t.daniels_treda.co.uk
Janek Warchoł wrote Tuesday, May 10, 2011 6:11 AM > I remember that someone once ...
8 years ago (2011-05-10 07:27:14 UTC) #7
Neil Puttock
LGTM. http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly File input/regression/clef-octavation.ly (right): http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode3 input/regression/clef-octavation.ly:3: \header{ \header { http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode5 input/regression/clef-octavation.ly:5: texidoc=" Octavate symbols ...
8 years ago (2011-05-11 18:28:07 UTC) #8
t.daniels_treda.co.uk
Neil wrote Wednesday, May 11, 2011 7:28 PM > LGTM. Thanks! I'll push after a ...
8 years ago (2011-05-12 16:16:28 UTC) #9
Trevor Daniels
8 years ago (2011-05-16 09:21:54 UTC) #10
Pushed patchset 2 on 16 May 2011: 
e2a41749e09a68d2c8e453bff3c65404d6e50e34
Sign in to reply to this message.

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