Context mods stored in variable, can be inserted into \with or \context

Context mods stored in variable, can be inserted into \with or \context

-) Context-Modifications: create C++ class to store them
-) context modifications lists are stored in a dedicated simple scheme object
     (C++ class Context_mod)
-) Changes to the parser:
  -) context_modifications objects (stored in variables) are now also allowed
     with \with clauses
  -) context_modifications objects are also allowed inside \context

-) this allows us to rewrite \RemoveEmptyStaffContext (unfortunately with
   a little different syntax, since we no longer store \Staff inside the
	 \RESC command) so that it no longer erases previous settings to the
	 Staff context. Now, instead of
        \context { \RemoveEmptyStaffContext }
	 one can do
        \context { \Staff \RemoveEmptyStaves }
	 with the same effect and preserve previous changes to the Staff context.
	 (The same applies of course to \DrumStaff, \RhythmicStaff, etc. as well)

-) Adjusted engraver-init.ly and the regtests accordingly; Also added regtest
   that checks for RESC not discarding previous settings to the Staff context

[Neil Puttock, 2010-03-13 23:24:24]

Hi Reinhold,

LGTM.

You need to check your indentation in parser.yy, since you're adding spaces
instead of hard tabs.

Cheers,
Neil

http://codereview.appspot.com/475041/diff/1/5
File input/regression/hara-kiri-keep-previous-settings.ly (right):

http://codereview.appspot.com/475041/diff/1/5#newcode33
input/regression/hara-kiri-keep-previous-settings.ly:33: % New
\RemoveEmptyStaves settings: Will erase previous settings...
Preserves previous settings

http://codereview.appspot.com/475041/diff/1/10
File lily/context-mod.cc (right):

http://codereview.appspot.com/475041/diff/1/10#newcode69
lily/context-mod.cc:69: add_context_mod (scm_car (m));
indent

http://codereview.appspot.com/475041/diff/1/10#newcode73
lily/context-mod.cc:73: Context_mod::to_alist () const
I'd name this something like get_mods (), otherwise it implies you're building
an alist to return (which obviously isn't the case)

http://codereview.appspot.com/475041/diff/1/11
File lily/include/context-mod.hh (right):

http://codereview.appspot.com/475041/diff/1/11#newcode22
lily/include/context-mod.hh:22: * Implement a structore to store context
modifications to be inserted
structure

http://codereview.appspot.com/475041/diff/1/11#newcode30
lily/include/context-mod.hh:30: // #include "lily-proto.hh"
Add prototype for Context_mod to lily-proto.hh ?

http://codereview.appspot.com/475041/diff/1/11#newcode49
lily/include/context-mod.hh:49: VIRTUAL_COPY_CONSTRUCTOR(Context_mod,
Context_mod);
VIRTUAL_COPY_CONSTRUCTOR (

http://codereview.appspot.com/475041/diff/1/12
File lily/lily-lexer.cc (right):

http://codereview.appspot.com/475041/diff/1/12#newcode52
lily/lily-lexer.cc:52: {"contextModifications", CONTEXT_MOD},
contextmodifications (all the parser keywords are lower case)

[Reinhold, 2010-03-14 00:17:46]

http://codereview.appspot.com/475041/diff/1/10
File lily/context-mod.cc (right):

http://codereview.appspot.com/475041/diff/1/10#newcode73
lily/context-mod.cc:73: Context_mod::to_alist () const
On 2010/03/13 23:24:24, Neil Puttock wrote:
> I'd name this something like get_mods (), otherwise it implies you're building
> an alist to return (which obviously isn't the case)

Yes, of course. I simply copied Context_def (where we really return an alist)
and adjusted the code, but overlooked this little detail...

http://codereview.appspot.com/475041/diff/1/10#newcode75
lily/context-mod.cc:75: return mods_;
reverse is missing here (each new mod is prepended!). At first look it might not
make a difference, but it does if a block contains two conflicting settings.
Inside \context, the last one is applied, if we don't reverse mods_ here, the
first takes precedence!

In particular, if we have
    \remove "Clef_engraver"
    \consists "Clef_engraver"
inside a \context {\Staff ...} block, the staff will contain the clef engraver,
while inside a \with block, it would be removed...

http://codereview.appspot.com/475041/diff/1/11
File lily/include/context-mod.hh (right):

http://codereview.appspot.com/475041/diff/1/11#newcode30
lily/include/context-mod.hh:30: // #include "lily-proto.hh"
On 2010/03/13 23:24:24, Neil Puttock wrote:
> Add prototype for Context_mod to lily-proto.hh ?

Hmm, you are right, that might be a good idea (just in case some class later on
will work with pointers to Context_mod...). Should I then include lily-proto.hh,
even if technically not required?

http://codereview.appspot.com/475041/diff/1/11#newcode49
lily/include/context-mod.hh:49: VIRTUAL_COPY_CONSTRUCTOR(Context_mod,
Context_mod);
On 2010/03/13 23:24:24, Neil Puttock wrote:
> VIRTUAL_COPY_CONSTRUCTOR (

Hehe, that's copied verbatim from Context_def...
(BTW, a grep shows 157 lines with missing spaces just in lily/*.cc, and 203
lines with missing spaces in lily/include/*.hh -- Shall we fix them all in one
go?)

http://codereview.appspot.com/475041/diff/1/12
File lily/lily-lexer.cc (right):

http://codereview.appspot.com/475041/diff/1/12#newcode52
lily/lily-lexer.cc:52: {"contextModifications", CONTEXT_MOD},
On 2010/03/13 23:24:24, Neil Puttock wrote:
> contextmodifications (all the parser keywords are lower case)

That keywords looks absolutely awkward. Maybe we should revisit that
convention... The alternative would be to not introduce a new parser keyword at
all and simply use \with instead (although that does not describe what the
following block is supposed to do when assigned to a variable, i.e. "mymods =
\with { \consists.... }"

http://codereview.appspot.com/475041/diff/1/13
File lily/parser.yy (right):

http://codereview.appspot.com/475041/diff/1/13#newcode1069
lily/parser.yy:1069: context_mod_list:
Unfortunately something here breaks \consists Some_engraver (without quotes!).
The parser now tries to evaluate Some as a variable rather than interpret
"Some_engraver" as a string...

http://codereview.appspot.com/475041/diff/1/14
File ly/engraver-init.ly (right):

http://codereview.appspot.com/475041/diff/1/14#newcode466
ly/engraver-init.ly:466: }
Shall we add a convert-ly rule for \RemoveEmptyStaffContext ->
\Staff\RemoveEmptyStaves ? Of course, a simplar rule for the other
RemoveEmpty*StaffContext's is needed, too.