Skip to content
This repository was archived by the owner on Mar 14, 2020. It is now read-only.

[ADM] Fix bug about deep copy of ADM tree#113

Closed
DonnaWuDongxia wants to merge 1 commit into
intel:masterfrom
DonnaWuDongxia:deep-copy-bug
Closed

[ADM] Fix bug about deep copy of ADM tree#113
DonnaWuDongxia wants to merge 1 commit into
intel:masterfrom
DonnaWuDongxia:deep-copy-bug

Conversation

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor

In ADM.copySubtree function, all property values had been treated as object, and been done with deep copy, but will be error for properties whose type is no object.
Another thing is if we do deep copy here, then how about all cases where we called setProperty? We need to change them all? Then what if the programmer forgot to do deep copy? So, I think we need to do deep copy in setProperty function.
For the event data, I think it may be need deep copy too.

@grgustaf

grgustaf commented Jul 3, 2012

Copy link
Copy Markdown
Contributor

Donna, can you explain the bug you're talking about with copySubtree? I admittedly didn't test very much when I made the change to use $.extend recently... I didn't think I was really changing the behavior very much, so I only tested a little bit.

I think non-objects will be just copied by value, so I don't know you're referring to. It seemed to work fine in the testing I did, as well.

@grgustaf

grgustaf commented Jul 3, 2012

Copy link
Copy Markdown
Contributor

As for setProperty, again I'd like to know what actual problem you're trying to solve?

@grgustaf

grgustaf commented Jul 3, 2012

Copy link
Copy Markdown
Contributor

OK, I was able to reproduce a problem with the copySubtree - thanks for catching that. I believe I've fixed it in master. As for the setProperty part, can you provide a test case where this is a problem?

@DonnaWuDongxia

Copy link
Copy Markdown
Contributor Author

Donna sent Geoff an email about this.

Comment thread src/js/adm.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you change this? It seems there's no difference between your implementation and the old one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants