NSKeyValueObserving: Implement setKeys:triggerChangeNotificationsForDependentKey:

[hmelder]

This pull request implements the deprecated class method setKeys:triggerChangeNotificationsForDependentKey: for creating dependencies in KVO.

It is fundamentally broken as it is tied to the meta class, and subclassing it will break dependencies added in the superclass. Additionally, it requires the object's meta class to hold additional state.

As described in keypathsforvaluesaffectingvaluef#discussion which is the replacement for the now deprecated setKeys:triggerChangeNotificationsForDependentKey::

The default implementation of this [keyPathsForValuesAffectingValueForKey:] method searches the receiving class for a method whose name matches the pattern +keyPathsForValuesAffecting, and returns the result of invoking that method if it is found. Any such method must return an NSSet. If no such method is found, an NSSet that is computed from information provided by previous invocations of the now-deprecated setKeys:triggerChangeNotificationsForDependentKey: method is returned, for backward binary compatibility.

This implementation is consistent with the one in Foundation.
However, it seems like the old GCC KVO implementation is not. Precidence of the newer APIs, and internal caching of values is broken. I've marked all new tests as hopeful, but we should definitely fix that.

[hmelder]

It would be good to create a conditional configuration flag for disabling all deprecated (possibly performance degrading) APIs. I'll make some benchmarks, as keyPathsForValuesAffectingValueForKey: can be a bottleneck for KVO.

[triplef]

It would be good to create a conditional configuration flag for disabling all deprecated (possibly performance degrading) APIs.

That would be great. WANT_DEPRECATED_KVC_COMPAT would be another candidate for this flag.

[hmelder]

@rfm could you take a look at it? :D

[rfm]

Arghy, I though't I'd reviewed this and that it had been committed weeks ago ... this means I've just made a buggy release of base with the implementation missing, so I need to make a bugfix release.
However, this branch now has additional changes we don't want in the release :-(
The process for deprecating stuff startd with a release marking the feature deprecated, and that hasn't been done, so it makes no sense to add conditional compilation stuff removing the method at this stage (if ever: we normally change the behavior of deprecated features with user defaults settings before removing them).

[rfm]

I think what's needed is to back-out the last three commits, and then (if, and only if, the alternative is fully working), mark
[setKeys:triggerChangeNotificationsForDependentKey:] as deprecated.
Once we have made a release deprecating a feature, we have about a year before removing that deprecated feature, durning which we ensure that any GNUstep code using it is updated and working without it, and we hope that anyone else using it has had enough time to update their code, before we make a release which removes it.

[gcasa]

It would be good to create a conditional configuration flag for disabling all deprecated (possibly performance degrading) APIs.

That would be great. WANT_DEPRECATED_KVC_COMPAT would be another candidate for this flag.

I'm wondering why we need another flag for this. We already have a flag to use the old implementation of KVO.

[rfm]

I'm wondering why we need another flag for this. We already have a flag to use the old implementation of KVO.
That's a different issue. The point of WANT_DEPRECATED_KVC_COMPAT is to conditionally remove code which inherently makes the system run slow ... as a performance optimisation.

I don't approve of that approach though: the correct way to deal with a structural problem is to change the code that depends on it (so it doesn't depend on it any more), deprecate it, and remove it either completely or by disabling at runtime.

We have a standard mechanism for the case where we want to keep stuff in the codebase for OSX compatibility even though we think it's bad: the GSMacOSXCompatible user default, which can be tested (so efficiently that it imposes no significant runtime overhead) with GSPrivateDefaultsFlag(GSMacOSXCompatible).

[triplef]

However, this branch now has additional changes we don't want in the release :-(

You could check out the commit before the additional changes (I guess 2060863) and merge that as a quick fix.

[rfm]

Good point, I will do that.

…

On 17 Feb 2025 at 07:49, Frederik Seiffert ***@***.***> wrote: > > > However, this branch now has additional changes we don't want in the release :-( > > You could check out the commit before the additional changes (I guess 2060863) and merge that as a quick fix. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***> triplef left a comment (gnustep/libs-base#489) > > > However, this branch now has additional changes we don't want in the release :-( > > You could check out the commit before the additional changes (I guess 2060863) and merge that as a quick fix. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>