Re: Create a two-argument form of define-event-class (issue 10965043)

[dak]

Reviewers: thomasmorley651,


https://codereview.appspot.com/10965043/diff/1/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

https://codereview.appspot.com/10965043/diff/1/scm/define-event-classes.scm#newcode87
scm/define-event-classes.scm:87: (define ancestor-lookup
(make-hash-table (length all-event-classes)))
AOn 2013/07/05 23:20:48, thomasmorley651 wrote:
> I'm not convinced about the naming.

> 'ancestor' is defined in titling-init.ly, ofcourse with different
functionality.
> 'ancestor-lookup' is a _very_ generic naming, and it's usage _is_
generic in a
> certain way.
> Though, I'd prefer something like 'ancestor-all-event-classes-lookup'.
> No, that's to long.
> But you get the point.

Actually, I don't get the point.  This is not a public variable, so
there is not much chance for collision.  It's local to the lily module,
and come Guilev2, it will be local to this file.

https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm
File scm/document-music.scm (right):

https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode33
scm/document-music.scm:33: (let* ((class (ly:camel-case->lisp-identifier
(car entry)))
On 2013/07/05 23:20:48, thomasmorley651 wrote:
> Apart from using tabs I'm not able to see any difference to the old
file.

The call to ly:make-event-class takes one argument less, and doc-context
is no longer defined.

> No offense, just curious about it:
> I was told not to use tabs, what's the reason for using them here?

The first commit in the series is a revert.  It brought the tabs back
from the dead: apparently I untabified as part of the commit in
question.  I can probably rerun the Scheme indenter.  The "revert"
commit is not a clean revert anyway.

https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode91
scm/document-music.scm:91: (classes (ly:make-event-class class))
Again: this line has changed non-trivially, and the context around it is
then a large merge conflict because of having gotten untabified in the
original commit that has now been reverted.

Description:
Create a two-argument form of define-event-class

This definition of define-event-class just specifies the event class
symbol itself as well as its immediate parent class.  Redefining
existing event classes is not (yet?) supported.


I've come to the persuasion that the \DefineEventClass interface was
not a good idea as it introduces a separate event class hierarchy for
every Global context.  Since Global contexts share iterators (which
are part of the music types), that does not make sense.  The previous
approach tried to address the problem of permanent changes
transgressing the lifetime of a session: session variables make for a
better match to that problem.

The old definition of define-event-class was needlessly contorted for
what it allowed: this one is reasonably straightforward.

Also contains commits:


Reinitialize all-event-classes and all-grob-descriptions after session


Revert "Make EventClass hierarchy a property of Global context"

This reverts commit ae2db5b21bf232f5145f3a3e091689c8fc7247e9.

Conflicts:
        lily/context-scheme.cc
        lily/global-context.cc
        lily/music.cc
        lily/part-combine-iterator.cc
        scm/define-event-classes.scm
        scm/document-music.scm

Independently introduced conflict:
        lily/footnote-engraver.cc

Please review this at https://codereview.appspot.com/10965043/

Affected files:
  M input/regression/scheme-text-spanner.ly
  M lily/context-scheme.cc
  M lily/context.cc
  M lily/engraver.cc
  M lily/footnote-engraver.cc
  M lily/global-context.cc
  M lily/include/context.hh
  M lily/include/music.hh
  M lily/music.cc
  M lily/part-combine-iterator.cc
  M lily/rhythmic-music-iterator.cc
  M ly/engraver-init.ly
  M ly/performer-init.ly
  M scm/define-context-properties.scm
  M scm/define-event-classes.scm
  M scm/define-grobs.scm
  M scm/document-music.scm