Neil, Thanks for the review. I've put my responses below.
On 5/10/09 5:17 PM, "n.putt...@gmail.com" <n.putt...@gmail.com> 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 > > > _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel