Re: Create a two-argument form of define-event-class (issue 10965043)
On 2013/07/06 00:05:35, dak wrote: 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. True. Ofcourse. It's more that I'd prefer namings which mirrors more exactly what the defined stuff will do. (Though, that would mean to rename a the definition in titling-init.ly, too. And probably a lot of others throughout our source-code.) 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. Ahh, I've overlooked it. 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. Thanks for your explication. 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. https://codereview.appspot.com/10965043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Create a two-argument form of define-event-class (issue 10965043)
I didn't apply the patch and I can't review C++, though, after a first quick glance, some nitpicks: 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))) 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. https://codereview.appspot.com/10965043/diff/1/scm/define-grobs.scm File scm/define-grobs.scm (right): https://codereview.appspot.com/10965043/diff/1/scm/define-grobs.scm#newcode28 scm/define-grobs.scm:28: (define-session-public all-grob-descriptions Very nice! 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))) Apart from using tabs I'm not able to see any difference to the old file. No offense, just curious about it: I was told not to use tabs, what's the reason for using them here? https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode89 scm/document-music.scm:89: (props (cdr obj)) Same here https://codereview.appspot.com/10965043/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Create a two-argument form of define-event-class (issue 10965043)
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
