Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add KVO tests and remove hack in -[COObject replaceContentOfCollection:... #42

Open
ericwa opened this issue Jan 28, 2016 · 1 comment

Comments

@ericwa
Copy link
Member

ericwa commented Jan 28, 2016

I tried removing this hack:

        // FIXME: Remove this copy hack to ensure KVO doesn't report a old value
        // identical to the new one.

        // NOTE: This mess is because -copy on a COPrimitiveCollection preserves
        // the number of -beginMutation calls, and we want the copy
        // to go back to being immutable once we are finished modifying it.
        if ([self isCoreObjectCollection: collection])
        {
            collection = [collection copy];
        }
        else
        {
            collection = [collection mutableCopy];
        }
        [(NSMutableArray *)collection setArray: content];

https://github.com/etoile/CoreObject/blob/master/Core/COObject.m#L977

but it caused weird failures in the attributed string tests.
I don't understand the comment:

        // FIXME: Remove this copy hack to ensure KVO doesn't report a old value
        // identical to the new one.

Anyway, we should add KVO tests to find out exactly what that hack is doing, and then remove the hack.

@qmathe
Copy link
Member

qmathe commented Jan 28, 2016

I'm not 100% of every details in the explanation below, but iirc it's something close to that.

When you call -willChangeValueForKey:, KVO invokes -valueForKey: and retains/copies the returned value that will be made later available in the change notification under the key NSKeyValueChangeOldKey.

I can think of two explanations for this hack, either COPrimitiveCollection classes were not implementing NSCopying at the time this comment was added, so KVO couldn't make a copy to snapshot the collection prior to the change, or simply KVO never copies the collection but retains it on -willChangeValueForKey:.

If the collection is not copied, after mutating it, we cannot assign the collection in -replaceContentOfCollection:withCollection: like that:

collection = content

but we must do

collection = [content copy]

If we don't that and 'collection' is identical to 'content', then the returned collection to be put in the variable storage is the same than previously, this means both NSKeyValueChangeOldKey and NSKeyValueChangeNewKey will return the same value when they shouldn't.

From what I remember it's a hack I added to get some tests passing (may be the attributed string tests in fact).

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

No branches or pull requests

2 participants