My First Patch: issue #1063

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

My First Patch: issue #1063

Rodolfo Zitellini
Hi all,
I made the little patch that should resolve #1063. Was made with diff
-n, and I tried my best to follow GNU coding standards. This is my
first attempt, so please be clement :).

The modifications work like this:
before reverting something, it makes sure another copy of the element
you are reverting is present in the alist, so that you never drop the
original value (this is what happens now when you revert a nested
property non previously overridden).

Comments are welcome! :)

Rodolfo

nested-property.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: My First Patch: issue #1063

Carl Sorensen
Hi Rodolfo,

Thanks for posting this patch!  I'm really happy to have you working on
developing LilyPond!

On 5/19/10 9:46 AM, "Rodolfo Zitellini" <[hidden email]> wrote:

> Hi all,
> I made the little patch that should resolve #1063. Was made with diff
> -n, and I tried my best to follow GNU coding standards. This is my
> first attempt, so please be clement :).

Wow -- this is the first time I've ever heard "clement" used in a sentence.
In the U.S., all I hear is "inclement"; I've even heard discussions about
whether "clement" is a word or not.  Thanks for the education!

>
> The modifications work like this:
> before reverting something, it makes sure another copy of the element
> you are reverting is present in the alist, so that you never drop the
> original value (this is what happens now when you revert a nested
> property non previously overridden).

It appears that your code only removes the first occurrence found.  In
general, \revert should remove all but the last occurrence found.  We want
\revert to get us back to the original definition, not just to the
definition before the current definition.

If MyGrob's default value for 'myProp is 1, and I do

\override MyGrob #'myProp = #10
\override MyGrob #'myProp = #2
\revert MyGrob #'myProp

the value of 'myProp for MyGrob should be 1, not 10.

Thanks,

Carl


---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: My First Patch: issue #1063

Rodolfo Zitellini
> Wow -- this is the first time I've ever heard "clement" used in a sentence.
> In the U.S., all I hear is "inclement"; I've even heard discussions about
> whether "clement" is a word or not.  Thanks for the education!
>
Well I have to admit I am not 100% sure it is correct, but I looked it
up in the dictionary before using it :)

>
> If MyGrob's default value for 'myProp is 1, and I do
>
> \override MyGrob #'myProp = #10
> \override MyGrob #'myProp = #2
> \revert MyGrob #'myProp
>
> the value of 'myProp for MyGrob should be 1, not 10.
>

This works with my patch too. I have noticed, using the debugger, that
every \override generates a call to execute_revert_property() before
calling execute_override_property() on the same property - so you end
up with only two copies (new and old) in the list all the times (this
seems to be done for "normal" and nested properties).

My patch still leaves open the issue on
\override MyGrob #''(nested-prop)
but I beleve this to be unrelated to #1063 (and currently broken, see
last mail :). I did some preliminary work on this, but my mods just
crash the regtests (I mean - literally!) so I think I'm not quite
ready to post a patch :)

Rodolfo

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|

Re: My First Patch: issue #1063

Carl Sorensen



On 5/20/10 5:08 AM, "Rodolfo Zitellini" <[hidden email]> wrote:

>> Wow -- this is the first time I've ever heard "clement" used in a sentence.
>> In the U.S., all I hear is "inclement"; I've even heard discussions about
>> whether "clement" is a word or not.  Thanks for the education!
>>
> Well I have to admit I am not 100% sure it is correct, but I looked it
> up in the dictionary before using it :)

I checked the dictionary after I read it, and you are absolutely correct.

>
>>
>> If MyGrob's default value for 'myProp is 1, and I do
>>
>> \override MyGrob #'myProp = #10
>> \override MyGrob #'myProp = #2
>> \revert MyGrob #'myProp
>>
>> the value of 'myProp for MyGrob should be 1, not 10.
>>
>
> This works with my patch too. I have noticed, using the debugger, that
> every \override generates a call to execute_revert_property() before
> calling execute_override_property() on the same property - so you end
> up with only two copies (new and old) in the list all the times (this
> seems to be done for "normal" and nested properties).

Great!  I'll test the patch as soon as I can.

Thanks,

Carl


---
----
Join the Frogs!