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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel