search

facebookarchive / KVOController

Simple, modern, thread-safe key-value observing for iOS and OS X.
Other
7.34k stars 925 forks source link

Custom NSKeyValueObservingOptions to add FBKVONotificationKeyPathKey #126

Open drkibitz opened 7 years ago

drkibitz commented 7 years ago

This PR might be a little "iffy", but based on a comment I made here, I thought I'd give it a shot.

My reasoning (as in that comment), is that I'm a little skittish in updating from my hash I am using for this project, as I use KVO a lot, and wondering if creating mutable copies of the change dictionary for a particular use case impacts performance measurably for someone like me, and if it is necessary for many use cases like mine (I haven't needed this yet in my usage).

The "iffy" part of course is adding a new option to an existing Foundation enum. Though I actually don't mind it all that much, and does match the functionalities of the other options, in that they add new entries to the change dictionary as well. I also made the option the default when observing more than one keyPath, though this may be problematic, well maybe this whole PR, but still looking forward to some feedback anyway. Thanks in advance!

nlutsenko commented 7 years ago

@drkibitz This change looks great, with one exception: extending an existing bitmask. If at any given point in time - Apple is going to add another value that overlaps with our custom ones - we are very likely to be "screwed", since this is going to violate compatibility between the old vs new SDKs and produce crazy and undefined results.

I am very curious about exploring other possibilities to accomplish the same...

The easiest solution that comes to my mind: have another bitmask of options and a set of methods to support them? Say, if you want to precisely tune the optimizations - you use the new set of methods, that also includes using the new bitmask with our own pre-defined values.

Thoughts? Anything else that we can use to achieve the same? (going to request changes on this PR for now, since stability is outmost priority)

drkibitz commented 7 years ago

@nlutsenko 4 ideas, the first I almost submitted as this PR, but went with what you already reviewed instead 😉

  1. Just do the mutable copy and add the key for methods that deal with multiple paths. This would mean adding an intermediary private method that would accept a flag for whether to add the key to the dictionary.
  2. Similar to the first idea, but just add a public parameter for a single BOOL flag of whether to add the key for the methods dealing with multiple keysPaths, maybe these are new methods that are invoked with a YES internally by the current public methods.
  3. Like what you said, make a new bitmask, duplicating the functionality of the foundation one, but with the addition of the new one. Make all current public methods take this new bitmask instead.
  4. Also like what you said. Make a new bitmask, but only related to things specific to KVOController, which right now would be one flag. Make new methods that accept all the parameters that current members do, but with one extra for customOptions: or something like that. Internally current methods can call this method with the new flag turned on automatically.

Ok 5, which can be a combination of 1 and 4. Meaning new methods, but only current public methods that deal with multiple keyPaths internally pass the custom option flag turned on, while methods dealing with a single keyPath do not bother and just leave it off.

What do you think?

drkibitz commented 7 years ago

@nlutsenko ping, for previous comment