Re: Context mods stored in variable, can be inserted into \with or \context (issue475041)

[reinhold . kainhofer]

Reviewers: Neil Puttock,


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.

Description:
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

Please review this at http://codereview.appspot.com/475041/show

Affected files:
  A input/regression/context-mod-context.ly
  A input/regression/context-mod-with.ly
  M input/regression/hara-kiri-drumstaff.ly
  A input/regression/hara-kiri-keep-previous-settings.ly
  M input/regression/hara-kiri-percent-repeat.ly
  M input/regression/hara-kiri-pianostaff.ly
  M input/regression/hara-kiri-rhythmicstaff.ly
  M input/regression/hara-kiri-tabstaff.ly
  A lily/context-mod.cc
  A lily/include/context-mod.hh
  M lily/lily-lexer.cc
  M lily/parser.yy
  M ly/engraver-init.ly