-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Create a lock screen widget (aka Today Extension) #281
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
Changes to Loop:
1. Created a TodayExtensionDataManager which keeps an eye on DeviceDataManager
and stores data in TodayExtensionContext
2. Added Loop to a shared app group
Changes Loop TodayExtension:
1. New target
2. Reuses the HUD view from StatusTableViewController
3. Deserializes TodayExtensionContext data from share app group defaults
4. Displays basic data (currently only the current BG) in the lock view
In addition, inject some debug data if we're running on a simulator so that we can easily test the experience.
- wrap the StackView in a UIView - set the dimensions properly - fix all constraint issues
Along the way, refactor the lastTempBasal visual calculations out of StatusViewController and into LoopManager so that it can be shared with TodayExtensionDataManager.
This makes the alert icon do the right thing. Do some refactors along the way to clean things up.
| var sensor: SensorDisplayable? | ||
| } | ||
|
|
||
| class TodayExtensionContext { |
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 class is trying to do more than one thing. On one hand you've got convenience methods on NSUserDefaults which are better in an extension UserDefaults. On the other hand you've got raw data serialization which belongs in struct that implements RawRepresentable.
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.
Also, it's unlikely this community will ever agree on a single interface, so this should be named more specifically to allow more Widgets in the future.
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've completely overhauled this class to separate out the functionality.
|
|
||
| var glucose: GlucoseContext? { | ||
| get { | ||
| if data["gcgv"] == nil { return 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.
Use a guard statement.
| if data["gcgv"] == nil { return nil } | ||
|
|
||
| var sensor: SensorDisplayableContext? = nil | ||
| if data["gcsv"] != 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.
Optional unwrapping is a much clearer approach. There should hardly ever be a case for using as!
| get { | ||
| return data["eg"] as? String | ||
| } | ||
| set(eg) { |
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.
Don't use cryptic variable names. You can drop "(eg)" and reference newValue in the implementation.
Loop TodayExtension/Info.plist
Outdated
| <key>CFBundlePackageType</key> | ||
| <string>XPC!</string> | ||
| <key>CFBundleShortVersionString</key> | ||
| <string>1.0</string> |
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 should match the container version. Version strings are set in concert using agvtool
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.
fixed.
Loop.xcodeproj/project.pbxproj
Outdated
| dstPath = ""; | ||
| dstSubfolderSpec = 13; | ||
| files = ( | ||
| 4F70C1E81DE8DCA7006380B7 /* Loop TodayExtension.appex in Embed App Extensions */, |
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.
Similar to the comment above, this should get a more specific name.
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.
fixed.
|
|
||
| var eventualGlucose: String? { | ||
| get { | ||
| return data["eg"] as? String |
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.
Don't use cryptic dictionary keys here. Match the property names exactly. I chose to use these on the watch dictionaries because they're being shuttled over a low-bandwidth Bluetooth connection.
| @IBOutlet weak var batteryHUD: BatteryLevelHUDView! | ||
| @IBOutlet weak var subtitleLabel: UILabel! | ||
|
|
||
| override func viewDidLoad() { |
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.
Remove template boilerplate here and below.
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.
fixed.
| } | ||
|
|
||
|
|
||
| completionHandler(NCUpdateResult.newData) |
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.
You need to call the completion handler in all paths.
| @@ -0,0 +1,334 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
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.
Per my comment on the issue, ideally these views aren't duplicated. You can create a new framework which contains a nib with these heirarchies and the view classes and share it between targets. This will allow use in future extension targets like notification content.
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.
Great - per our discussion I'm deferring this work and have created #284 to track progress.
|
Just because it's not documented in this thread, let me just say Thank You again for this awesome contribution! |
|
Thanks for the feedback, Nate! I'll go through and make point fixes, but a couple of general broader comments:
Also, FWIW - there's a bug in the way that it's saving context right now causing the app to crash.. so I wouldn't deploy it yet... :-) |
|
Re: 1. I think that's totally fine. Thanks for the courage to submit the PR and the humility to take the feedback from folks much more immersed in the ecosystem. Definitely helps everybody learn! Re: crash, hopefully my feedback helps to sort that out, and feel free to share if you're stuck :) I'm an arms-length away from deployment, but in my excitement I can't help but giving feedback, especially since I've had real-world with refactoring extension targets in the past. |
1. Move the store/retrieve semantics into a NSUserDefaults extension 2. Change TodayExtensionContext to implement RawRepresenable and generally follow the semantics in WatchContext 3. Fixed all cases where we were storing non-conformant data, which was resulting in serialization errors related: fixed a minor bug in TodayViewController where we were not calling completionHandler() reliably.
| class TodayExtensionContext { | ||
| var data: [String:Any] = [:] | ||
| let storage = UserDefaults(suiteName: "group.com.loudnate.Loop") | ||
| final class TodayExtensionContext: NSObject, RawRepresentable { |
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.
No need to extend NSObject. And this is probably more canonically a struct.
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.
FWIW, I modeled it on WatchContext:
final class WatchContext: NSObject, RawRepresentable {
I tried changing it to:
struct StatusExtensionContext: RawRepresentable {
In order to do that I had to create an empty init() in order to allow StatusExtensionDataManager to create a new, empty context. But then it complains that Cannot assign to property: 'context' is a 'let' constant whenever I tried to assign to values inside the context.
ideas?
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.
WatchContext used to implement NSSecureCoding, which required it to subclass NSObject as it was serialized to the filesystem. There's no need for that anymore.
Right, the downside of immutable structs is that you need to create a giant initializer. (likely, you'd have the client create each of the component structs and pass those to the initializer). let properties need to set at initialization time and not after.
| data["gcst"] = lgv?.sensor?.trendType | ||
| data["gcsl"] = lgv?.sensor?.isLocal | ||
| } | ||
| var latestGlucose: GlucoseContext? |
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.
Use let for all set-once properties throughout this file
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 tried that and get the following errors:
StatusExtensionContext.swift: Property 'self.latestGlucose' not initialized at super.init callStatusExtensionDataManager.swift: Cannot assign to property: 'latestGlucose' is a 'let' constant
ideas?
|
Ok - I believe I've addressed all the comments in your last code review. Thanks for the helpful dialog - I'm new to iOS development so I'm happy to take whatever guidance I can get! The main change (aside from renaming everything) was to overhaul the context class and separate out its functions while preserving a relatively clean API. FWIW, it seems like both context classes could usefully be replaced by protocol buffers or something similar. Also, what should I do about unit tests here? I'm not changing any core functionality but since the context class is hand rolled, I could see a unit test that verifies its functionality. I'm happy to make any further changes, regardless of how nitpicky. Let me know what you find! |
|
oh - and I'm also getting this warning for all framework extensions currently:
From some quick searching, it looks like we'd need to flip a bit on the frameworks, but I'm no expert on this. Happy to file an issue for this to track it if it's not something that we can change inside the extension itself. |
|
|
||
| final class StatusExtensionDataManager: NSObject { | ||
| unowned let dataManager: DeviceDataManager | ||
| private var lastContext: StatusExtensionContext? |
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.
Unused?
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.
yeah - vestigial. looking forward I should be keeping the last context for comparison purposes but I'll get rid of it for now.
|
|
||
| if let lastPoint = predictedGlucose?.last { | ||
| context.eventualGlucose = String( | ||
| format: NSLocalizedString("Eventually %@", comment: "The subtitle format describing eventual glucose. (1: localized glucose value description)"), |
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 should be done in either the view or view controller.
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'm struggling with this. My original idea was to pass the raw value through and let the extension turn it into a string, but that resulted in a lot of more complex objects (eg. HKQuantity) getting pushed through the context. My thinking was that the status extension can deal with a String blob so I'd turn it into a string here. Can't do that in a View/ViewController on the Loop side since I don't want the StatusExtensionDataManager to depend on any of the views. Hence, I'm doing it this way.
Open to ideas for how to proceed. For what it's worth, the current approach is flawed - I haven't figured out how to remove the double's precision from the resulting value. So right now I'm getting context strings like "Eventually 92.1283 mg/dL" on my actual device :-/
I've tried extending HKQuantity to give it a better description, but !@$@# HKQuantity doesn't let you have access to its underlying unit so you can't actually improve the description. Oy. Hoping you have ideas!
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 think maybe the hangup is the use of LoopKit's protocols in these context objects, e.g. ReservoirValue. Unless parts of LoopKit are refactored to support the limited API allowed in extensions, you'll need to remove all the custom framework dependencies from the widget extension.
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.
sorry, I didn't follow your last comment - not sure if it was a gihub glitch, or did you cross comment streams? I think your comment might have been in regard to the linker warning we're seeing, but this comment stream is about the eventual glucose string.
re: eventual glucose string, one approach is to pull apart the HKQuantity into a double value and a unit string. Problem with that is that I can't seem to find any way to extract the unit type from the HKQuantity. I'll go ahead and do that and hardcode it to mg/dL for now and we'll do the formatting on the far side in the ViewController.
| import CarbKit | ||
| import LoopKit | ||
|
|
||
| final class StatusExtensionDataManager: NSObject { |
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.
No need to subclass NSObject here.
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.
fixed.
| if let unitString = raw["latestGlucose_unit"] as? String, | ||
| let latestValue = raw["latestGlucose_value"] as? Double, | ||
| let startDate = raw["latestGlucose_startDate"] as? Date { | ||
| latestGlucose = GlucoseContext( |
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.
You could instead let each of these structs implement RawRepresentable, and replace the underscore namespacing with nested dictionaries.
It can sometimes be cleaner to read to use extensions to conform to protocols, rather than one long type definition, e.g.:
extension GlucoseContext: RawRepresentable {
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.
yeah, I toyed with that because I would very much like to use nested dictionaries (the stringified namespacing is not awesome). But then I wound up with less namespacing issues in the strings and more boilerplate code to implement RawRepresentable in each class. I figured I'd go for less code, but happy to do it with nested RawRepresentable dicts if you think it's the right way to go.
Entitlements files support build-setting substitutions, and accessing Info.plist can be done via the |
Loop/Loop.entitlements
Outdated
| <true/> | ||
| <key>com.apple.security.application-groups</key> | ||
| <array> | ||
| <string>group.com.loudnate.Loop</string> |
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.
Same here.
| extension UserDefaults { | ||
|
|
||
| private enum Key: String { | ||
| case StatusExtensionContext = "com.loudnate.Loop.StatusExtensionContext" |
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.
Please use com.loopkit.Loop for new keys.
| case StatusExtensionContext = "com.loudnate.Loop.StatusExtensionContext" | ||
| } | ||
|
|
||
| static func shared() -> UserDefaults? { |
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 should be a class var like standard is rather than a static function. Also, shared isn't descriptive enough IMO. group or appGroup would make more sense.
Loop/Managers/LoopDataManager.swift
Outdated
| } | ||
| } | ||
|
|
||
| func calculateNetBasalRate() -> (Double?, Double?, Date?) { |
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.
FIXME followup: This doesn't feel right as an instance method, and I don't particularly like calculate as a verb. It should also point out that its basing its information on lastTempBasal somehow, e.g. lastNetBasalRate as a calculated property?
I think the better path is to create a new value type (rather than the current tuple) to represent basal rates, and provide an instance method to do these transformations.
| @objc private func update(_ notification: Notification) { | ||
|
|
||
| self.dataManager.glucoseStore?.preferredUnit() { | ||
| (unit, error) in |
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.
unnecessary newline on 25/26?
Loop/Models/NetBasal.swift
Outdated
| import InsulinKit | ||
| import LoopKit | ||
|
|
||
| class NetBasal { |
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.
Struct would be preferred.
Loop/Models/NetBasal.swift
Outdated
| import LoopKit | ||
|
|
||
| class NetBasal { | ||
| var rate: Double |
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.
Use let
|
|
||
| class var appGroup: UserDefaults? { | ||
| get { | ||
| return UserDefaults(suiteName: "group.com.loudnate.Loop") |
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 still needs to reflect the correct group ID. Use the NSBundle APIs to access the Info.plist keys/values. Because you can't ask which bundle UserDefaults belongs to, each client will need to determine this using a bundle-specific class like a view controller.
Bundle(for: MyClass.Type).object(forInfoDictionaryKey: "AppropriateKey")Except I'd recommend creating a typed runtime property as an extension on Bundle to keep the key in one place, e.g. .appGroupSuiteName or something.
Of course that means this property now has to take an argument... sorry about that.
In addition, add the MAIN_APP_BUNDLE_IDENTIFIER into the Info.plist so that each target can access it.
|
ok - I think I've resolved all your recent comments. Thanks for the detailed explanation of how to access Info.plist variables from the code! One thing I did which might be slightly funky is that I created a 2nd Bundle extension in Loop to separate out the generally useful Bundle extensions (.shortVersionString, .bundleDisplayName, etc) from .appGroupSuiteName which has to be different for each target. I believe that this is necessary since otherwise I'd have two extensions setting the same variable (which I believe is a no-no) but happy to revise if this is not the right approach. Oh, and I also had to add MainAppBundleIdentifier to the Info.plist for Loop and Loop Status Extension since I couldn't find another variable to draw from. Done for now. Let me know what else I can change! |
|
Hm. Can you be a little more explicit? I'm trying to fill in the gaps here, but I'm not following you. Based on your last message, here's what I'm receiving:
Is that right? It seems a little convoluted to me, so I assume that this isn't what you intend. Happy to code it up any way that you want, but need a little more clarity first... From my perspective, it would be really helpful to me if you'd write out the specific APIs you'd like to see, and the files you'd like to see them in. thanks! |
loudnate
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.
I put some snippets inline. Extension management is definitely a complex topic; I'm sorry you had to break ground here on your own.
| import Foundation | ||
|
|
||
| extension Bundle { | ||
| static var appGroupSuiteName: String { |
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.
private var mainAppBundleIdentifier: String? {
return object(forInfoDictionaryKey: "MainAppBundleIdentifier") as? String
}
var appGroupSuiteName: String {
return "group.\(mainAppBundleIdentifier!)"
}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.
You can share this code between targets now (and it can go in the follow-up framework refactor later)
| case StatusExtensionContext = "com.loopkit.Loop.StatusExtensionContext" | ||
| } | ||
|
|
||
| class var appGroup: UserDefaults? { |
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.
You can strike this.
| } | ||
|
|
||
| func widgetPerformUpdate(completionHandler: (@escaping (NCUpdateResult) -> Void)) { | ||
| guard let context = UserDefaults.appGroup?.statusExtensionContext else { |
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.
let bundle = Bundle(for: type(of: self))
guard let context = UserDefaults(suiteName: bundle.appGroupSuiteName)?.statusExtensionContext else {
// ...| if error == nil, let unit = unit { | ||
| self.createContext(unit) { (context) in | ||
| if let context = context { | ||
| UserDefaults.appGroup?.statusExtensionContext = 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.
let bundle = Bundle(for: type(of: self)) // or Bundle.main might be cleaner
UserDefaults(suiteName: bundle.appGroupSuiteName)?.statusExtensionContext = context
Loop/Extensions/NSBundle+Loop.swift
Outdated
|
|
||
| import Foundation | ||
|
|
||
| extension Bundle { |
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.
Strike this file
Eliminate duplicate code where we're choosing the appropriate bundle inside the extension; instead let the caller choose the right bundle and have common code for .appGroupSuiteName.
|
thanks for the clear explanation. This is a much better approach than what I had before. I like that we're clearly separating the responsibility of accessing known values from UserDefaults from the responsibility of choosing the right bundle. One note: I used Bundle.main in both the main app and the extension since according to the docs it should always work: https://developer.apple.com/reference/foundation/bundle Ok - let me know what else you'd like to see! |
|
There's two open comments on how NetBasal should be an immutable value type, but after that things look good. Please make sure to test in both Debug and Release build configurations so no users are caught off-guard |
|
Oops - sorry about that. I committed the NetBasal fix to the wrong branch. Merged it in correctly now - let me know if you need further work there. What is the right way to test in Debug and Release build configs? The Loop scheme specifies Debug for the Run action and that's what I've generally been using. Should I temporarily switch to Release in that scheme or is there some better way? |
|
Run Product > Archive, then either Upload to the App Store for TestFlight
distribution or export and Ad Hoc build and install using the Devices
window.
There should be some folks in the Gitter channel who can help with this
step too.
…On Thu, Dec 8, 2016 at 6:21 PM Bharat Mediratta ***@***.***> wrote:
Oops - sorry about that. I committed the NetBasal fix to the wrong branch.
Merged it in correctly now - let me know if you need further work there.
What is the right way to test in Debug and Release build configs? The Loop
scheme specifies Debug for the Run action and that's what I've generally
been using. Should I temporarily switch to Release in that scheme or is
there some better way?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#281 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAM_jMhVOsSPSA2wd38XFGnF-pJxcI0Jks5rGLsWgaJpZM4K9Cm->
.
|
| } | ||
|
|
||
| struct SensorDisplayableContext: SensorDisplayable { | ||
| var isStateValid: 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.
These can be let
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.
fixed.
|
ok - successfully built an archive, exported it for an Ad Hoc build and deployed it to my iPhone 6+. The app seems to be working fine - but the extension seems to be working off of old data and not updating. I don't see a crash in the crash logs - and the Loop Status Extension doesn't show up in the process list, even though I'm attached to my device and I'm looking at the widget. Any thoughts about what's going on here? |
This requires a slight restructuring of the way we initialize GlucoseContext so that we create the SensorDisplayable first if it exists.
|
Hey team. I'm struggling to debug why this works fine for a Debug build but not a production build. I think at this point it makes more sense to merge it to dev and get more eyes on the problem. This would also free me up to do the follow-on refactors (ie, moving the shared views into their own framework, etc). What do you think? |
…281) * when pump manager is deleted, reset the basalDeliveryState * responses to code review * make bolus state optional * updated unit tests * using inactive instead of none for bolus state * bolus state not optionals, but use noBolus instead of none
Show the standard heads up display, in a widget, available on the lock screen.
The majority of the work is in a new "Today Extension" target. It's relatively straightforward, but some notes to assist in reading/reviewing:
ref: #276