Allows user to set ChordName text

Allows user to set ChordName text

[dak, 2012-09-06 08:50:40]

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#newc...
lily/chord-name-engraver.cc:83: || ly_is_procedure (chord_name_->get_property
("text")));
If it is a procedure, shouldn't it be called with the calculated value?

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
lily/chord-name-engraver.cc:87: {
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.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
lily/chord-name-engraver.cc:136: markup = scm_call_4 (name_proc, pitches, bass,
inversion,
You do all the calculation and then throw it away?  Where is the point?

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
lily/chord-name-engraver.cc:149: && ly_is_equal (chord_as_scm, last_chord_))
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.

[MikeSol, 2012-09-06 09:59:09]

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#newc...
lily/chord-name-engraver.cc:83: || ly_is_procedure (chord_name_->get_property
("text")));
On 2012/09/06 08:50:40, dak wrote:
> If it is a procedure, shouldn't it be called with the calculated value?

Right you are - I should have used get_property_data.  Will fix.

http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
lily/chord-name-engraver.cc:149: && ly_is_equal (chord_as_scm, last_chord_))
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.  As for the present point, that is an interesting
conundrum...I'll  drum up some logic for that.  That may eliminate the need for
guarding the values above having to do with chord changes, in which point the if
else statement will be able to be simplified.

[dak, 2012-09-12 11:05:32]

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#newc...
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.  As for the present point, that is an interesting
> conundrum...I'll  drum up some logic for that.  That may eliminate the need
for
> guarding the values above having to do with chord changes, in which point the
if
> else statement will be able to be simplified.

Is there a reason you did none of the above, addressed only the very first point
of my review, ignored all of the rest and pushed?

[mike7, 2012-09-12 11:14:36]

On 12 sept. 2012, at 14:05, dak@gnu.org wrote:

> 
> 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#newc...
> 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.  As for the present point, that is an interesting
>> conundrum...I'll  drum up some logic for that.  That may eliminate the
> need for
>> guarding the values above having to do with chord changes, in which
> point the if
>> else statement will be able to be simplified.
> 
> Is there a reason you did none of the above, addressed only the very
> first point of my review, ignored all of the rest and pushed?
> 

There are four points in your previous review and I addressed all of them in the
code before I pushed to staging.  If it's unclear how any of them are addressed,
please make reference to a specific one and I can put a comment in the code to
clear things up.

Cheers,
MS

[dak, 2012-09-12 11:30:28]

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#newc...
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#newc...
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#newc...
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?

[mike7, 2012-09-12 11:48:45]

>
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
> 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#newc...
> 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#newc...
> 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/

[dak, 2012-09-12 16:11:05]

On 2012/09/12 11:48:45, mike7 wrote:
> >
>
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
> > 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

I have a hard time finding an interpretation where doing nothing is
the proper remedy.

> - could you show what you mean by proposing
> an alternative (in pseudo-code if you'd like) to the code that's in
> there?

I did it in pseudo-code above, and you ignored it.  So let's do it in
straight code.

The diff to the original has the form

- if (rest_event_)
+ if (rest_event_ && !make_markup) { }
+ else if (rest_event_)
    {
       SCM no_chord_markup = get_property ("noChordSymbol");
       if (!Text_interface::is_markup (no_chord_markup))
...
    } else

when it should rather have the form

  if (rest_event_)
    {
-       SCM no_chord_markup = get_property ("noChordSymbol");
-       if (!Text_interface::is_markup (no_chord_markup))
...
+     if (make_markup)
+       {
+         SCM no_chord_markup = get_property ("noChordSymbol");
+         if (!Text_interface::is_markup (no_chord_markup))
...
+       }
    } else



>
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
> > 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.

Only if make_markup is set.  Otherwise you calculate them and throw
them away.

Why?  Why do all the work in the case where make_markup is not even
set?  Why not skip all that much earlier when make_markup is not set?

>
http://codereview.appspot.com/6496085/diff/1/lily/chord-name-engraver.cc#newc...
> > 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.

And is there a reason the logic of the code is not changed to reflect
the changes of the code?

[dak, 2012-09-12 16:16:56]

On 2012/09/12 16:11:05, dak wrote:

> when it should rather have the form
> 
>   if (rest_event_)
>     {
> -       SCM no_chord_markup = get_property ("noChordSymbol");
> -       if (!Text_interface::is_markup (no_chord_markup))
> ...
> +     if (make_markup)
> +       {
> +         SCM no_chord_markup = get_property ("noChordSymbol");
> +         if (!Text_interface::is_markup (no_chord_markup))
> ...
> +       }
>     } else

Actually, come to think of it you might want to move
if (make_markup)
upwards to surround the whole if (rest_event_), and then you will
arrive at the situation where a markup (and all information required
for it) is only calculated when it will get used, addressing the next
point as well.

That was the intent of the original suggestion.

[mike7, 2012-09-12 17:55:23]

On 12 sept. 2012, at 19:16, dak@gnu.org wrote:

> On 2012/09/12 16:11:05, dak wrote:
> 
>> when it should rather have the form
> 
>>   if (rest_event_)
>>     {
>> -       SCM no_chord_markup = get_property ("noChordSymbol");
>> -       if (!Text_interface::is_markup (no_chord_markup))
>> ...
>> +     if (make_markup)
>> +       {
>> +         SCM no_chord_markup = get_property ("noChordSymbol");
>> +         if (!Text_interface::is_markup (no_chord_markup))
>> ...
>> +       }
>>     } else
> 
> Actually, come to think of it you might want to move
> if (make_markup)
> upwards to surround the whole if (rest_event_), and then you will
> arrive at the situation where a markup (and all information required
> for it) is only calculated when it will get used, addressing the next
> point as well.
> 
> That was the intent of the original suggestion.
> 

Excellent - this is the exact type of feedback I needed.  Many thanks.

Cheers,
MS

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