Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

Custom NSKeyValueObservingOptions to add FBKVONotificationKeyPathKey #126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

drkibitz
Copy link

@drkibitz drkibitz commented Mar 7, 2017

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
Copy link
Contributor

@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)

Copy link
Contributor

@nlutsenko nlutsenko left a comment

Choose a reason for hiding this comment

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

Requesting per-discussion re: long-term stability.

@drkibitz
Copy link
Author

drkibitz commented Mar 27, 2017

@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
Copy link
Author

@nlutsenko ping, for previous comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants