Code review - Issue 4489042: Fix calculation of vertical offset when 'staff-padding is sethttps://codereview.appspot.com/2011-05-16T09:21:54+00:00rietveld
Message from unknown
2011-05-06T22:11:14+00:00Trevor Danielsurn:md5:9a59d12b39dec44b097a3ae928f79204
Message from tdanielsmusic@googlemail.com
2011-05-06T22:22:19+00:00Trevor Danielsurn:md5:392d5050e8575a6c061421fbc58f8593
This is a fix for issue 877, "Ottava clefs may not look good". See
http://code.google.com/p/lilypond/issues/detail?id=877
The regtests run clean except for
cue-clef-octavation.ly
skiptypesetting-multimeasurerest.ly
both of which are improved, and 1 pixel differences in
clef-oct.ly
Rather surprisingly this is a bug in
Side_position_interface::aligned_side ()
but it only appears to affect clefs.
Please review.
Trevor
Message from Carl.D.Sorensen@gmail.com
2011-05-06T22:26:59+00:00Carlurn:md5:39498ba48edc9137839926b0a28fe33a
LGTM.
I tried fixing this but couldn't track down all the interfaces to figure it out. I saw that we were going off staff position rather than parent position, but didn't know where to fix it.
Thanks!
Message from t.daniels@treda.co.uk
2011-05-06T22:45:59+00:00t.daniels_treda.co.ukurn:md5:0e235d84435145aa04efd7a65d425fc9
Carl, you wrote Friday, May 06, 2011 11:26 PM
> LGTM.
Thanks!
> I tried fixing this but couldn't track down all the interfaces to
> figure
> it out. I saw that we were going off staff position rather than
> parent
> position, but didn't know where to fix it.
I've done little else in my LilyPond time for the
past two weeks! But I now understand a bit more
about grob aligning!
Trevor
Message from lemniskata.bernoullego@gmail.com
2011-05-08T06:17:37+00:00Janek Warcholurn:md5:6cd3ff4483baecc56d50fb9c963340d4
Style nitpick.
As for the code, i've read it, but unfortunately i'd have to study it a lot more to understand it (my poor programming skills, eh :/)
http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):
http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc#newcode65
lily/side-position-interface.cc:65: bool include_staff =
I see quite a lot of whitespace diffs here and there. IIRC we care about them, so please fix :)
Message from t.daniels@treda.co.uk
2011-05-08T07:38:41+00:00t.daniels_treda.co.ukurn:md5:3e8373a75010d4899b1dd915ef037b20
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: bool include_staff =
> I see quite a lot of whitespace diffs here and there. IIRC we care
> about
> them, so please fix :)
If you look more carefully you'll see these changes
are actually correcting whitespace errors in the
original code. But thanks for taking the trouble
to review my fix :)
Trevor
Message from lemniskata.bernoullego@gmail.com
2011-05-10T05:11:36+00:00Janek Warcholurn:md5:a6eda6e2756e13072313384aa3b4f8dc
2011/5/8 Trevor Daniels <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: bool include_staff =
>> I see quite a lot of whitespace diffs here and there. IIRC we care about
>> them, so please fix :)
>>
>
> If you look more carefully you'll see these changes
> are actually correcting whitespace errors in the
> original code.
Ooops! Sorry :)
I remember that someone once told me that such canges should be done
separately from the actual coding stuff - that's why i thought your changes
were accidental (i didn't examine them closely indeed).
Honestly i'm not sure if this policy of separating style fixes and coding is
good. Surely it's better to have these separated, but this requires
additional time and work. I found this a little discouraging - style fixes
like that are best done 'by the way'.
> But thanks for taking the trouble
> to review my fix :)
I thought it's worth a try since we have too few reviewers. Too bad that i
didn't say anything useful :)
cheers,
Janek
Message from t.daniels@treda.co.uk
2011-05-10T07:27:14+00:00t.daniels_treda.co.ukurn:md5:e71fcdb93ed645e3a9dd199c9c8c583b
Janek WarchoĊ wrote Tuesday, May 10, 2011 6:11 AM
> I remember that someone once told me that such canges should be
> done
> separately from the actual coding stuff - that's why i thought
> your changes
> were accidental (i didn't examine them closely indeed).
> Honestly i'm not sure if this policy of separating style fixes and
> coding is
> good. Surely it's better to have these separated, but this
> requires
> additional time and work. I found this a little discouraging -
> style fixes
> like that are best done 'by the way'.
Style changes might be better separated, but the general advice
is to use an editor which removes trailing whitespace automatically
so all errors are corrected whenever a file is edited. If my editor
were
not set up this way I would not have noticed the whitespace errors
in the
original source and they would have persisted. Better to have them
fixed than not, I think, and fixing them whenever a file is edited
has been
the standard practice of several developers.
But I should have included a 'fix whitepace errors' in the commit
message.
My mistake. I'll do that before pushing.
Trevor
Message from n.puttock@gmail.com
2011-05-11T18:28:07+00:00Neil Puttockurn:md5:633345d8c2d4ce9233ef790fb7ed3029
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 should be correctly positioned close to
texidoc = "Octavate
http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode11
input/regression/clef-octavation.ly:11: \new Staff { \clef "G^8" g''1 }
indent
http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode18
input/regression/clef-octavation.ly:18: \layout { ragged-right = ##t }
remove
http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc
File lily/side-position-interface.cc (right):
http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc#newcode280
lily/side-position-interface.cc:280: Real diff = dir * staff_extent[dir] + staff_padding
add parentheses to enforce indent
Message from unknown
2011-05-12T16:12:01+00:00Trevor Danielsurn:md5:6d57bcb60bb5d294c202a49e63ef8b90
Message from t.daniels@treda.co.uk
2011-05-12T16:16:28+00:00t.daniels_treda.co.ukurn:md5:37945f7d9dd4fd8189f808d312f5a8d3
Neil wrote Wednesday, May 11, 2011 7:28 PM
> LGTM.
Thanks! I'll push after a couple of days if there are no more
comments.
> http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode3
> input/regression/clef-octavation.ly:3: \header{
> \header {
Done
> http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode5
> input/regression/clef-octavation.ly:5: texidoc=" Octavate symbols
> should
> be correctly positioned close to
> texidoc = "Octavate
Done
> http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode11
> input/regression/clef-octavation.ly:11: \new Staff { \clef "G^8"
> g''1 }
> indent
Done
> http://codereview.appspot.com/4489042/diff/1/input/regression/clef-octavation.ly#newcode18
> input/regression/clef-octavation.ly:18: \layout { ragged-right =
> ##t }
> remove
Done
> http://codereview.appspot.com/4489042/diff/1/lily/side-position-interface.cc#newcode280
> lily/side-position-interface.cc:280: Real diff = dir *
> staff_extent[dir]
> + staff_padding
> add parentheses to enforce indent
Done. Neat idea!
> http://codereview.appspot.com/4489042/
Trevor
Message from tdanielsmusic@googlemail.com
2011-05-16T09:21:54+00:00Trevor Danielsurn:md5:00076b416fbd9c9ecc138d64169a31de
Pushed patchset 2 on 16 May 2011:
e2a41749e09a68d2c8e453bff3c65404d6e50e34