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

Added missing NSOrderedSet methods (insert, remove, replace, etc) #181

Merged
merged 2 commits into from
Aug 7, 2014

Conversation

JRG-Developer
Copy link
Contributor

Added missing NSOrderedSet methods (insert, remove, replace, etc):

See Issue #75, comment by @batkuip "Those methods are trickier to implement because the mutable set accessor uses mutableSetValueForKey which seems to call the insertObject:atIndex internally causing it loop forever."

This code creates a temporary mutable set using the ordered set (not the mutable set which loops as noted), making sure to call appropriate KVC methods.

Credit for original code/idea goes to Dmitry Makarenkoless and JLust on StackOverFlow (http://stackoverflow.com/questions/7385439/exception-thrown-in-nsorderedset-generated-accessors).

See Issue rentzsch#75, comment by @batkuip "Those methods are trickier to implement because the mutable set accessor uses mutableSetValueForKey which seems to call the insertObject:atIndex internally causing it loop forever."

This code creates a temporary mutable set using the ordered set (not the mutable set which loops as noted), making sure to call appropriate KVC methods.

Credit for original code/idea goes to @dmitry Makarenkoless and @jlust on StackOverFlow (http://stackoverflow.com/questions/7385439/exception-thrown-in-nsorderedset-generated-accessors).
@batkuip
Copy link
Contributor

batkuip commented Nov 20, 2013

Cool. I'll check it out when I get home. I think I tried the SO fix you are referencing and it works but it didn't send the right notifications making it impractical for use with tableviews etc.. I think this sends out a notification that ALL set items have changed?

@JRG-Developer
Copy link
Contributor Author

@batkuip, I believe it should notify observers only about indexes that will be affected.

E.g. from SO code:

- (void)insertObject:(FRPlaylistItem *)value inSubitemsAtIndex:(NSUInteger)idx {

    // Create an indexSet with just the index to be changed
    NSIndexSet* indexes = [NSIndexSet indexSetWithIndex:idx];

    // Notify KVO about upcoming change
    [self willChange:NSKeyValueChangeInsertion valuesAtIndexes:indexes forKey:kItemsKey];

    NSMutableOrderedSet *tmpOrderedSet = [NSMutableOrderedSet orderedSetWithOrderedSet:[self mutableOrderedSetValueForKey:kItemsKey]];
    [tmpOrderedSet insertObject:value atIndex:idx];    
    [self setPrimitiveValue:tmpOrderedSet forKey:kItemsKey];

    // Notify KVO about change that occurred
    [self didChange:NSKeyValueChangeInsertion valuesAtIndexes:indexes forKey:kItemsKey];
}

The other methods have similar KVC calls.

We've been using this in table views (prior to using mogenerator to create the code automatically), and I don't believe any of our apps have issues due to this.

What situation are you envisioning that it would cause a problem?

@batkuip
Copy link
Contributor

batkuip commented Nov 20, 2013

I think I was interested in willChangeValueForKey:withSetMutation:usingObjects: and didChangeValueForKey:withSetMutation:usingObjects: and I couldn't easily recreate them.

@JRG-Developer
Copy link
Contributor Author

I don't believe that willChangeValueForKey:withSetMutation:usingObjects: and didChangeValueForKey:withSetMutation:usingObjects: are applicable to NSMutableOrderedSet.

Per my understanding of NSKeyValueObserving Protocol Reference (see here), I believe these methods are only for unordered to-many relationships (i.e. NSMutableSet).

Instead, I believe that willChange:valuesAtIndexes:forKey: and didChange:valuesAtIndexes:forKey: should be used for ordered to-many relationships, which this code does call.

If you find I'm mistaken (I don't believe both are called?), please let me know.

Ideally, Apple would fix this ordered set issue once and for all, but until then, I think this might be the best that can be done...

@rentzsch
Copy link
Owner

rentzsch commented Jul 9, 2014

Good news everyone, I just tested this against 10.7 and 10.9 and it works. It will land in mogenerator v1.28, due out Real Soon Now.

@rentzsch rentzsch merged commit a971c39 into rentzsch:master Aug 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants