[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Allows user to set ChordName text (issue 6496085)
From: |
dak |
Subject: |
Re: Allows user to set ChordName text (issue 6496085) |
Date: |
Wed, 12 Sep 2012 11:30:28 +0000 |
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/