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

Better modeling for KVO collection changes #1032

Merged
merged 28 commits into from Jan 24, 2014
Merged

Conversation

jspahrsummers
Copy link
Member

This abstracts over Cocoa's built-in collections with <RACCollection> and <RACOrderedCollection> protocols, and then models changes to them with <RACCollectionMutation> and <RACOrderedCollectionMutation>. There are ordered and unordered versions of KVO change kinds.

Although this might seem like over-abstraction, it means that new collections and new mutation types can be added in the future (or by consumers) without affecting existing code.

I chose to focus on mutation instead of immutable transformation for two reasons:

  1. Performance. I hate this line of argumentation, but the performance of copying really can add up. Since these kind of changes can be really granular, it's likely that they'll be getting applied constantly.
  2. Granular KVO notifications. Mutation makes it really easy to generate lossless (or mostly lossless) KVO notifications when updating a collection.

Plus, “immutable” transformation can always be emulated by creating a mutable copy of the input.

This API will eventually be useful for granular table/collection view updates.

@dnalot @iMartinKiss @indragiek

To do:

  • Finish up documentation
  • Implement automatic changes for view-based NSTableViews
  • Add UITableView bindings
  • Add UICollectionView bindings
  • Unit tests

@jspahrsummers
Copy link
Member Author

@robrix This addresses some of the things we were discussing the other day, I think. The collection mutation protocols are functors, so it should be easy to "remap" changes to a different type of object (like updating your VM to match changes in your model).

@kastiglione
Copy link
Member

This API can be used to perform granular table/collection view updates.

This was the first thing that came to mind when reading through. 💡

///
/// NSMutableArray *VMs = [self mutableArrayValueForKey:@keypath(self.viewModels)];
/// [viewModelsMutation mutateCollection:VMs];
/// }];
Copy link
Member

Choose a reason for hiding this comment

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

This pattern reminds me of RAC(…) = RACObserve(…). What are your thoughts on similar macros?

(macro names for illustration)

RACMutate(self, viewModels) = [RACObserveMutations(self, models)
    reduceEach:^(id _, id<RACOrderedCollectionMutation> modelsMutation) {
        return [modelsMutation map:^(Model *model) {
            return [[ViewModel alloc] initWithModel:model];
        }];
    }];

Copy link
Member Author

Choose a reason for hiding this comment

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

Collection updates are comparatively uncommon, and we already get flak for the common macros. I'd rather not bloat the framework further with more.

@notxcain
Copy link

Damn, I hate this when you work on something a whole month, and then there is a pull request in ReactiveCocoa, doing exactly the same thing ;(( I feel bad ;( But the cool part is the fact that this idea is originated in someone else head )

@indragiek
Copy link
Member

Totally stoked for this! Question about the NSTableView integration: would the API be directly manipulating the table view (ie. automatically calling -insertRowsAtIndexes:.. etc.) using a subscription to the mutations signal? In other words, would it be something like this (based on @kastiglione's suggested macro pattern):

RACSomething(self, tableView) = RACObserveMutations(self, models);

@notxcain
Copy link

@indragiek You should take into consideration that table view reflects sectioned collection. Not a simple array.

@indragiek
Copy link
Member

This is for NSTableView, which doesn't have section support like UITableView does.

Indragie Karunaratne

On Fri, Dec 27, 2013 at 2:28 AM, Denis Mikhaylov notifications@github.com
wrote:

@indragiek You should take into consideration that table view reflects sectioned collection. Not a simple array.

Reply to this email directly or view it on GitHub:
#1032 (comment)

@notxcain
Copy link

@indragiek my bad )

@jspahrsummers
Copy link
Member Author

@indragiek Not sure yet. I'm just gonna start implementing it and see what makes sense. Part of that will be determining whether animation support is important at all.

@robrix
Copy link
Contributor

robrix commented Jan 2, 2014

This is a nice approach! It appears not to handle moves at the moment, but that seems like a relatively straightforward extension.

I would suggest that a notion of applying some ordering is a good mutation to capture, since it’s not uncommon to mutate elements of a set which gets sorted into an array for display. This gives mutations from unordered to ordered (sorting, changing an element such that it is sorted at a different index), and from ordered to ordered (resorting, moving, etc).

@neilpa
Copy link
Member

neilpa commented Jan 2, 2014

@robrix I agree with you about the need for a move mutation, especially when considering animations you don't want to implement it as a remove then add.

I'm not sure sorting (and filtering) fit this mutation model as well. They may be better handled as a separate operators or collection transforms. Specifically you want RACCollection -> RACOrderedCollection for sort and RACCollection -> RACCollection for filter. Map could fit in this boat too but I like the current approach on the mutations.

The hard part of this problem is something you already alluded to

changing an element such that it is sorted at a different index

Solving this well requires a way to efficiently aggregate item level changes into the collection's mutation stream. Based on the current code this might look like

@interface RACItemMutation : NSObject <RACCollectionMutation>
@property id item;
@property NSString* keyPath;
@property NSDictionary* change;
// or you could extract old and new values and  expose them directly instead of the KVO dictionary 
@end

@interface RACOrderedItemMutation : RACItemMutation <RACOrderedCollectionMutation>
@end

The naive approach to generating these is subscribing property keys that affect sort and/or filter for all items. There could be a default implementation for said approach that would work well enough on small collections. More complicated collections would likely require a custom RACCollection implementation with inherent knowledge of the contained items and their change patterns.

@lawrencelomax
Copy link
Contributor

This is awesome. I can certainly see that I would use this in a good amount of my code and can't wait to use it.

Could this be even more abstract when applied to Signals of collections? I'm thinking of an operator on RACSignal that could act similarly distinctUntilChanged; given a Signal of Collections, return a Signal of changes to the collection in each next event. If there are no changes in successive next events, the event is ignored. This would remove the reliance on KVO also, rac_valuesAndCollectionMutationsForKeyPath: could then build on top of this operator.

RACSignal<<RACCollection>> -> RACSignal<<RACCollectionMutation>>

I can think of one non-UI area would be (necessary) side-effecting when a remote resource changes, where the remote resource is represented by a Signal of collections. When there additions/deletions to the collection, side-effects occur. With the current implementation, this could be done by binding to-and-from a property. However, if the side-effect belongs further up the Signal chain, the Signal operator would be preferable.

@jspahrsummers
Copy link
Member Author

given a Signal of Collections, return a Signal of changes to the collection in each next event.

I have no intention of supporting anything like this. You basically need a full diffing algorithm to calculate deltas reliably.

@lawrencelomax
Copy link
Contributor

I see, this is something that NSKeyValueChangeRemoval and friends are providing.

@jspahrsummers
Copy link
Member Author

@lawrencelomax Unfortunately, that's not automatic either. Those change kinds require the use of -mutableArrayValueForKey: (or similar), or explicit KVO notifications about the changes made.

@jspahrsummers
Copy link
Member Author

With UITableView, I decided to focus on the use case of binding one single section. The alternatives (representing all mutations with index paths instead of index sets, storing index paths as the mutated objects themselves, a user-provided block at the point of binding) all sucked.

If people think that makes sense, I can implement a similar pattern for UICollectionView.

@robrix @neilpa Sorting is a sufficiently thorny problem that I'm gonna punt on it for now. I'd suppose that the mutations already captured should cover 80% of the use cases for automatic bindings.

@jspahrsummers
Copy link
Member Author

Also decided to punt on/avoid NSMutableDictionary support, which poses the difficult question of what to do when a key already exists in the dictionary, but with a different value. Since observing dictionary changes won't result in <RACCollectionMutation> objects anyways, it probably wouldn't even be that useful.

@neilpa
Copy link
Member

neilpa commented Jan 3, 2014

@jspahrsummers There's plenty o' good stuff here so punting on sorting makes sense. Given time this weekend I'll experiment with this as a replacement for some custom query/collection code I have in my current project. Requires back-porting to RAC 2.1 first though :(

@robrix
Copy link
Contributor

robrix commented Jan 6, 2014

@jspahrsummers agreed on punting that—it’s worth thinking about, but not worth delaying this (already useful!) stuff for.

@ghost ghost assigned joshaber Jan 6, 2014
@joshaber
Copy link
Member

🤘

@jspahrsummers
Copy link
Member Author

💻

/// should be animated.
///
/// Returns a disposable which can be used to cancel the binding.
- (RACDisposable *)rac_animateOrderedMutations:(RACSignal *)orderedMutations withInsertionAnimation:(NSTableViewAnimationOptions)insertionOptions removalAnimation:(NSTableViewAnimationOptions)removalOptions;
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice if the animations parameters (here and elsewhere) were signals since it seems likely you won't always want animations. But I'm happy to merge this without it for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I designed these APIs to be super simple. It's easy enough to implement this by hand that it'd be totally appropriate to reinvent for special behaviors w.r.t. animations, or updating multiple parts of a UI{Table,Collection}View at once.

@joshaber
Copy link
Member

🍬

Also needs some merge action.

Conflicts:
	ReactiveCocoaFramework/ReactiveCocoa.xcodeproj/project.pbxproj
	ReactiveCocoaFramework/ReactiveCocoa/NSObject+RACPropertySubscribing.m
	ReactiveCocoaFramework/ReactiveCocoaTests/NSObjectRACPropertySubscribingSpec.m
@joshaber
Copy link
Member

🆒

joshaber added a commit that referenced this pull request Jan 24, 2014
Better modeling for KVO collection changes
@joshaber joshaber merged commit f48f7e2 into 3.0-development Jan 24, 2014
@joshaber joshaber deleted the mutations branch January 24, 2014 20:51
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.

None yet

8 participants