Re: Add N.C. entry to ChordNames context.

[Carl D. Sorensen]

Neil,

Thanks for the review.  I've put my responses below.


On 5/10/09 5:17 PM, "address@hidden" <address@hidden> wrote:

> http://codereview.appspot.com/62076/diff/7/1008#newcode65
> Line 65: SCM no_chord_markup = get_property ("noChordSymbol");
> \set noChordSymbol = ##f will cause (harmless but annoying) errors here.
> You could check whether the markup's valid using
> Text_interface::is_markup ().

Thanks for the pointer.  I added text-interface.hh to the includes in order
to be able to call this.

> 
> http://codereview.appspot.com/62076/diff/7/1008#newcode70
> Line 70: chord_name_->set_property ("begin-of-line-visible",
> SCM_BOOL_T);
> If you just retrieved the markup instead and used the existing code
> below, you wouldn't need all this duplication.

I think I need to create chord_name_ in two different places, because in one
place the event is notes_[0] (if I have a chord), and in the other it's
rest_event_ (if I have a rest).  But all the other stuff can avoid
duplication, I think.
> 
> http://codereview.appspot.com/62076/diff/7/1008#newcode72
> Line 72: rest_event_ = 0;
> You don't need this, since you're clearing it in
> stop_translation_timestep ().
> 
> http://codereview.appspot.com/62076/diff/7/1008#newcode73
> Line 73: return;
> You'd normally only return from process_music () if there's an error, so
> it would make sense to check for rests after checking for noteheads.
> This would allow you to use the same code below to create the grob, set
> the markup and calculate last_chord_.

I've modified the code so that now I have

if (rest_event_)
   {
      create chord_name_ with rest_event_
   }
else 
  {
      create chord_name_ with notes_[0]
   }

set markup
store last_chord_
set begin-of-line-visible


As you mentioned, this eliminated duplication.  Thanks for the pointers.

With these changes, I've put a new patch on rietveld.  I'd appreciate it if
you'd see if I responded properly to your comments.

Thanks,

Carl



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