remove bar-size and replace its usage by bar-extent

remove bar-size and replace its usage by bar-extent

[benko.pal, 2011-01-18 21:32:15]

patchset related to
http://code.google.com/p/lilypond/issues/detail?id=1268
as outlined in
http://lists.gnu.org/archive/html/lilypond-devel/2011-01/msg00225.html

note that this is the first time I touch Python (for sure) and the documentation
(possibly).

[Graham Percival (old account), 2011-01-21 13:50:22]

The python bits looked fine.  I tried a regtest comparison and didn't see any
change at all other than test-output-distance, so maybe something went wrong. 
Should I see any difference in the graphical output?

I think there's some debugging code in here that needs to be cleaned up.

http://codereview.appspot.com/4025044/diff/1/lily/bar-line.cc
File lily/bar-line.cc (right):

http://codereview.appspot.com/4025044/diff/1/lily/bar-line.cc#newcode30
lily/bar-line.cc:30: #include <iostream>
Why iostream?  We have our own functions for printing, which (I think) makes it
easier to translate the text.

http://codereview.appspot.com/4025044/diff/1/lily/bar-line.cc#newcode342
lily/bar-line.cc:342: std::clog << __PRETTY_FUNCTION__ << ": (" << extent.at
(DOWN) << " . "
I'm suspicious about printing text this way.

http://codereview.appspot.com/4025044/diff/1/lily/bar-line.cc#newcode367
lily/bar-line.cc:367: 
Ditto.

[benko.pal, 2011-01-21 21:51:48]

new patch set up, thanks for reviewing.  answers below.

On 2011/01/21 13:50:22, Graham Percival wrote:
> The python bits looked fine.  I tried a regtest comparison and didn't see any
> change at all other than test-output-distance, so maybe something went wrong. 
> Should I see any difference in the graphical output?

no; I added a new file to the test suite; perhaps it's too big.

> I think there's some debugging code in here that needs to be cleaned up.

Indeed; sorry for having left those in.

[Graham Percival (old account), 2011-01-23 15:00:28]

Passes regtests, but a few minor nitpicks.

http://codereview.appspot.com/4025044/diff/10001/input/regression/bar-extent.ly
File input/regression/bar-extent.ly (right):

http://codereview.appspot.com/4025044/diff/10001/input/regression/bar-extent....
input/regression/bar-extent.ly:1: \version "2.12.3"
I think this should be 2.13.47 ?

http://codereview.appspot.com/4025044/diff/10001/input/regression/bar-extent....
input/regression/bar-extent.ly:2: 
Sorry for missing this earlier: we need a \header { \texidoc=""}} thing.  Take a
look at any of the other regtests.  :)

http://codereview.appspot.com/4025044/diff/10001/input/regression/drums.ly
File input/regression/drums.ly (right):

http://codereview.appspot.com/4025044/diff/10001/input/regression/drums.ly#ne...
input/regression/drums.ly:10: \version "2.13.46"
2.13.47

http://codereview.appspot.com/4025044/diff/10001/input/regression/grid-lines.ly
File input/regression/grid-lines.ly (right):

http://codereview.appspot.com/4025044/diff/10001/input/regression/grid-lines....
input/regression/grid-lines.ly:8: \version "2.13.46"
2.13.47

http://codereview.appspot.com/4025044/diff/10001/input/regression/lyrics-bar.ly
File input/regression/lyrics-bar.ly (right):

http://codereview.appspot.com/4025044/diff/10001/input/regression/lyrics-bar....
input/regression/lyrics-bar.ly:1: \version "2.13.46"
2.13.47

http://codereview.appspot.com/4025044/diff/10001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/4025044/diff/10001/ly/engraver-init.ly#newcode19
ly/engraver-init.ly:19: \version "2.13.46"
2.13.47

[benko.pal, 2011-01-23 15:32:07]

On 2011/01/23 15:00:28, Graham Percival wrote:
> Passes regtests, but a few minor nitpicks.

thanks, Graham; all observations implemented.

[Graham Percival (old account), 2011-01-23 15:59:32]

Looks good to me!

NB: I'm not vouching for the quality of the scheme or C++ code; that's outside
of my expertise.  But the .ly files look fine, and the results (in the form of
the regtest comparison) look fine.

[Carl, 2011-01-24 00:00:35]

Looks good to me.

[pkx166h, 2011-01-26 14:58:44]

Just a nit-pick.

Why is the french version of the chords.itely in this patch and none of the
other languanges?

James

[benko.pal, 2011-01-26 15:02:39]

> Why is the french version of the chords.itely in this patch and none of
> the other languanges?

I grepped for bar-size, was surprised too, but didn't investigate further.
I'll do it again before sending the patches (and if I find anything new,
I'll upload it to Rietveld).

p

[Graham Percival, 2011-01-26 17:41:59]

On Wed, Jan 26, 2011 at 02:58:44PM +0000, pkx166h@gmail.com wrote:
> Why is the french version of the chords.itely in this patch and none of
> the other languanges?

If the other languages didn't translate that part of chords.itely,
or if the translations are out of date, then they wouldn't have
"bar-size" in them.

Cheers,
- Graham

[Neil Puttock, 2011-01-27 23:46:31]

Hi Pál,

LGTM apart from the change in behaviour for dashed barlines: they're
overshooting the outer lines.  I couldn't see this from the regtest comparison
for bar-line-dashed.ly, but it's clearly visible on paper.

Cheers,
Neil

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc
File lily/bar-line.cc (left):

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc#oldcode135
lily/bar-line.cc:135: {
remove { }

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc#oldcode140
lily/bar-line.cc:140: {
remove { }

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc
File lily/bar-line.cc (right):

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc#newcode37
lily/bar-line.cc:37: {
remove { }

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc#newcode72
lily/bar-line.cc:72: if (ex.length() > 0)
length ()

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc#newcode113
lily/bar-line.cc:113: Real const h = extent.length();
length ()

http://codereview.appspot.com/4025044/diff/20001/lily/bar-line.cc#newcode325
lily/bar-line.cc:325: Real const h = extent.length();
length ()

http://codereview.appspot.com/4025044/diff/20001/lily/span-bar.cc
File lily/span-bar.cc (right):

http://codereview.appspot.com/4025044/diff/20001/lily/span-bar.cc#newcode80
lily/span-bar.cc:80: {
remove { }

[benko.pal, 2011-01-28 22:07:04]

all suggestions implemented, thanks all!

[hanwenn, 2011-01-31 11:16:20]

LGTM