From 8889e7ed81631cbbb269b2a7915bd5b531fd2663 Mon Sep 17 00:00:00 2001 From: Hugo Melder Date: Tue, 4 Feb 2025 07:34:14 +0100 Subject: [PATCH 1/6] NSKVOSupport: Implement legacy KVO API Implements the setKeys:triggerChangeNotificationsForDependentKey: class method. Please do not use it. It is fundamentally broken, and requires the object's meta class to hold additional state. Keys from this class method are the last resort when retrieving dependencies via keyPathsForValuesAffectingValueForKey:. This aligns with the implementation in Foundation. --- Source/NSKVOSupport.m | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/Source/NSKVOSupport.m b/Source/NSKVOSupport.m index 2b5f7a381..f3bbd6a2e 100644 --- a/Source/NSKVOSupport.m +++ b/Source/NSKVOSupport.m @@ -569,9 +569,29 @@ - (bool) isEmpty #pragma region KVO Core Implementation - NSObject category +static const char *const KVO_MAP = "_NSKVOMap"; + @implementation NSObject (NSKeyValueObserving) ++ (void) setKeys: (NSArray *) triggerKeys +triggerChangeNotificationsForDependentKey: (NSString *) dependentKey +{ + NSMutableDictionary *affectingKeys; + NSSet *triggerKeySet; + + affectingKeys = objc_getAssociatedObject(self, KVO_MAP); + if (nil == affectingKeys) + { + affectingKeys = [NSMutableDictionary dictionaryWithCapacity: 10]; + objc_setAssociatedObject(self, KVO_MAP, affectingKeys, + OBJC_ASSOCIATION_RETAIN); + } + + triggerKeySet = [NSSet setWithArray: triggerKeys]; + [affectingKeys setValue: triggerKeySet forKey: dependentKey]; +} + - (void) observeValueForKeyPath: (NSString *)keyPath ofObject: (id)object change: (NSDictionary *)change @@ -631,6 +651,7 @@ + (NSSet *) keyPathsForValuesAffectingValueForKey: (NSString *)key static NSSet *emptySet = nil; static gs_mutex_t lock = GS_MUTEX_INIT_STATIC; NSUInteger keyLength; + NSDictionary *affectingKeys; if (nil == emptySet) { @@ -701,6 +722,20 @@ + (NSSet *) keyPathsForValuesAffectingValueForKey: (NSString *)key { return [self performSelector:sel]; } + + // We compute an NSSet from information provided by previous invocations + // of the now-deprecated setKeys:triggerChangeNotificationsForDependentKey: + // if the original imp returns an empty set. + // This aligns with Apple's backwards compatibility. + affectingKeys = (NSDictionary *)objc_getAssociatedObject(self, KVO_MAP); + if (unlikely(nil != affectingKeys)) + { + NSSet *set = [affectingKeys objectForKey:key]; + if (set != nil) + { + return set; + } + } } return emptySet; } From d1812ef711143c2ef824969e4b3d88ea00b92094 Mon Sep 17 00:00:00 2001 From: Hugo Melder Date: Tue, 4 Feb 2025 07:37:25 +0100 Subject: [PATCH 2/6] NSKVOSupport: Unit tests for the legacy API --- Tests/base/NSKVOSupport/legacy.m | 315 +++++++++++++++++++++++++++++++ 1 file changed, 315 insertions(+) create mode 100644 Tests/base/NSKVOSupport/legacy.m diff --git a/Tests/base/NSKVOSupport/legacy.m b/Tests/base/NSKVOSupport/legacy.m new file mode 100644 index 000000000..5cb3625a4 --- /dev/null +++ b/Tests/base/NSKVOSupport/legacy.m @@ -0,0 +1,315 @@ +#import + +#import +#import +#import +#import +#import +#import + +#import "Testing.h" + +@interface Observee : NSObject +{ + NSString *_firstName; + NSString *_middleName; + NSString *_lastName; +} + +- (NSString *) firstName; +- (void) setFirstName: (NSString *)name; + +- (NSString *) middleName; +- (void) setMiddleName: (NSString *)name; + +- (NSString *) lastName; +- (void) setLastName: (NSString *)name; + +- (NSString *) shortFullName; +- (NSString *) fullName; + +@end + +@interface ObserveeMixed : Observee +{ + BOOL _trigger; +} + +- (BOOL) trigger; +- (void) setTrigger: (BOOL) trigger; + +@end + +@implementation Observee + +- (instancetype)init { + self = [super init]; + _firstName = _middleName = _lastName = @""; + return self; +} + +- (NSString *) firstName +{ + return _firstName; +} +- (void) setFirstName: (NSString *)name +{ + ASSIGN(_firstName, name); +} +- (NSString *) middleName +{ + return _middleName; +} +- (void) setMiddleName: (NSString *)name +{ + ASSIGN(_middleName, name); +} +- (NSString *) lastName +{ + return _lastName; +} +- (void) setLastName: (NSString *)name +{ + ASSIGN(_lastName, name); +} + +- (NSString *) shortFullName { +return [NSString stringWithFormat: @"%@ %@", _firstName, _lastName]; +} + +- (NSString *) fullName { +return [NSString stringWithFormat: @"%@ %@ %@", _firstName, _middleName, _lastName]; +} + +@end + +@implementation ObserveeMixed + +- (BOOL) trigger +{ + return _trigger; +} +- (void) setTrigger: (BOOL) trigger +{ + _trigger = trigger; +} + +// We expect this function to have priority over the legacy API ++ (NSSet *)keyPathsForValuesAffectingFullName +{ + return [NSSet setWithObject:@"trigger"]; +} + +@end + +@interface Observer : NSObject +@end + +@implementation Observer +{ + @public + NSString *lastKeyPath; + id lastObject; + NSDictionary *lastChange; + int notificationCount; +} + +- (void) resetValues +{ + lastKeyPath = nil; + lastObject = nil; + lastChange = nil; +} + +- (void) resetCounter +{ + notificationCount = 0; +} + +- (void)observeValueForKeyPath:(NSString *)keyPath + ofObject:(id)object + change:(NSDictionary *)change + context:(void *)context +{ + notificationCount += 1; + lastKeyPath = keyPath; + lastObject = object; + lastChange = change; +} + +@end + +void simpleDependency(void) +{ + Observer *o = [Observer new]; + Observee *e = [Observee new]; + + NSArray *keys = [NSArray arrayWithObjects: @"firstName", @"lastName", nil]; + + [Observee setKeys: keys + triggerChangeNotificationsForDependentKey:@"fullName"]; + + NSSet *s = [Observee keyPathsForValuesAffectingValueForKey:@"fullName"]; + NSSet *expectedSet = [NSSet setWithArray: keys]; + PASS_EQUAL(s, expectedSet, "'keyPathsForValuesAffectingValueForKey:' returns the correct values affecting 'fullName'"); + + NSKeyValueObservingOptions opts = + NSKeyValueObservingOptionOld | NSKeyValueObservingOptionNew; + + [e addObserver:o forKeyPath:@"fullName" options:opts context:NULL]; + + [e setFirstName:@"Hey"]; + + PASS_EQUAL(o->lastKeyPath, @"fullName", "last keypath is 'fullName'"); + PASS_EQUAL(o->lastObject, e, "last change object is correct"); + PASS(o->lastChange != nil, "last change is not nil"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeNewKey], @"Hey ", "new entry in change dict correct"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeOldKey], @" ", "old entry in change dict correct"); + PASS(o->notificationCount == 1, "notification count is 1"); + + [e setMiddleName:@"Not"]; // no change notification + + [e setLastName:@"You"]; + PASS_EQUAL(o->lastKeyPath, @"fullName", "last keypath is 'fullName'"); + PASS_EQUAL(o->lastObject, e, "last change object is correct"); + PASS(o->lastChange != nil, "last change is not nil"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeNewKey], @"Hey Not You", "new entry in change dict correct"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeOldKey], @"Hey Not ", "old entry in change dict correct"); + PASS(o->notificationCount == 2, "notification count is 2"); + + [e removeObserver:o forKeyPath:@"fullName"]; + + [o release]; + [e release]; +} + +void registeringMultipleDependencies(void) { + Observer *o; + Observee *e; + NSArray *arr; + NSSet *s; + + o = [Observer new]; + e = [Observee new]; + + arr = [NSArray arrayWithObject: @"firstName"]; + [Observee setKeys:arr + triggerChangeNotificationsForDependentKey:@"fullName"]; + s = [Observee keyPathsForValuesAffectingValueForKey:@"fullName"]; + PASS_EQUAL(s, [NSSet setWithArray: arr], "expecting 'firstName' as affecting key for 'fullName'"); + + arr = [NSArray arrayWithObject: @"middleName"]; + [Observee setKeys: arr + triggerChangeNotificationsForDependentKey:@"fullName"]; + s = [Observee keyPathsForValuesAffectingValueForKey:@"fullName"]; + PASS_EQUAL(s, [NSSet setWithArray: arr], "expecting 'middleName' as affecting key for 'fullName'"); + + arr = [NSArray arrayWithObjects: @"firstName", @"lastName", nil]; + [Observee setKeys:arr + triggerChangeNotificationsForDependentKey:@"shortFullName"]; + s = [Observee keyPathsForValuesAffectingValueForKey:@"shortFullName"]; + PASS_EQUAL(s, [NSSet setWithArray: arr], "expecting 'firstName' and 'lastName' as affecting key for 'fullName'"); + + NSKeyValueObservingOptions opts = + NSKeyValueObservingOptionOld | NSKeyValueObservingOptionNew; + + [e addObserver:o forKeyPath:@"fullName" options:opts context:NULL]; + + [e setFirstName:@"Hey"]; + PASS(o->notificationCount == 0, "no change notification received when modifying firstName"); + + [e setMiddleName:@"Not"]; + PASS_EQUAL(o->lastKeyPath, @"fullName", "last keypath is 'fullName'"); + PASS_EQUAL(o->lastObject, e, "last change object is correct"); + PASS(o->lastChange != nil, "last change is not nil"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeNewKey], @"Hey Not ", "new entry in change dict correct"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeOldKey], @"Hey ", "old entry in change dict correct"); + PASS(o->notificationCount == 1, "change notification received when modifying middleName"); + + [e setLastName:@"You"]; + PASS(o->notificationCount == 1, "no change notification received when modifying lastName"); + + [e removeObserver:o forKeyPath:@"fullName"]; + [o resetCounter]; + [o resetValues]; + + [e addObserver:o forKeyPath:@"shortFullName" options:opts context:NULL]; + + [e setFirstName:@"Hello"]; + PASS_EQUAL(o->lastKeyPath, @"shortFullName", "last keypath is 'shortFullName'"); + PASS_EQUAL(o->lastObject, e, "last change object is correct"); + PASS(o->lastChange != nil, "last change is not nil"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeNewKey], @"Hello You", "new entry in change dict correct"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeOldKey], @"Hey You", "old entry in change dict correct"); + PASS(o->notificationCount == 1, "change notification received when modifying firstName"); + + [e setMiddleName:@"Not"]; + PASS(o->notificationCount == 1, "no change notification received when modifying middleName"); + + [o resetValues]; + + [e setLastName:@"World"]; + PASS_EQUAL(o->lastKeyPath, @"shortFullName", "last keypath is 'shortFullName'"); + PASS_EQUAL(o->lastObject, e, "last change object is correct"); + PASS(o->lastChange != nil, "last change is not nil"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeNewKey], @"Hello World", "new entry in change dict correct"); + PASS_EQUAL([o->lastChange valueForKey: NSKeyValueChangeOldKey], @"Hello You", "old entry in change dict correct"); + PASS(o->notificationCount == 2, "change notification received when modifying lastName"); + + [e removeObserver:o forKeyPath:@"shortFullName"]; + + [o release]; + [e release]; +} + +void mixedLegacy(void) +{ + Observer *o = [Observer new]; + ObserveeMixed *e = [ObserveeMixed new]; + + NSArray *keys = [NSArray arrayWithObjects: @"firstName", @"lastName", nil]; + + [ObserveeMixed setKeys:keys + triggerChangeNotificationsForDependentKey:@"fullName"]; + + NSSet *s = [ObserveeMixed keyPathsForValuesAffectingValueForKey:@"fullName"]; + NSSet *expected = [NSSet setWithObject:@"trigger"]; + PASS_EQUAL(s, expected, "newer API has precedence over deprecated API"); + + + NSKeyValueObservingOptions opts = + NSKeyValueObservingOptionOld | NSKeyValueObservingOptionNew; + + [e addObserver:o forKeyPath:@"fullName" options:opts context:NULL]; + + // No trigger + + [e setFirstName:@"Hey"]; + [e setMiddleName:@"Not"]; + [e setLastName:@"You"]; + PASS(o->notificationCount == 0, "no change notification from either firstName, middleName, or lastName"); + + // Trigger + + [e setTrigger:YES]; + PASS(o->notificationCount == 1, "change notification from trigger"); + + [e removeObserver:o forKeyPath:@"fullName"]; + [o release]; + [e release]; +} + + +int +main(int argc, char *argv[]) +{ + START_SET("KVO Legacy Tests") + + simpleDependency(); + registeringMultipleDependencies(); + mixedLegacy(); + + END_SET("KVO Legacy Tests") + + return 0; +} From c76d0521921c4334107eeb8771e71ad1ff5b1391 Mon Sep 17 00:00:00 2001 From: Hugo Melder Date: Tue, 4 Feb 2025 07:46:34 +0100 Subject: [PATCH 3/6] Update ChangeLog --- ChangeLog | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ChangeLog b/ChangeLog index 5fd14e867..4cc76d591 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ +2025-02-04 Hugo Melder + + * Source/NSKVOSupport.m: + * Tests/base/NSKVOSupport/legacy.m: + Implements the setKeys:triggerChangeNotificationsForDependentKey: class + method. Please do not use it. It is fundamentally broken, and requires + the object's meta class to hold additional state. + Keys from this class method are the last resort when retrieving + dependencies via keyPathsForValuesAffectingValueForKey:. + This aligns with the implementation in Foundation. + 2025-01-04 Richard Frith-Macdonald * Headers/Foundation/NSRegularExpression.h: From 71c5bb8b094ae7791024c9fe272bb19f0beb2149 Mon Sep 17 00:00:00 2001 From: hmelder Date: Tue, 4 Feb 2025 15:06:54 +0800 Subject: [PATCH 4/6] GCC does not support ivars in implementation block --- Tests/base/NSKVOSupport/legacy.m | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/base/NSKVOSupport/legacy.m b/Tests/base/NSKVOSupport/legacy.m index 5cb3625a4..46299a661 100644 --- a/Tests/base/NSKVOSupport/legacy.m +++ b/Tests/base/NSKVOSupport/legacy.m @@ -103,9 +103,6 @@ + (NSSet *)keyPathsForValuesAffectingFullName @end @interface Observer : NSObject -@end - -@implementation Observer { @public NSString *lastKeyPath; @@ -113,6 +110,9 @@ @implementation Observer NSDictionary *lastChange; int notificationCount; } +@end + +@implementation Observer - (void) resetValues { From 2060863762a261646f3fd0f8c9f2ec41a49d357c Mon Sep 17 00:00:00 2001 From: hmelder Date: Tue, 4 Feb 2025 15:12:58 +0800 Subject: [PATCH 5/6] Mark as hopeful on GCC --- Tests/base/NSKVOSupport/legacy.m | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Tests/base/NSKVOSupport/legacy.m b/Tests/base/NSKVOSupport/legacy.m index 46299a661..5ddffd800 100644 --- a/Tests/base/NSKVOSupport/legacy.m +++ b/Tests/base/NSKVOSupport/legacy.m @@ -305,10 +305,18 @@ void mixedLegacy(void) { START_SET("KVO Legacy Tests") + #if defined(__GNUC__) + testHopeful = YES; + #endif + simpleDependency(); registeringMultipleDependencies(); mixedLegacy(); + #if defined(__GNUC__) + testHopeful = NO; + #endif + END_SET("KVO Legacy Tests") return 0; From 30a0efaeb05f299dffe50f379a68a863f896345b Mon Sep 17 00:00:00 2001 From: Hugo Melder Date: Mon, 17 Mar 2025 05:14:40 +0100 Subject: [PATCH 6/6] Mark setKeys:triggerKeys triggerChangeNotificationsForDependentKey: as deprecated --- Headers/Foundation/NSKeyValueObserving.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Headers/Foundation/NSKeyValueObserving.h b/Headers/Foundation/NSKeyValueObserving.h index b01c09eff..8396a821d 100644 --- a/Headers/Foundation/NSKeyValueObserving.h +++ b/Headers/Foundation/NSKeyValueObserving.h @@ -229,7 +229,7 @@ typedef NSString *NSKeyValueChangeKey; * they should also be sent for dependentKey. */ + (void) setKeys: (NSArray*)triggerKeys -triggerChangeNotificationsForDependentKey: (NSString*)dependentKey; +triggerChangeNotificationsForDependentKey: (NSString*)dependentKey __attribute__((deprecated)); #if OS_API_VERSION(MAC_OS_X_VERSION_10_5,GS_API_LATEST)