Re: Set indent based on instrument name (issue 6457049)

[mtsolo]

Hey Phil!

First and foremost, congrats on this work.  I'm thrilled to see you
venturing into the C++ side.  You're tackling an issue that, while just
a few lines of code, uses a lot of advanced LilyPond structures, so it's
not easy.  My suggestions don't have to do with the math (all of it is
fine) but rather a way to restructure your work so that it fits better
into how LilyPond code works.

At the engraver level, it is generally not a good idea to touch layout
information.  When this happens, it is usually to kill grobs or set
properties.  Avoid measuring extents when engraving is happening because
they could be dependent on other callbacks which could trigger many
layout decisions before engraving is finished.

Here, what I would do is use the Pointer_group_interface to add two grob
arrays to the root system of instrument names - one that contains all
InstrumentName grobs with instrumentName and one that contains all
InstrumentName grobs with shortInstrumentName.  You can stash these in
two separate vectors and then loop through the vectors, invoking
Pointer_group_interface::add_grob in a finalize method for the engraver.
 Then, create two properties of the system grob called
instrument-name-length and short-instrument-name-length and two
callbacks that look up the appropriate grob array and do the X-extent
calculations you do in the engraver.  To avoid code-duping, they can
likely use an internal function for most of their work.  Properties, in
LilyPond, are the best way to stash information.  You almost never want
to create global variables or class member variables to do this.

Then, in paper-score.cc, create functions
  Real Paper_score::get_instrument_name_length ()
and
  Real Paper_score::get_short_instrument_name_length ()
These function will check to see if there is one or many systems and, if
so, glean the instrument name length or short instrument name length
from that.  Otherwise, it will return 0.

Then, for line_dimensions_int (Output_def *def, int n), make two extra
arguments where you pass in the results of these functions.  These will
replace the global variables you're creating in output-def.cc.  It works
because line_dimensions_int is only ever called when a Paper_score is
available, so you can always interrogate the Paper_score for the
appropriate indent values before making the function call.

These changes will make the code more maintainable and less prone to
kick up bugs in other areas.  Let me know if you have any questions!

Cheers,
MS


http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc
File lily/instrument-name-engraver.cc (right):

http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc#newcode116
lily/instrument-name-engraver.cc:116: SCM Text_scheme =
Text_interface::interpret_markup (layout->self_scm (),
Ditto for variable names - use lowercase.

http://codereview.appspot.com/6457049/diff/4001/lily/instrument-name-engraver.cc#newcode121
lily/instrument-name-engraver.cc:121: Real Text_len =
Text_int[MAX]-Text_int[MIN];
Real text_len = text_int.length ();

http://codereview.appspot.com/6457049/