Re: Allows user to set ChordName text (issue 6496085)

[dak]

Yes, I did not check you actually addressed the points of the review
before the countdown ended.  But I see my role as a reviewer, not as a
surveillance team.


http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode87
lily/chord-name-engraver.cc:87: {
On 2012/09/06 08:50:40, dak wrote:
> What kind of contorted logic and guessing game is that?
> if (make_markup)
> {
>     [old code ending in setting "text"]
> }

> Please don't obfuscate code just to save reindentation.

This comment has not been addressed.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode136
lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc,
pitches, bass, inversion,
On 2012/09/06 08:50:40, dak wrote:
> You do all the calculation and then throw it away?  Where is the
point?

This comment has not been addressed.  If neither rest_event nor
make_markup are set, you calculate pitches, bass, inversion and throw
them away.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newcode149
lily/chord-name-engraver.cc:149: && ly_is_equal (chord_as_scm,
last_chord_))
On 2012/09/06 09:59:09, MikeSol wrote:
> On 2012/09/06 08:50:40, dak wrote:
> > If one is doing the chord calculation manually, you can't make the
decision of
> > whether a chord changed based on the automatic calculation.  For
better or
> > worse, you need to compare the computed chord versions/text.

> To respond to your points above, I don't throw away the values above
because
> they're used here.

I don't see where the current code version would use them.  Can you
point that out to me?

http://codereview.appspot.com/6496085/