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

[dak]

On 2012/09/12 11:48:45, mike7 wrote:
> >

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

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

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

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

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