Re: Fixing issue 37 with extra position callback (issue3928041)

[address@hidden]

Carl,

Thanks for the comments!
I've integrated all but one of them - when you say "Also add to TabStaff," what 
do you mean?  I was under the impression that TabStaff would inherit this 
engraver because it inherits from Staff.

Cheers,
MS

On Jan 8, 2011, at 1:38 PM, address@hidden wrote:

> Reviewers: MikeSol,
> 
> Message:
> Looks great, Mike!
> 
> You have some code indentation issues that don't match our style.  Other
> than that, I think it's good to go.
> 
> Of course, we do need a regression test as well.
> 
> Thanks,
> 
> Carl
> 
> 
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc
> File lily/beam-collision-engraver.cc (right):
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode4
> lily/beam-collision-engraver.cc:4: Copyright (C) 1997--2010 Han-Wen
> Nienhuys <address@hidden>
> Copyright should be 2011 Mike Solomon.
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode48
> lily/beam-collision-engraver.cc:48: {
> brace not needed
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode55
> lily/beam-collision-engraver.cc:55: {
> remove {
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode57
> lily/beam-collision-engraver.cc:57: {
> remove {
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam-collision-engraver.cc#newcode83
> lily/beam-collision-engraver.cc:83: {
> remove {
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc
> File lily/beam.cc (right):
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode944
> lily/beam.cc:944: {
> indent { 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode962
> lily/beam.cc:962: {
> indent 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode967
> lily/beam.cc:967: magic = -2.0;
> { on separate line, indented 2 spaces
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode972
> lily/beam.cc:972: {
> indent
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode992
> lily/beam.cc:992: {
> indentation
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode997
> lily/beam.cc:997: } else {
> indentation
> 
> http://codereview.appspot.com/3928041/diff/1/lily/beam.cc#newcode1008
> lily/beam.cc:1008: } else
> else on its own line, same indentation as if
> 
> http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly
> File ly/engraver-init.ly (right):
> 
> http://codereview.appspot.com/3928041/diff/1/ly/engraver-init.ly#newcode68
> ly/engraver-init.ly:68: \consists "Beam_collision_engraver"
> Also add to TabStaff
> 
> Description:
> Fixing issue 37 with extra position callback
> Adds beam collision engraver
> 
> Goodbye whitespace
> 
> Please review this at http://codereview.appspot.com/3928041/
> 
> Affected files:
> A lily/beam-collision-engraver.cc
> M lily/beam.cc
> M lily/include/beam.hh
> M ly/engraver-init.ly
> M scm/define-grobs.scm
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/lilypond-devel