Set indent based on instrument name

An updated version which takes the instrumentName length from the interval from the stencil.  This took me most of the day to work out :-(.  Please review the general approach.  I get the occasional fail on make test which I will look at tomorrow.

[PhilEHolmes, 2012-07-29 18:20:16]

My first attempt at coding in LilyPond.  Treat me gently...

[dak, 2012-07-29 20:16:55]

On 2012/07/29 18:20:16, PhilEHolmes wrote:
> My first attempt at coding in LilyPond.  Treat me gently...

I think we should be able to do better than just counting characters in a
string.  It should be possible calculating physical string width, shouldn't it?

[email_philholmes.net, 2012-07-29 20:46:28]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Sunday, July 29, 2012 9:16 PM
Subject: Re: Set indent based on instrument name (issue 6457049)


> On 2012/07/29 18:20:16, PhilEHolmes wrote:
>> My first attempt at coding in LilyPond.  Treat me gently...
>
> I think we should be able to do better than just counting characters in
> a string.  It should be possible calculating physical string width,
> shouldn't it?
>
> http://codereview.appspot.com/6457049/

Absolutely.  Count this as phase 1.  FWIW, this very simple approach works 
surprisingly well.

How would I calculate the string size?

--
Phil Holmes 

[PhilEHolmes, 2012-07-30 17:45:20]

Please review.

[Graham Percival, 2012-07-30 17:54:36]

Little nitpicks based on my C++ experience in other projects, with no knowledge
whatsoever of lilypond internals.

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...
lily/instrument-name-engraver.cc:111:
Instrument_name_engraver::Get_text_len_from_name (SCM scheme_text)
convention is to use lower-case names for class functions (or methods if you
prefer)

http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc
File lily/output-def.cc (right):

http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc#newcode38
lily/output-def.cc:38: Real long_name_len = 0.0;
could these be class member variables instead of global variables?

... hmm, more generally, I'm not sold on the whole set_inst_name_len approach. 
Is there any way you could just pass an additional argument to
line_dimenstions_int , and determine the data you need from that function?

[email_philholmes.net, 2012-07-30 21:14:41]

----- Original Message ----- 
From: <graham@percival-music.ca>
To: <PhilEHolmes@googlemail.com>; <dak@gnu.org>; <email@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Monday, July 30, 2012 6:54 PM
Subject: Re: Set indent based on instrument name (issue 6457049)


> Little nitpicks based on my C++ experience in other projects, with no
> knowledge whatsoever of lilypond internals.
>
>
>
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...
> lily/instrument-name-engraver.cc:111:
> Instrument_name_engraver::Get_text_len_from_name (SCM scheme_text)
> convention is to use lower-case names for class functions (or methods if
> you prefer)
>
> http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc
> File lily/output-def.cc (right):
>
> http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc#newcode38
> lily/output-def.cc:38: Real long_name_len = 0.0;
> could these be class member variables instead of global variables?

I don't believe so.  I'd be happy to be corrected by someone who understands 
this better than I do, but my understanding of c++ (which I guess at based 
on c#) says that, in order to access a class member variable, you need to 
have an instantiation of the class.  The problem we have is that the 
knowledge of the instrument name strings is in a different class 
(Instrument_name_engraver).  We need to persist the value of the longest 
instrument name, and pass it to output-def, where the indents are set.  If 
we instantiated an output-def class in Instrument_name_engraver, we would 
lose the instantiation when Instrument_name_engraver dies, so the values 
would not be persisted.  Hence why I ended up with global variables.  The 
one thing I thought about on the way home tonight is that these need to be 
zeroed when we create a new Score, so I need to find out/be told how to do 
that.

> ... hmm, more generally, I'm not sold on the whole set_inst_name_len
> approach.  Is there any way you could just pass an additional argument
> to line_dimenstions_int , and determine the data you need from that
> function?
>
> http://codereview.appspot.com/6457049/

Not AFAICS.  It currently requires 2 arguments: Output_def *def, int n, and 
there's no way we can satisfy them from Instrument_name_engraver.  I think 
adding an extra function to access the global variables is an effective 
encapsulation of the variable, and works for me.

--
Phil Holmes 

[bernard_marcade.biz, 2012-07-30 22:44:33]

On Mon, Jul 30, 2012 at 10:14:37PM +0100, Phil Holmes wrote:
> ----- Original Message ----- From: <graham@percival-music.ca>
>> lily/output-def.cc:38: Real long_name_len = 0.0;
>> could these be class member variables instead of global variables?
>
> I don't believe so.  I'd be happy to be corrected by someone who 
> understands this better than I do, but my understanding of c++ (which I 
> guess at based on c#) says that, in order to access a class member 
> variable, you need to have an instantiation of the class. 

In C++ variables can be declared static. If this is done all instances of
the class share the same instance of the variable and it can exist
even if the class has no instances see:

http://www.learncpp.com/cpp-tutorial/811-static-member-variables/

Bernard

[Graham Percival, 2012-07-30 23:12:49]

On Mon, Jul 30, 2012 at 11:44:28PM +0100, Bernard Hurley wrote:
> On Mon, Jul 30, 2012 at 10:14:37PM +0100, Phil Holmes wrote:
> > ----- Original Message ----- From: <graham@percival-music.ca>
> >> lily/output-def.cc:38: Real long_name_len = 0.0;
> >> could these be class member variables instead of global variables?
> >
> > I don't believe so.  I'd be happy to be corrected by someone who 
> > understands this better than I do, but my understanding of c++ (which I 
> > guess at based on c#) says that, in order to access a class member 
> > variable, you need to have an instantiation of the class. 

That is true.

> In C++ variables can be declared static. If this is done all instances of
> the class share the same instance of the variable and it can exist
> even if the class has no instances see:

Yes, that's also true.

Let me rephrase my concern: in C++-land, having a global variable
(including static variables) are viewed upon like picking one's
nose.  If there is absolutely no other way to achieve one's goal,
it could be tolerated.  But I would be very surprised if this
could not be implemented without global variables, so I think
another draft of the patch will be necessary.

Hopefully somebody familiar with lilypond internals can skim the
patch (it's quite small) and give a suggestion as to how to avoid
the global variables.

- Graham

[mail_philholmes.net, 2012-07-31 08:44:04]

----- Original Message ----- 
From: "Graham Percival" <graham@percival-music.ca>
To: "Bernard Hurley" <bernard@marcade.biz>
Cc: "Phil Holmes" <email@philholmes.net>; <dak@gnu.org>; 
<reply@codereview-hr.appspotmail.com>; <PhilEHolmes@googlemail.com>; 
<lilypond-devel@gnu.org>
Sent: Tuesday, July 31, 2012 12:12 AM
Subject: Re: Set indent based on instrument name (issue 6457049)


> On Mon, Jul 30, 2012 at 11:44:28PM +0100, Bernard Hurley wrote:
>> On Mon, Jul 30, 2012 at 10:14:37PM +0100, Phil Holmes wrote:
>> > ----- Original Message ----- From: <graham@percival-music.ca>
>> >> lily/output-def.cc:38: Real long_name_len = 0.0;
>> >> could these be class member variables instead of global variables?
>> >
>> > I don't believe so.  I'd be happy to be corrected by someone who
>> > understands this better than I do, but my understanding of c++ (which I
>> > guess at based on c#) says that, in order to access a class member
>> > variable, you need to have an instantiation of the class.
>
> That is true.
>
>> In C++ variables can be declared static. If this is done all instances of
>> the class share the same instance of the variable and it can exist
>> even if the class has no instances see:
>
> Yes, that's also true.
>
> Let me rephrase my concern: in C++-land, having a global variable
> (including static variables) are viewed upon like picking one's
> nose.  If there is absolutely no other way to achieve one's goal,
> it could be tolerated.  But I would be very surprised if this
> could not be implemented without global variables, so I think
> another draft of the patch will be necessary.
>
> Hopefully somebody familiar with lilypond internals can skim the
> patch (it's quite small) and give a suggestion as to how to avoid
> the global variables.

I'd be happy to change it if someone could suggest an improvement.  When I 
initially asked how this could be done, Keith said:

"Maybe each Instrument_name_engraver, one for each staff, could push
information to a central location"

which does sound rather like a global variable.

--
Phil Holmes 

[dak, 2012-07-31 10:59:36]

Graham Percival <graham@percival-music.ca> writes:

> On Mon, Jul 30, 2012 at 11:44:28PM +0100, Bernard Hurley wrote:
>> On Mon, Jul 30, 2012 at 10:14:37PM +0100, Phil Holmes wrote:
>> > ----- Original Message ----- From: <graham@percival-music.ca>
>> >> lily/output-def.cc:38: Real long_name_len = 0.0;
>> >> could these be class member variables instead of global variables?
>> >
>> > I don't believe so.  I'd be happy to be corrected by someone who 
>> > understands this better than I do, but my understanding of c++ (which I 
>> > guess at based on c#) says that, in order to access a class member 
>> > variable, you need to have an instantiation of the class. 
>
> That is true.
>
>> In C++ variables can be declared static. If this is done all instances of
>> the class share the same instance of the variable and it can exist
>> even if the class has no instances see:
>
> Yes, that's also true.
>
> Let me rephrase my concern: in C++-land, having a global variable
> (including static variables) are viewed upon like picking one's
> nose.

That's total nonsense with regard to static member variables.  There are
perfectly valid reasons to have variables per-class (which is what a
static member variable is) rather than per instance, for example for
class reflection (information about the class itself, like its name and
other stuff).  C++ uses the equivalent of static members for its virtual
function tables.

For information _flow_ bound to instances, one would not use global
variables.

It sounds like this would be the case here.

-- 
David Kastrup

[bernard_marcade.biz, 2012-07-31 12:45:50]

On Tue, Jul 31, 2012 at 12:59:16PM +0200, David Kastrup wrote:
> Graham Percival <graham@percival-music.ca> writes:
> 
> > On Mon, Jul 30, 2012 at 11:44:28PM +0100, Bernard Hurley wrote:
> >> On Mon, Jul 30, 2012 at 10:14:37PM +0100, Phil Holmes wrote:
> >> > ----- Original Message ----- From: <graham@percival-music.ca>
> >> >> lily/output-def.cc:38: Real long_name_len = 0.0;
> >> >> could these be class member variables instead of global variables?
> >> >
> >> > I don't believe so.  I'd be happy to be corrected by someone who 
> >> > understands this better than I do, but my understanding of c++ (which I 
> >> > guess at based on c#) says that, in order to access a class member 
> >> > variable, you need to have an instantiation of the class. 
> >
> > That is true.
> >
> >> In C++ variables can be declared static. If this is done all instances of
> >> the class share the same instance of the variable and it can exist
> >> even if the class has no instances see:
> >
> > Yes, that's also true.
> >
> > Let me rephrase my concern: in C++-land, having a global variable
> > (including static variables) are viewed upon like picking one's
> > nose.
> 
> That's total nonsense with regard to static member variables.  There are
> perfectly valid reasons to have variables per-class (which is what a
> static member variable is) rather than per instance, for example for
> class reflection (information about the class itself, like its name and
> other stuff).  C++ uses the equivalent of static members for its virtual
> function tables.
> 
> For information _flow_ bound to instances, one would not use global
> variables.
> 
> It sounds like this would be the case here.
>

Moreover a static member variable can be declared private when it is
only available to methods of the class. This deals with the usual
objections to global variables.

Bernard Hurley

[dak, 2012-07-31 13:36:30]

http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc
File lily/output-def.cc (right):

http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc#newcode275
lily/output-def.cc:275: set_inst_name_len (Real long_inst_name_len, Real
short_inst_name_len)
Correct me if I am wrong, but it does not seem like you initialize
long_name_len/short_name_len other than globally.  Meaning that the instrument
names of the whole LilyPond run (possibly over thousands of documentation
snippets) are relevant for the results at the end.

[mail_philholmes.net, 2012-07-31 14:04:23]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <graham@percival-music.ca>; 
<email@philholmes.net>; <bernard@marcade.biz>; <mail@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Tuesday, July 31, 2012 2:36 PM
Subject: Re: Set indent based on instrument name (issue 6457049)


>
> http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc
> File lily/output-def.cc (right):
>
> http://codereview.appspot.com/6457049/diff/4001/lily/output-def.cc#newcode275
> lily/output-def.cc:275: set_inst_name_len (Real long_inst_name_len, Real
> short_inst_name_len)
> Correct me if I am wrong, but it does not seem like you initialize
> long_name_len/short_name_len other than globally.  Meaning that the
> instrument names of the whole LilyPond run (possibly over thousands of
> documentation snippets) are relevant for the results at the end.
>
> http://codereview.appspot.com/6457049/
>

Correct.  I realised that last night, and this is why make test fails.  I 
sent an email identifying this as a problem, although I realise you can't 
possibly read every email.  The lengths need to be reset whenever there is a 
new score block.

--
Phil Holmes 

[MikeSol, 2012-08-01 06:45:22]

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...
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...
lily/instrument-name-engraver.cc:121: Real Text_len =
Text_int[MAX]-Text_int[MIN];
Real text_len = text_int.length ();

[Keith, 2012-08-04 07:28:29]

On 2012/08/01 06:45:22, MikeSol wrote:

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

This is a case where measuring extents early seems appropriate.

Phil wants the line-indents, thus line-lengths, thus some very early layout
decisions, to depend on the extents of the instrument names.

The dependency is reversed in this case from the more usual situations where
Mike's suggestion holds, such as the extent of a hairpin, which depends on the
layout.

> Here, what I would do is use the Pointer_group_interface to add two grob
arrays

Running through those arrays and re-checking the lengths of the instrument names
every time we look up the line-indent, would let the line-breaker see if those
lengths changed for some reason depending on the evolving layout.  I do not
expect the line-breaker to behave sensibly if the indents change like this.

> Properties, in LilyPond, are the best way to stash
> information.  

Phil, working with properties, or in this case Scheme variables associated with
layout, does seems necessary.  See set_variable().
Old scores where we chose \paper {indent = 2.3\cm } to fit the instrument names
should continue to work.  
If you can figure out enough Scheme to have LilyPond initialize indent so that
it evaluates a different Scheme variable (say, default-indent) and have the
engraver updates that variable, then we can have automatically-adapting intends
by default, and let users override by replacing the link to default-indent with
their own choice.

[dak, 2012-08-04 07:37:21]

On 2012/08/04 07:28:29, Keith wrote:
> On 2012/08/01 06:45:22, MikeSol wrote:
> 
> > 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.
> > 
> 
> This is a case where measuring extents early seems appropriate.

I suppose this would spell the death knell to solving a somewhat different
problem in the way linked to in comment 5 of issue 766
<URL:http://code.google.com/p/lilypond/issues/detail?id=766#c5>.

[Keith, 2012-08-04 08:04:18]

On Sat, 04 Aug 2012 00:37:21 -0700, <dak@gnu.org> wrote:

> On 2012/08/04 07:28:29, Keith wrote:
>> On 2012/08/01 06:45:22, MikeSol wrote:
>
>> > 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.
>> >
>
>> This is a case where measuring extents early seems appropriate.
>
> I suppose this would spell the death knell to solving a somewhat
> different problem in the way linked to in comment 5 of issue 766
> <URL:http://code.google.com/p/lilypond/issues/detail?id=766#c5>.
>

I do not hear any death knell.
Nicolas' function makes the instrument name fill the space available, based on
'indent'.  If the engraver evaluates the extents of instrument names with this
function in place, the function can still look up 'indent' with no cycle of
call-backs or any such problem.

After line breaking, when the actual stencils are formed for each line, Nicolas'
function will be called again for each actual printing of an instrument name. 
These calls will use the possibly-changed value of 'indent', but the same value
for each staff.

[mike7, 2012-08-04 15:59:19]

On 4 août 2012, at 09:28, k-ohara5a5a@oco.net wrote:

> On 2012/08/01 06:45:22, MikeSol wrote:
> 
>> 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.
> 
> 
> This is a case where measuring extents early seems appropriate.
> 
> Phil wants the line-indents, thus line-lengths, thus some very early
> layout decisions, to depend on the extents of the instrument names.
> 
> The dependency is reversed in this case from the more usual situations
> where Mike's suggestion holds, such as the extent of a hairpin, which
> depends on the layout.
> 

I agree that the calculation will happen early (definitely before line
breaking), but if someone writes an exotic callback for the stencil function of
instrument name that somehow is linked to other grobs, it should only be called
after engraving is done.

>> Here, what I would do is use the Pointer_group_interface to add two
> grob arrays
> 
> Running through those arrays and re-checking the lengths of the
> instrument names every time we look up the line-indent, would let the
> line-breaker see if those lengths changed for some reason depending on
> the evolving layout.  I do not expect the line-breaker to behave
> sensibly if the indents change like this.

I didn't express myself clearly.  I meant that there should be a finalize method
in the engraver that loops through the vectors and adds them to a grob array via
Pointer_group_interface::add_grob.  finalize is only ever called once for a
given engraver.

> 
>> Properties, in LilyPond, are the best way to stash
>> information.
> 
> Phil, working with properties, or in this case Scheme variables
> associated with layout, does seems necessary.  See set_variable().
> Old scores where we chose \paper {indent = 2.3\cm } to fit the
> instrument names should continue to work.
> If you can figure out enough Scheme to have LilyPond initialize indent
> so that it evaluates a different Scheme variable (say, default-indent)
> and have the engraver updates that variable, then we can have
> automatically-adapting intends by default, and let users override by
> replacing the link to default-indent with their own choice.
> 
> http://codereview.appspot.com/6457049/

I agree that users should be able to override something, but they should be able
to override a property of InstrumentName or System (depending on how things are
implemented).
I'm always wary of Scheme variables because of global scopes and multiple
scores: that has come to bite me in the past.  I generally steer clear of this
approach and try to make things grob-centric (or if necessary prob-centric). 
There are some things that fall outside the scope of grob-related issues (like
the paper bloc) but I think it's best to try to use grobs, callbacks, and
properties as much as possible.  I'm definitely not saying that your solution
won't work, but I think that going down the grob road is more architecturally
sound.  Of course, if Phil has time, he can investigate the plausibility of both
:-)

Cheers,
MS