Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly File input/regression/scheme-text-spanner.ly (right): https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode117 input/regression/scheme-text-spanner.ly:117: (event-drul (cons '() '( On 2013/07/23 12:31:25, dak wrote: Frankly, just throw out the crap event-drul and current-event, and instead use event-start and event-stop instead of (car event-drul) and (cdr event-drul). current-event also is never set to anything except (car event-drul), so you can just replace it with event-start. This whole file is a huge parade of bad code. Done. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode157 input/regression/scheme-text-spanner.ly:157: (set! event-drul (cons '() '()) On 2013/07/23 13:05:47, david.nalesnik wrote: On 2013/07/23 12:31:25, dak wrote: You could do (set-car! event-drul '()) (set-cdr! event-drul '()) instead in order to not create a new cons cell. OK, will do. Fixed using new variables. https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
Instead, complete-grob-entry (or whatever it was called) needs to be changed in order to work non-destructively. It does not make sense to do this separately from the original code. OK, I won't bother yet with add-grob-definition here, but I wonder: if I use copy-tree on a literal expression, is it alright to modify the resulting list with assoc-set! and the like? In other words, does copying the list create another literal that I should again refrain from tampering with? I ask because the following attempt at avoidance looks like overkill to me: #(define (add-grob-definition grob-name grob-entry) (let* ((meta (assoc 'meta grob-entry)) (meta-entry (cdr meta)) (class (assoc-get 'class meta-entry)) (ifaces (assoc 'interfaces meta-entry)) (ifaces-entry (cdr ifaces)) (ifaces-entry (append (case class ((Item) '(item-interface)) ((Spanner) '(spanner-interface)) ((Paper_column) '((item-interface paper-column-interface))) ((System) '((system-interface spanner-interface))) (else '(unknown-interface))) ifaces-entry)) (ifaces-entry (uniq-list (sort ifaces-entry symbol?))) (ifaces-entry (cons 'grob-interface ifaces-entry)) (ifaces (cons (car ifaces) ifaces-entry)) (meta-entry (acons 'name grob-name meta-entry)) (meta-entry (map (lambda (x) (if (eq? (car x) 'interfaces) ifaces x)) meta-entry)) (meta (cons (car meta) meta-entry)) (grob-entry (map (lambda (x) (if (eq? (car x) 'meta) meta x)) grob-entry))) (set-object-property! grob-name 'translation-type? list?) (set-object-property! grob-name 'is-grob? #t) (set! all-grob-descriptions (cons (cons grob-name grob-entry) all-grob-descriptions Thanks for your patience! David https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly File input/regression/scheme-text-spanner.ly (right): https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode28 input/regression/scheme-text-spanner.ly:28: (set! meta-entry (assoc-set! meta-entry 'name grob-name)) Of course, these lines are also modifying constants. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode81 input/regression/scheme-text-spanner.ly:81: (set! lst (assoc-set! lst 'name (car x))) And these modify constants as well. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode117 input/regression/scheme-text-spanner.ly:117: (event-drul (cons '() '( Frankly, just throw out the crap event-drul and current-event, and instead use event-start and event-stop instead of (car event-drul) and (cdr event-drul). current-event also is never set to anything except (car event-drul), so you can just replace it with event-start. This whole file is a huge parade of bad code. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode157 input/regression/scheme-text-spanner.ly:157: (set! event-drul (cons '() '()) You could do (set-car! event-drul '()) (set-cdr! event-drul '()) instead in order to not create a new cons cell. https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
On 2013/07/23 13:05:47, david.nalesnik wrote: On 2013/07/23 12:31:25, dak wrote: Frankly, just throw out the crap event-drul and current-event, and instead use event-start and event-stop instead of (car event-drul) and (cdr event-drul). OK, will do. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode157 input/regression/scheme-text-spanner.ly:157: (set! event-drul (cons '() '()) On 2013/07/23 12:31:25, dak wrote: You could do (set-car! event-drul '()) (set-cdr! event-drul '()) instead in order to not create a new cons cell. OK, will do. Well, you will have a hard time doing both. I think I wrote the second comment before the first, and of course if you follow through with the first, the second will more or less by necessity turn into (set! event-start '()) (set! event-stop '()). https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
Thank you, David. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly File input/regression/scheme-text-spanner.ly (right): https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode28 input/regression/scheme-text-spanner.ly:28: (set! meta-entry (assoc-set! meta-entry 'name grob-name)) On 2013/07/23 12:31:25, dak wrote: Of course, these lines are also modifying constants. Yes, but this routine is cribbed from completize-grob-entry, which suffers from the same issues as you pointed out. Shouldn't this be a focus of another issue which addresses major problems in define-grobs.scm? My intention here was simply to fix a minor glitch. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode81 input/regression/scheme-text-spanner.ly:81: (set! lst (assoc-set! lst 'name (car x))) On 2013/07/23 12:31:25, dak wrote: And these modify constants as well. Yes; again, shouldn't this be the focus of another issue? https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode117 input/regression/scheme-text-spanner.ly:117: (event-drul (cons '() '( On 2013/07/23 12:31:25, dak wrote: Frankly, just throw out the crap event-drul and current-event, and instead use event-start and event-stop instead of (car event-drul) and (cdr event-drul). current-event also is never set to anything except (car event-drul), so you can just replace it with event-start. This whole file is a huge parade of bad code. OK, will do. https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode157 input/regression/scheme-text-spanner.ly:157: (set! event-drul (cons '() '()) On 2013/07/23 12:31:25, dak wrote: You could do (set-car! event-drul '()) (set-cdr! event-drul '()) instead in order to not create a new cons cell. OK, will do. https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
On 2013/07/23 13:27:49, dak wrote:
On 2013/07/23 13:05:47, david.nalesnik wrote:
On 2013/07/23 12:31:25, dak wrote:
Frankly, just throw out the crap event-drul and current-event, and
instead
use
event-start and event-stop instead of (car event-drul) and (cdr
event-drul).
OK, will do.
https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-spanner.ly#newcode157
input/regression/scheme-text-spanner.ly:157: (set! event-drul (cons
'()
'())
On 2013/07/23 12:31:25, dak wrote:
You could do (set-car! event-drul '())
(set-cdr! event-drul '())
instead in order to not create a new cons cell.
OK, will do.
Well, you will have a hard time doing both. I think I wrote the
second comment
before the first, and of course if you follow through with the first,
the second
will more or less by necessity turn into (set! event-start '()) (set!
event-stop
'()).
Sorry, replied a bit hastily!
I've thought some more about the issue of trying to change constants
elsewhere in the file.
Specifically, regarding the addition of the new grob description to
all-grob-descriptions, would the following be any better?
So, as I understand it, remove add-grob-description, duplicating its
changes to the description of 'SchemeTextSpanner, and creating a new
variable to avoid tampering with all-grob-descriptions, using that new
variable in the layout block.
#(define all-grob-descriptions-with-outlier
(cons
(cons
'SchemeTextSpanner
`(
(bound-details . ((left . ((Y . 0)
(padding . 0.25)
(attach-dir . ,LEFT)
))
(left-broken . ((end-on-note . #t)))
(right . ((Y . 0)
(padding . 0.25)
))
))
(dash-fraction . 0.2)
(dash-period . 3.0)
(direction . ,UP)
(font-shape . italic)
(left-bound-info . ,ly:line-spanner::calc-left-bound-info)
(outside-staff-priority . 350)
(right-bound-info . ,ly:line-spanner::calc-right-bound-info)
(staff-padding . 0.8)
(stencil . ,ly:line-spanner::print)
(style . dashed-line)
(meta . ((name . SchemeTextSpanner)
(class . Spanner)
(interfaces . (grob-interface
font-interface
line-interface
line-spanner-interface
side-position-interface
spanner-interface))
all-grob-descriptions))
#(set-object-property! 'SchemeTextSpanner 'translation-type? list?)
#(set-object-property! 'SchemeTextSpanner 'is-grob? #t)
[...]
\layout {
\context {
\Global
\grobdescriptions #all-grob-descriptions-with-outlier
}
\context {
\Voice
\consists \schemeTextSpannerEngraver
}
}
https://codereview.appspot.com/11614044/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
On 2013/07/23 13:52:13, david.nalesnik wrote: Sorry, replied a bit hastily! [...] I've thought some more about the issue of trying to change constants elsewhere in the file. Specifically, regarding the addition of the new grob description to all-grob-descriptions, would the following be any better? So, as I understand it, remove add-grob-description, duplicating its changes to the description of 'SchemeTextSpanner, and creating a new variable to avoid tampering with all-grob-descriptions, using that new variable in the layout block. No, that's not necessary. all-grob-descriptions is defined as a session variable, so it will be restored at the end of the session. You must not change it _destructively_ (meaning that you must not change any of its cons cells or what they point to), but you can replace with a different list as a whole. Since the code only added to the _front_ of all-grob-descriptions, this part was entirely harmless. The problem is rather with the code in complete-grob-entry or what it was. That one worked destructively on constant lists (like, one may add, our current code for creating all-grob-descriptions originally does). So what is being done is just as bad as our existing code elsewhere, but there is no interference with it. The part adding stuff to the front of all-grob-descriptions is fine by itself. https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
On 2013/07/23 15:06:51, dak wrote: On 2013/07/23 13:52:13, david.nalesnik wrote: Sorry, replied a bit hastily! [...] I've thought some more about the issue of trying to change constants elsewhere in the file. Specifically, regarding the addition of the new grob description to all-grob-descriptions, would the following be any better? So, as I understand it, remove add-grob-description, duplicating its changes to the description of 'SchemeTextSpanner, and creating a new variable to avoid tampering with all-grob-descriptions, using that new variable in the layout block. No, that's not necessary. all-grob-descriptions is defined as a session variable, so it will be restored at the end of the session. You must not change it _destructively_ (meaning that you must not change any of its cons cells or what they point to), but you can replace with a different list as a whole. Since the code only added to the _front_ of all-grob-descriptions, this part was entirely harmless. The problem is rather with the code in complete-grob-entry or what it was. That one worked destructively on constant lists (like, one may add, our current code for creating all-grob-descriptions originally does). So what is being done is just as bad as our existing code elsewhere, but there is no interference with it. The part adding stuff to the front of all-grob-descriptions is fine by itself. Hmmm, so if I understand correctly: The problem in the regtest is that add-grob-definition should not be applied to the quasiquoted list. In doing so, however, nothing is being done that doesn't have precedent in define-grobs.scm. I'm not understanding from your answer if you'd like me to change anything here, though. I hesitate to do something like the following! #(add-grob-definition 'SchemeTextSpanner (list (cons 'bound-details (list (cons 'left (list (cons 'Y 0) (cons 'padding 0.25) (cons 'attach-dir LEFT))) (cons 'left-broken (list (cons 'end-on-note #t))) (cons 'right (list (cons 'Y 0) (cons 'padding 0.25) (cons 'dash-fraction 0.2) (cons 'dash-period 3.0) (cons 'direction UP) (cons 'font-shape 'italic) (cons 'left-bound-info ly:line-spanner::calc-left-bound-info) (cons 'outside-staff-priority 350) (cons 'right-bound-info ly:line-spanner::calc-right-bound-info) (cons 'staff-padding 0.8) (cons 'stencil ly:line-spanner::print) (cons 'style 'dashed-line) (cons 'meta (list (cons 'class 'Spanner) (cons 'interfaces (list 'font-interface 'line-interface 'line-spanner-interface 'side-position-interface)) https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
On 2013/07/23 20:05:40, david.nalesnik wrote: Hmmm, so if I understand correctly: The problem in the regtest is that add-grob-definition should not be applied to the quasiquoted list. In doing so, however, nothing is being done that doesn't have precedent in define-grobs.scm. Pretty much I'm not understanding from your answer if you'd like me to change anything here, though. I hesitate to do something like the following! #(add-grob-definition 'SchemeTextSpanner (list (cons 'bound-details (list [...] No, that's nonsensical. Instead, complete-grob-entry (or whatever it was called) needs to be changed in order to work non-destructively. It does not make sense to do this separately from the original code. I'm just not overly enthused of having to do this kind of stuff myself right now as I am working on other, hard stuff. So I'm throwing out the information in the hope that it becomes somebody else's problem™. https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: input/regression/scheme-text-spanner.ly: fix problem with constants (issue 11614044)
On 2013/07/23 20:16:24, dak wrote: On 2013/07/23 20:05:40, david.nalesnik wrote: I hesitate to do something like the following! #(add-grob-definition 'SchemeTextSpanner (list (cons 'bound-details (list [...] No, that's nonsensical. Instead, complete-grob-entry (or whatever it was called) completize -- which sounds like a made-up word to me -- perhaps complete is ambiguous: a verb or an adjective? needs to be changed in order to work non-destructively. It does not make sense to do this separately from the original code. I'm just not overly enthused of having to do this kind of stuff myself right now as I am working on other, hard stuff. So I'm throwing out the information in the hope that it becomes somebody else's problem™. Ah, OK! This will give me (or whoever) something to think about. Thanks, David https://codereview.appspot.com/11614044/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
