Fix 153: \once\set properly restores the context property

Fix 153: \once\set properly restores the context property

\once\set works by installing a finalization hook in the iterator.
To restore the context property value before the \once\set, we simply
cache the old value and pass it to the finalization hook as third
argument. The finalizatino hook then sends a SetProperty event with
the old value rather than an UnsetProperty event, which reverts the
value to the default.

[Trevor Daniels, 2011-07-21 07:09:18]

LGTM, but I'd prefer longer names for o, m and tg
(especially tg!).

Trevor

[joeneeman, 2011-07-21 16:15:41]

If you do

\set #'foo = #1
c
\once \set #'foo = #2
c
\once \set #'foo = #3
c

then the final value of foo should be 1. But with this code, I think it will be
2.

[Neil Puttock, 2011-07-21 17:06:57]

On 2011/07/21 16:15:41, joeneeman wrote:
> If you do
> 
> \set #'foo = #1
> c
> \once \set #'foo = #2
> c
> \once \set #'foo = #3
> c
> 
> then the final value of foo should be 1. But with this code, I think it will
be
> 2.

The \once finalization occurs before the new \once \set is processed, so it
always returns to the original (or most recent) \set.

[Reinhold, 2011-07-21 17:10:58]

On 2011/07/21 16:15:41, joeneeman wrote:
> If you do
> 
> \set #'foo = #1
> c
> \once \set #'foo = #2
> c
> \once \set #'foo = #3
> c
> 
> then the final value of foo should be 1. But with this code, I think it will
be
> 2.

Why should it be 2? The finalization hook to reset the value to the previous one
is processed at the end of the current moment, long before the next \once\set
will be processed (at the begin of the next moment). So when the second
\once\set is processed, the value of #'foo has already been reset to #1 and the
finalization  hook installed for the secnd \once\set will reset the property to
1.

BTW, things like

\set #'foo = #1
c
\once \set #'foo = #2
\once \set #'foo = #3
c
c

also work properly (at the end the value will be 1), because apparently the
finalization hooks are processed in the correct (reverse) order.

Anyway, thanks for thinking about this case. I have updated the regtest to
include it.

[Neil Puttock, 2011-07-21 17:34:07]

LGTM.

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly
File input/regression/set-once.ly (right):

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:1: \version "2.15.5"
2.15.6

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:5: texidoc = "@code{\once\set} should revert the
setting after the next moment
\once \set

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:6: to the previous value rather than the default."
this sounds a bit ambiguous; could be misinterpreted

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:11: < e -1>1 |
<e-1>1

etc.

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:14: < e -1> -"left" |
you could add an assert function (using ApplyContext) to verify that the value
returns to the original \set (if you're feeling paranoid :)

http://codereview.appspot.com/4810042/diff/4001/lily/property-iterator.cc
File lily/property-iterator.cc (right):

http://codereview.appspot.com/4810042/diff/4001/lily/property-iterator.cc#new...
lily/property-iterator.cc:43: /* For \once\set install a finalization hook to
reset the property to the old
\once \set

http://codereview.appspot.com/4810042/diff/4001/lily/property-iterator.cc#new...
lily/property-iterator.cc:44: * value after the next timestep */
don't you mean at the end of the current timestep?

http://codereview.appspot.com/4810042/diff/4001/lily/property-iterator.cc#new...
lily/property-iterator.cc:74: // restore the value before the \once\set command
\once \set

[Reinhold, 2011-07-21 18:25:35]

Will also correct those spacing issues pointed out by Neil before I'll push.

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly
File input/regression/set-once.ly (right):

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:6: to the previous value rather than the default."
On 2011/07/21 17:34:07, Neil Puttock wrote:
> this sounds a bit ambiguous; could be misinterpreted

Yes, I'll reformulate it. How about 
"@code{\once \set} should change a context property value for just one timestep
and then return to the previous value, rather than resetting the property to its
global default value."

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:14: < e -1> -"left" |
On 2011/07/21 17:34:07, Neil Puttock wrote:
> you could add an assert function (using ApplyContext) to verify that the value
> returns to the original \set (if you're feeling paranoid :)

Hehe... No, that would make our life harder should we ever break that, because
then the regtest build will fail for all of use, rather than simply showing some
differences in the images.

http://codereview.appspot.com/4810042/diff/4001/lily/property-iterator.cc
File lily/property-iterator.cc (right):

http://codereview.appspot.com/4810042/diff/4001/lily/property-iterator.cc#new...
lily/property-iterator.cc:44: * value after the next timestep */
On 2011/07/21 17:34:07, Neil Puttock wrote:
> don't you mean at the end of the current timestep?

Well, nice catch, I'm actually mixing two different views: 
View 1) \set is not assigned to a note, but is written between two notes, so the
following note (to which it shall apply) will be the NEXT timestep
View 2) \set is evaluated together with the next following note, so during
processing its value is for the CURRENT timestep.

You are right, I should probably stick to view 2 and use current timestep here.

[pkx166h, 2011-07-22 20:56:42]

pass make and reg tests.

James

[Neil Puttock, 2011-07-23 20:48:20]

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly
File input/regression/set-once.ly (right):

http://codereview.appspot.com/4810042/diff/4001/input/regression/set-once.ly#...
input/regression/set-once.ly:6: to the previous value rather than the default."
On 2011/07/21 18:25:35, Reinhold wrote:
> On 2011/07/21 17:34:07, Neil Puttock wrote:
> > this sounds a bit ambiguous; could be misinterpreted
> 
> Yes, I'll reformulate it. How about 
> "@code{\once \set} should change a context property value for just one
timestep
> and then return to the previous value, rather than resetting the property to
its
> global default value."

Hmm, I still think `the previous value' is ambiguous; if there's no explicit
setting, but a global default does exist, then the value will return to the
global setting.

[Reinhold, 2011-07-28 12:26:58]

On 2011/07/23 20:48:20, Neil Puttock wrote:
> On 2011/07/21 18:25:35, Reinhold wrote:
> > Yes, I'll reformulate it. How about 
> > "@code{\once \set} should change a context property value for just one
> timestep
> > and then return to the previous value, rather than resetting the property to
> its
> > global default value."
> 
> Hmm, I still think `the previous value' is ambiguous; if there's no explicit
> setting, but a global default does exist, then the value will return to the
> global setting.


I have now changed it to "... and then return to the previous value.". Noone
would expect \once \set to return to the default value, so it doesn't make much
sense to refer to the current buggy situation at all.

I have committed and pushed this as commit
d1db9b1d6ebb1014429974f22162b44bf9a03533