input/regression/scheme-text-spanner.ly: fix problem with constants

input/regression/scheme-text-spanner.ly: fix problem with constants

The file `input/regression/scheme-text-spanner.ly' assigns a constant
pair to the variable `event-drul' and subsequently attempts to modify
the pair.  This can lead to scheme-text-spanners not appearing when
requested in simultaneous contexts.  This patch fixes the problem.

[david.nalesnik, 2013-07-23 11:32:57]

Please review.

Thanks,
David

[dak, 2013-07-23 12:31:25]

https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-s...
File input/regression/scheme-text-spanner.ly (right):

https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-s...
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-s...
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-s...
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-s...
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.

[david.nalesnik, 2013-07-23 13:05:47]

Thank you, David.

https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-s...
File input/regression/scheme-text-spanner.ly (right):

https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-s...
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-s...
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-s...
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-s...
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.

[dak, 2013-07-23 13:27:49]

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-s...
> 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
'()).

[david.nalesnik, 2013-07-23 13:52:13]

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-s...
> > 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
  }
}

[dak, 2013-07-23 15:06:51]

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.

[david.nalesnik, 2013-07-23 20:05:40]

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))))))

[dak, 2013-07-23 20:16:24]

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™.

[david.nalesnik, 2013-07-23 20:28:46]

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

[david.nalesnik, 2013-07-24 15:11:00]

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

[david.nalesnik, 2013-07-25 15:20:25]

https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-s...
File input/regression/scheme-text-spanner.ly (right):

https://codereview.appspot.com/11614044/diff/1/input/regression/scheme-text-s...
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-s...
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.