SC.ContentValueSupport only works with one content key #831

Closed
wants to merge 1 commit into
from

4 participants

@jameschao

SC.ContentValueSupport doesn't work with more than one content key, because in contentPropertyDidChange(), the code fails to loop through all of the items in contentKeys

@publickeating
SproutCore member

Hi,

I checked this out and while trying to write a unit test to show the bug, I found that it is working properly as far as I can tell. contentPropertyDidChange(key) gets called for each contentProperty that changes and key is a single value, so it's correct to return as soon as it is found rather than continue the loop (technically key should only match one contentKey).

If you can create a unit test to show the problem, I will re-open. Thanks.

@wolfgangGoedel

The error still occurs if contentPropertyDidChange(key) is called with key === "*". In this case only the first contentKey will be updated because the condition in the loop will be true the first time.

The proposed change fixes the problem.

@dcporter
SproutCore member

I don't understand this code well enough to reopen the issue, but I am having trouble seeing how proper support for "*" is implemented when there are multiple content keys. It definitely looks like multiple contentKeys are ignored due to the preemptive return.

@wolfgangGoedel have you run this code and confirmed the fix? It looks to me like the return is in the wrong place, and the attached patch actually makes things worse. I could be wrong but I believe this patch is broken.

The current master code returned as soon as possible to avoid looping and doing a bunch of (potentially expensive?) calls to getDelegateProperty; the updated code can do a conditional return inside the loop's conditional block to avoid the same. if (key !== "*") return this;

Also of course it will need a unit test. It'll take a little work but it belongs in:

frameworks/foundation/tests/mixins/content_value_support/content.js

at the bottom, in the (currently mis-commented) SC.ContentValueSupport#contentKeys module.

@jameschao

Hi all, I took another stab at the bug and did get to write some unit tests. Submitted a new pull request #1106. Thanks.

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