[LOOP 949] Unify carb entry + bolus flow #41

mpangburn · 2020-01-08T21:18:09Z

Provide a dynamic predicted glucose chart on the bolus screen.
Require explicit confirmation to add a carb entry without bolusing.
Require user acknowledgement if a bolus recommendation goes stale while on the bolus screen, e.g. when a new glucose value arrives.
Refactor to support computing glucose predictions without adding a potential carb entry or bolus to their respective stores.

Note: The guts of CarbEntryViewController come right from LoopKit, with only a couple tweaks to navigation. Happy to make changes that come out of reviewing the other parts of this file, if deemed necessary.

ps2 · 2020-01-10T20:00:49Z

This is great. It both prevents double entry of carbs, and also, by delaying storage of entered carbs, makes the possibility of background automatic dosing while the screen is up much more rare. Those are two big mitigations. Automatic dosing can still happen when a new bg comes in, but it sounds like that is already covered.

A few UI notes:

Title for bolus screen should be "Meal Bolus" when following carb entry.
"Carb & Bolus Forecast" title can be removed; it shows not just those two, but all effects.
The sub-graph description text can be removed, I think. The first time this UI is used, the user is going to see that the graph is live.
The active carbohydrates and active insulin can be removed as well. Those were added to give some context, but the graph gives a higher level view that includes these, and more.

The primary Loop forecast graph and this one should both be changed to include undelivered, but scheduled, insulin (i.e. pending insulin) effects. The dosing calculation should still only include delivered insulin. This keeps the forecast more consistent in the various contexts it can be shown.

mpangburn · 2020-01-16T00:59:37Z

Title for bolus screen should be "Meal Bolus" when following carb entry.

Done

"Carb & Bolus Forecast" title can be removed; it shows not just those two, but all effects.

Done

The sub-graph description text can be removed, I think. The first time this UI is used, the user is going to see that the graph is live.

Done

The active carbohydrates and active insulin can be removed as well. Those were added to give some context, but the graph gives a higher level view that includes these, and more.

Done

The primary Loop forecast graph and this one should both be changed to include undelivered, but scheduled, insulin (i.e. pending insulin) effects. The dosing calculation should still only include delivered insulin.

Done

darinkrauss

A number of questions and nits, but nothing that would prevent approval. LGTM. 🚀

Note: I only reviewed the diff between CarbEntryViewController.swift and CarbEntryEditViewController.swift in LoopKit.

Loop/View Controllers/CarbEntryViewController.swift

Loop/View Controllers/CarbAbsorptionViewController.swift

Loop/Managers/LoopDataManager.swift

Loop/View Controllers/BolusViewController.swift

darinkrauss · 2020-01-20T22:38:58Z

Loop/View Controllers/BolusViewController.swift

-    var activeCarbohydratesDescription: String? = nil {
-        didSet {
-            activeCarbohydratesLabel?.text = activeCarbohydratesDescription
+    var maxBolus: Double = 25


[nit] Shouldn't maximum bolus allowed be from settings?

Perhaps optional until gathered from settings?

Preferable to have some default maximum in case the value isn't set in settings.

Loop/View Controllers/BolusViewController.swift

darinkrauss · 2020-01-20T22:53:21Z

Loop/View Controllers/BolusViewController.swift

        // This gets rid of the empty space at the top.
        tableView.tableHeaderView = UIView(frame: CGRect(x: 0, y: 0, width: tableView.bounds.size.width, height: 0.01))
+
+        glucoseChart.glucoseDisplayRange = HKQuantity(unit: .milligramsPerDeciliter, doubleValue: 60)...HKQuantity(unit: .milligramsPerDeciliter, doubleValue: 200)


Is this just a minimum display range? Or is this fixed? What if the actual prediction goes below 60 or above 200?

This range is a placeholder for when there is no data / data hasn't yet loaded. The actual range is determined dynamically in the chart implementation.

# Conflicts: # Loop/Managers/LoopDataManager.swift # Loop/View Controllers/CarbAbsorptionViewController.swift

ps2

Looks great to me!

ps2 · 2020-01-23T17:42:01Z

Loop/DefaultAssets.xcassets/Oval Selection.imageset/Contents.json

@@ -0,0 +1,53 @@
+{


What role is this asset serving? I see you've added it as a background to buttons in several places.

This was pulled over from LoopKit; it's the background shadow behind the selected absorption time emoji on the carb entry screen.

ps2 · 2020-01-28T15:39:40Z

This needs to be synced with the latest dev updates.

# Conflicts: # Loop/Base.lproj/Main.storyboard

mpangburn · 2020-01-29T04:10:25Z

This needs to be synced with the latest dev updates.

Done. Also cherry-picked your commit from DIY and updated the text of the secondary continue button.

ps2

Just one string that needs to be localized.

ps2 · 2020-01-29T22:14:00Z

Loop/View Controllers/CarbEntryViewController.swift

            title = NSLocalizedString("carb-entry-title-add", value: "Add Carb Entry", comment: "The title of the view controller to create a new carb entry")
        }
+
+        navigationItem.rightBarButtonItem = UIBarButtonItem(title: "Next", style: .plain, target: self, action: #selector(continueButtonPressed))


"Next" needs to be localized.

mpangburn · 2020-01-29T22:23:17Z

It looks like you may have reviewed an outdated commit; the string is localized.

ps2

LGTM!

andrew-dixon · 2020-02-13T19:10:57Z

We've been using this with my son via DIY Loop dev and have a few comments relating to the "Meal Bolus" screen on the UI that might improve it, making it even clearer for carers who might not be as familiar with it as a daily user.

Change the "Cancel" on the top right to "Back" and send the user back to the carb entry screen. This makes it clear to the user that the carb entry hasn't been entered. If they want out of the process altogether, they can then click the "Cancel" button on the carb entry screen.
When the bolus screen loads up the eye is immediately drawn to the "Save without Bolusing" button as it is high contrast and large. This has confused several of our caregivers into thinking Loop is suggesting no bolus. I would, therefore, recommend "greying out" this button, changing the text to "Deliver", and only making the button "active" once a bolus amount has been entered. If the entered amount is 0 (zero) then the text on the button could change to "Save without Bolusing" when it activates.

Hope that is helpful and clear. Thanks.

mpangburn added 5 commits December 22, 2019 15:13

Include glucose prediction in bolus controller

c4cfc7d

Unify carb entry + bolus flow

640cc26

Alert on bolus recommendation change

e2e9e6e

Recompute RC when considering past carb entry

472e3d3

Update comments re carb entry chart

86dd64a

mpangburn requested review from darinkrauss, nhamming and ps2 January 8, 2020 21:18

mpangburn changed the title ~~Unify carb entry + bolus flow~~ [LOOP 949] Unify carb entry + bolus flow Jan 15, 2020

mpangburn added 2 commits January 15, 2020 14:40

Visual updates per review

55d0618

Include pending insulin in prediction charts

f2df68b

Don't predict glucose with stale data

99c0a22

This was referenced Jan 19, 2020

Avoiding duplication of carb entries LoopKit/Loop#1012

Closed

Updated carb entry flow LoopKit/Loop#1244

Merged

darinkrauss approved these changes Jan 20, 2020

View reviewed changes

ps2 mentioned this pull request Jan 21, 2020

Update Loop to compile with LoopKit carb effects update #45

Merged

mpangburn added 3 commits January 21, 2020 14:30

Bolus recommendations include pending insulin

dc46f33

Updates from review

c8240a1

Merge branch 'dev' into carb-entry-flow

d9c1db3

# Conflicts: # Loop/Managers/LoopDataManager.swift # Loop/View Controllers/CarbAbsorptionViewController.swift

ps2 approved these changes Jan 23, 2020

View reviewed changes

mpangburn added 4 commits January 28, 2020 20:00

Merge branch 'dev' into carb-entry-flow

1343ae5

# Conflicts: # Loop/Base.lproj/Main.storyboard

Fix renaming of recencyInterval

fddb9e9

Visual tweaks from DIY

bc8ba38

Align continue button text

a302aaa

ps2 requested changes Jan 29, 2020

View reviewed changes

ps2 approved these changes Jan 29, 2020

View reviewed changes

mpangburn merged commit 065e01a into dev Jan 29, 2020

[LOOP 949] Unify carb entry + bolus flow #41

[LOOP 949] Unify carb entry + bolus flow #41

Uh oh!

Conversation

mpangburn commented Jan 8, 2020

Uh oh!

ps2 commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpangburn commented Jan 16, 2020

Uh oh!

darinkrauss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ps2 commented Jan 28, 2020

Uh oh!

mpangburn commented Jan 29, 2020

Uh oh!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpangburn commented Jan 29, 2020

Uh oh!

ps2 left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-dixon commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ps2 commented Jan 10, 2020 •

edited

Loading