[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: |
address@hidden |
Subject: |
Re: Allows user to set ChordName text (issue 6496085) |
Date: |
Wed, 12 Sep 2012 14:48:37 +0300 |
> 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.
>
I think I misunderstood your comment - could you show what you mean by
proposing an alternative (in pseudo-code if you'd like) to the code that's in
there?
> 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.
I'm not sure what you mean - every time these are calculated, these are used on
line 139. Unless I am reading the code incorrectly.
>
> 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?
>
They're no longer used as per your suggestion to compare text instead of these
values.
> http://codereview.appspot.com/6496085/
- Allows user to set ChordName text (issue 6496085), dak, 2012/09/06
- Re: Allows user to set ChordName text (issue 6496085), mtsolo, 2012/09/06
- Re: Allows user to set ChordName text (issue 6496085), dak, 2012/09/12
- Re: Allows user to set ChordName text (issue 6496085), dak, 2012/09/12
- Re: Allows user to set ChordName text (issue 6496085),
address@hidden <=
- Re: Allows user to set ChordName text (issue 6496085), dak, 2012/09/12
- Re: Allows user to set ChordName text (issue 6496085), dak, 2012/09/12