Permalink
Browse files

Funnel all changes through -modifyWithBlock:

The old way funneled through -setValue:forKeyPath:. If there is no
document, -modifyWithBlock: now applies the change immediately; if there
is a document, the modification block is made root-relative and passed
up.

We don't call -commitModificationsKeepingChanges: on collections we
didn't send -beginModifications to (perhaps because the collection was
added as the modification).
  • Loading branch information...
jmah committed Nov 14, 2011
1 parent a4ea66f commit 6dfbdad0990069f6436260a423aeed0ce94ca506
View
@@ -21,8 +21,6 @@
@property (readonly, nonatomic, retain) RKRevision *currentRevision;
@property (readonly, nonatomic, copy) RKMutableDictionary *root;
-- (BOOL)modifyWithBlock:(RKModificationBlock)modBlock;
-
-- (RKModificationBlock)modificationBlockToSetValue:(id)newValue forRootKeyPath:(NSString *)keyPath;
+- (BOOL)modifyWithBlock:(RKModificationBlock)rootRelativeModBlock;
@end
View
@@ -33,11 +33,11 @@ - (id)initWithIdentifier:(NSString *)identifierOrNil;
return self;
}
-- (BOOL)modifyWithBlock:(RKModificationBlock)modBlock;
+- (BOOL)modifyWithBlock:(RKModificationBlock)rootRelativeModBlock;
{
[self.root beginModifications];
[self willChangeValueForKey:@"currentRevision"];
- BOOL success = modBlock(self.root);
+ BOOL success = rootRelativeModBlock(self.root);
if (success)
_currentRevision = [[RKUnsavedRev alloc] initAsSuccessorOfRev:self.currentRevision];
[self didChangeValueForKey:@"currentRevision"];
@@ -46,27 +46,6 @@ - (BOOL)modifyWithBlock:(RKModificationBlock)modBlock;
}
-#pragma mark RKMutableDictionary
-
-- (RKModificationBlock)modificationBlockToSetValue:(id)newValue forRootKeyPath:(NSString *)keyPath;
-{
- id oldValue = [self.root valueForKeyPath:keyPath];
-
- // If unchanged, always succeed (and avoid capturing unneeded values)
- if ((oldValue == newValue) || [oldValue isEqual:newValue])
- return [^BOOL(RKMutableDictionary *localRoot) { return YES; } copy];
-
- return [^BOOL(RKMutableDictionary *localRoot) {
- id curValue = [localRoot valueForKeyPath:keyPath];
- if ((curValue == oldValue) || [curValue isEqual:oldValue]) {
- [localRoot setValue:newValue forKeyPath:keyPath];
- return YES;
- }
- return (curValue == newValue) || [curValue isEqual:newValue];
- } copy];
-}
-
-
#pragma mark RKDocument: Private: Identifier Generation
+ (uint16_t)randomUint16Min:(uint16_t)minVal max:(uint16_t)maxVal;
@@ -14,4 +14,6 @@
// In constrast to NS(Mutable)Dictionary, -objectForKey: and -setObject:forKey: are funneled through -valueForKey: and setValue:forKey:
+- (RKModificationBlock)modificationBlockToSetValue:(id)newValue forKey:(NSString *)key;
+
@end
@@ -26,6 +26,7 @@ @implementation RKMutableDictionary {
NSMutableDictionary *_backingDictionary;
NSUInteger _modificationDepth;
+ NSMutableArray *_valuesWithOpenedModificationsStack; // Objects are NSMutableArray
NSMutableArray *_deferredDidChangeKeysStack; // Objects are NSMutableArray
NSMutableArray *_rollbackValuesStack; // Objects are NSMutableDictionary
BOOL _runningDeferredDidChangeKeys;
@@ -102,41 +103,60 @@ - (BOOL)insideModificationBlock;
- (BOOL)modifyWithBlock:(RKModificationBlock)modBlock;
{
- [self beginModifications];
- BOOL success = modBlock(self);
- [self commitModificationsKeepingChanges:success];
- return success;
+ RKDocument *strongDocument = self.document;
+ if (strongDocument) {
+ // Our document applies and tracks all modifications
+ NSString *keyPathFromRoot = [self keyPathFromRootCollection];
+ RKModificationBlock rootRelativeBlock = [modBlock copy];
+ if (keyPathFromRoot) {
+ rootRelativeBlock = [^(RKMutableDictionary *root) {
+ return modBlock([root valueForKeyPath:keyPathFromRoot]);
+ } copy];
+ }
+
+ return [strongDocument modifyWithBlock:rootRelativeBlock];
+
+ } else {
+ // No document; apply the modification ourselves
+ [self beginModifications];
+ BOOL success = modBlock(self);
+ [self commitModificationsKeepingChanges:success];
+ return success;
+ }
}
- (void)beginModifications;
{
if (_modificationDepth++ == 0) {
+ _valuesWithOpenedModificationsStack = [NSMutableArray array];
_deferredDidChangeKeysStack = [NSMutableArray array];
_rollbackValuesStack = [NSMutableArray array];
}
+ NSMutableArray *openedModifications = [NSMutableArray array];
+ [_valuesWithOpenedModificationsStack addObject:openedModifications];
[_deferredDidChangeKeysStack addObject:[NSMutableArray array]];
[_rollbackValuesStack addObject:[NSMutableDictionary dictionary]];
[self enumerateKeysAndObjectsUsingBlock:^(NSString *key, id obj, BOOL *stop) {
- if ([obj conformsToProtocol:@protocol(RKCollection)])
+ if ([obj conformsToProtocol:@protocol(RKCollection)]) {
+ [openedModifications addObject:obj];
[obj beginModifications];
+ }
}];
}
- (void)commitModificationsKeepingChanges:(BOOL)keepChanges;
{
NSParameterAssert(_modificationDepth > 0);
-#warning Handle collections that came/went, syncing their mod depth or something
- [self enumerateKeysAndObjectsUsingBlock:^(NSString *key, id obj, BOOL *stop) {
- if ([obj conformsToProtocol:@protocol(RKCollection)])
- [obj commitModificationsKeepingChanges:keepChanges];
- }];
-
+ NSArray *modificationsToClose = [_valuesWithOpenedModificationsStack lastObject];
NSArray *deferredDidChangeKeys = [_deferredDidChangeKeysStack lastObject];
NSDictionary *rollbackValues = [_rollbackValuesStack lastObject];
+ for (id <RKCollection> collection in modificationsToClose)
+ [collection commitModificationsKeepingChanges:keepChanges];
+
if (!keepChanges) {
for (NSString *key in [NSSet setWithArray:deferredDidChangeKeys])
[self setPrimitiveValue:[rollbackValues valueForKey:key] forKey:key];
@@ -148,6 +168,7 @@ - (void)commitModificationsKeepingChanges:(BOOL)keepChanges;
}];
_runningDeferredDidChangeKeys = NO;
+ [_valuesWithOpenedModificationsStack removeLastObject];
[_deferredDidChangeKeysStack removeLastObject];
[_rollbackValuesStack removeLastObject];
@@ -163,21 +184,16 @@ - (id)valueForKey:(NSString *)key;
- (void)setValue:(id)value forKey:(NSString *)key;
{
NSParameterAssert([key isKindOfClass:[NSString class]]);
- RKDocument *strongDocument = self.document;
- if ([self insideModificationBlock] || !strongDocument) {
+ if ([self insideModificationBlock]) {
id preparedValue = [self prepareValue:value forKey:key];
[self willChangeValueForKey:key];
[self setPrimitiveValue:preparedValue forKey:key];
[self didChangeValueForKey:key];
- } else /* (![self insideModificationBlock] && strongDocument) */ {
- // Translate request into a modification block, if we have a document
- // Modification block does a root-relative -setValue:forKeyPath:
-
- NSString *rootRelative = joinKeyPath(self.keyPathFromRootCollection, key);
- RKModificationBlock rootRelativeModBlock = [strongDocument modificationBlockToSetValue:value forRootKeyPath:rootRelative];
- BOOL success = [strongDocument modifyWithBlock:rootRelativeModBlock];
+ } else {
+ // Translate request into a modification block
+ BOOL success = [self modifyWithBlock:[self modificationBlockToSetValue:value forKey:key]];
if (!success)
[NSException raise:NSInternalInconsistencyException format:@"Failed to apply immediate modification block in -[RKMutableDictionary setValue:%@ forKey:%@]. This is either a logic error, or the dictionary was modified concurrently (which is illegal).", value, key];
@@ -186,6 +202,7 @@ - (void)setValue:(id)value forKey:(NSString *)key;
- (void)setPrimitiveValue:(id)preparedValue forKey:(NSString *)key;
{
+ NSAssert1([self insideModificationBlock], @"%s must only be called from inside a modification block", __func__);
[_backingDictionary setValue:preparedValue forKey:key];
}
@@ -219,4 +236,25 @@ - (void)didChangeValueForKey:(NSString *)key;
[super didChangeValueForKey:key];
}
+
+#pragma mark RKMutableDictionary
+
+- (RKModificationBlock)modificationBlockToSetValue:(id)newValue forKey:(NSString *)key;
+{
+ id oldValue = [self valueForKey:key];
+
+ // If unchanged, always succeed (and avoid capturing unneeded values)
+ if ((oldValue == newValue) || [oldValue isEqual:newValue])
+ return [^BOOL(RKMutableDictionary *localSelf) { return YES; } copy];
+
+ return [^BOOL(RKMutableDictionary *localSelf) {
+ id curValue = [localSelf valueForKey:key];
+ if ((curValue == oldValue) || [curValue isEqual:oldValue]) {
+ [localSelf setValue:newValue forKey:key];
+ return YES;
+ }
+ return (curValue == newValue) || [curValue isEqual:newValue];
+ } copy];
+}
+
@end
@@ -86,45 +86,4 @@ - (void)testKVCChangeCapture;
STAssertTrue([secondRev compare:firstRev] == NSOrderedDescending, @"New revision should compare later than first");
}
-- (void)testGeneratedModificationBlocks;
-{
- RKDocument *doc = [[RKDocument alloc] initWithIdentifier:nil];
- [doc.root setValue:@"Freddie" forKey:firstNameKey];
- [doc.root setValue:@"Mercury" forKey:lastNameKey];
-
- RKModificationBlock setFreddieToFarrokh = [doc modificationBlockToSetValue:@"Farrokh" forRootKeyPath:firstNameKey];
- STAssertEqualObjects([doc.root valueForKey:firstNameKey], @"Freddie", @"Generating modification block shouldn't have side effects");
-
- BOOL modResult = [doc modifyWithBlock:setFreddieToFarrokh];
- STAssertTrue(modResult, @"Modification should succeed with old value");
- STAssertEqualObjects([doc.root valueForKey:firstNameKey], @"Farrokh", @"Modification block should alter value");
-
- // Keep firstName as @"Farrokh"
- modResult = [doc modifyWithBlock:setFreddieToFarrokh];
- STAssertTrue(modResult, @"Modification should succeed if already at new value");
- STAssertEqualObjects([doc.root valueForKey:firstNameKey], @"Farrokh", @"Modification block should alter value");
-
- [doc.root setValue:@"completelyDifferent" forKey:firstNameKey];
- modResult = [doc modifyWithBlock:setFreddieToFarrokh];
- STAssertFalse(modResult, @"Modification should fail if value is neither old nor new");
- STAssertEqualObjects([doc.root valueForKey:firstNameKey], @"completelyDifferent", @"Failed modification block should not change collection");
-
- [doc.root setValue:nil forKey:firstNameKey];
- modResult = [doc.root modifyWithBlock:setFreddieToFarrokh];
- STAssertFalse(modResult, @"Modification should fail if value is neither old nor new");
- STAssertNil([doc.root valueForKey:firstNameKey], @"Failed modification block should not change collection");
-
- [doc.root setValue:@"Freddie" forKey:firstNameKey];
- RKModificationBlock noRealChange = [doc modificationBlockToSetValue:@"Freddie" forRootKeyPath:firstNameKey];
- [doc.root setValue:@"completelyDifferent" forKey:firstNameKey];
- modResult = [doc.root modifyWithBlock:noRealChange];
- STAssertTrue(modResult, @"Any modification should succeed if setter was identity");
- STAssertEqualObjects([doc.root valueForKey:firstNameKey], @"completelyDifferent", @"Any modification should succeed if setter was identity");
-
- [doc.root setValue:nil forKey:firstNameKey];
- modResult = [doc.root modifyWithBlock:noRealChange];
- STAssertTrue(modResult, @"Any modification should succeed if setter was identity");
- STAssertTrue([doc.root count] == 1, @"Any modification should succeed if setter was identity");
-}
-
@end
@@ -15,5 +15,6 @@
- (void)testCreation;
- (void)testModificationBlocks;
- (void)testDictionaryHierarchy;
+- (void)testGeneratedModificationBlocks;
@end
@@ -171,6 +171,45 @@ - (void)testDictionaryHierarchy;
STAssertEqualObjects([parent valueForKeyPath:CHILD"."CHILD"."FN], @"Susan", nil);
}
-// TODO: Check values are copied (if funneled through -setValue:forKey:)
+- (void)testGeneratedModificationBlocks;
+{
+ RKMutableDictionary *dict = [[RKMutableDictionary alloc] init];
+ [dict setValue:@"Freddie" forKey:FN];
+ [dict setValue:@"Mercury" forKey:LN];
+
+ RKModificationBlock setFreddieToFarrokh = [dict modificationBlockToSetValue:@"Farrokh" forKey:FN];
+ STAssertEqualObjects([dict valueForKey:FN], @"Freddie", @"Generating modification block shouldn't have side effects");
+
+ BOOL modResult = [dict modifyWithBlock:setFreddieToFarrokh];
+ STAssertTrue(modResult, @"Modification should succeed with old value");
+ STAssertEqualObjects([dict valueForKey:FN], @"Farrokh", @"Modification block should alter value");
+
+ // Keep firstName as @"Farrokh"
+ modResult = [dict modifyWithBlock:setFreddieToFarrokh];
+ STAssertTrue(modResult, @"Modification should succeed if already at new value");
+ STAssertEqualObjects([dict valueForKey:FN], @"Farrokh", @"Modification block should alter value");
+
+ [dict setValue:@"completelyDifferent" forKey:FN];
+ modResult = [dict modifyWithBlock:setFreddieToFarrokh];
+ STAssertFalse(modResult, @"Modification should fail if value is neither old nor new");
+ STAssertEqualObjects([dict valueForKey:FN], @"completelyDifferent", @"Failed modification block should not change collection");
+
+ [dict setValue:nil forKey:FN];
+ modResult = [dict modifyWithBlock:setFreddieToFarrokh];
+ STAssertFalse(modResult, @"Modification should fail if value is neither old nor new");
+ STAssertNil([dict valueForKey:FN], @"Failed modification block should not change collection");
+
+ [dict setValue:@"Freddie" forKey:FN];
+ RKModificationBlock noRealChange = [dict modificationBlockToSetValue:@"Freddie" forKey:FN];
+ [dict setValue:@"completelyDifferent" forKey:FN];
+ modResult = [dict modifyWithBlock:noRealChange];
+ STAssertTrue(modResult, @"Any modification should succeed if setter was identity");
+ STAssertEqualObjects([dict valueForKey:FN], @"completelyDifferent", @"Any modification should succeed if setter was identity");
+
+ [dict setValue:nil forKey:FN];
+ modResult = [dict modifyWithBlock:noRealChange];
+ STAssertTrue(modResult, @"Any modification should succeed if setter was identity");
+ STAssertTrue([dict count] == 1, @"Any modification should succeed if setter was identity");
+}
@end

0 comments on commit 6dfbdad

Please sign in to comment.