-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Monitor changes and update the widget in real time. #335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Use KVO to watch the UserDefaults object and update the view as appropriate.
| self, | ||
| forKeyPath: defaults.statusExtensionContextObservableKey, | ||
| options: [.new, .initial], | ||
| context: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's important to use a context here. Example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. The purpose of the context member was pretty opaque to me. I'm assuming it's some kind of semaphore that lets you know that the notification is relevant to this particular instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it legit to use the context variable (defined as var context: StatusExtensionContext?) for this purpose? It's convenient. If not, I'll create a new var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't, foremost because it's convoluting the term "context" for future readers, but secondly because you're looking for a unique pointer value here, and it should feel safer to instead use one containing a simple integer value that never changes.
| defaults.addObserver( | ||
| self, | ||
| forKeyPath: defaults.statusExtensionContextObservableKey, | ||
| options: [.new, .initial], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you aren't using the change dict in the observer than you should leave these blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you elaborate on that? Not sure what you mean by using the change dict in the observer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing the .new key here is so it is included in the observeValue call below. .initial means the observer is called right away, which isn't what you want.
| } | ||
|
|
||
| override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) { | ||
| update() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always check the context. If it doesn't match, then you must call super.
| subtitleLabel.textColor = UIColor.secondaryLabelColor | ||
| } | ||
|
|
||
| override func viewWillAppear(_ animated: Bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be called multiple times, and disappear can as well. These two aren't always balanced in the case of an interactive transition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that's good to know. I'll move these to viewDidLoad() and deinit, following the pattern in PredictionTableViewController
1. call addObserver() in viewDidLoad() 2. call removeObserver() in deinit 3. don't specify options in addObserver() 4. provide a context to the observer code
|
Ok - I think I covered all the issues you raised. Thanks for the guidance! I'm not sure how to properly test this in the simulator, because when I go to the lock screen, the Loop app isn't updating so I don't see any changes. Any thoughts on how to test it in the simulator? If not, I'll deploy it to my phone and just observe it for 5-10 minutes :-) |
| } | ||
|
|
||
| override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) { | ||
| if context != &self.context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early returns should really only exist in guard statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. fixed!
(see Carthage/Carthage#3019 for details)
Use KVO to watch the UserDefaults object and update the view as
appropriate. This is a first pass at resolving #320.