-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Listen for notification when Insulin Delivery Store updates its cache #1150
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
ps2
left a comment
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.
Looks good. Just a small comment change request, and please update from dev and then tests should pass for this.
Thanks!
Loop/Managers/LoopDataManager.swift
Outdated
| queue: OperationQueue.main | ||
| ) { (note) in | ||
| self.dataAccessQueue.async { | ||
| self.logger.default("Received notification of cached dosing changing") |
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.
"cached dosing" -> "insulin delivery store cache"
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.
✅ 11a1a03
|
Doing a little cleanup on names: |
| self.notify(forChange: .bolus) | ||
| } | ||
| }, | ||
| NotificationCenter.default.addObserver( |
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.
The addObserver above this is not specifying a name for notifications from doseStore. Could you add the right name? I believe it is DoseStore.valuesDidChange.
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.
Yep I’ll probably be able to do that tomorrow night or this weekend.
|
This can be merged if it is rebased on dev and updated with the new notification name. |
This addresses #1037 and requires changes in LoopKit/LoopKit#304