Basal picker by ps2 · Pull Request #4 · tidepool-org/LoopKit

ps2 · 2019-02-27T05:18:10Z

A new basal schedule editor using a UIPickerView for modifying entries. Constrains user input to specified value and time increments, and prevents creation of invalid schedules. Also expected to help prevent decimal place errors. Back button is renamed "cancel" when changes are unsynced.

https://trello.com/c/IkRMs6mB/345-basal-schedule-entry-constraints

…mits

…r row height

darinkrauss

A number of comments. Overall I like the design. Not sure I agree with the separation of the whole number from fractional number in the picker though. It is very odd to go from 2.00 to 1.90. Was using a single number component too clunky going from 0.00 to 5.00 in 0.05 steps?

Oh, and there are some issues with leaving the picker open when tapping Edit or +.

LoopKitUI/UIColor.swift

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

darinkrauss · 2019-02-28T05:31:29Z

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

+
+    func validate() {
+        if delegate?.validateBasalScheduleEntryTableViewCell(self) == true
+        {


{ on preceding line? Plus, no need for == true.

The result of this expression is an optional. If there is no delegate for validation, we don't consider the value invalid.

darinkrauss · 2019-02-28T05:36:40Z

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

+            let time = startTimeForTimeComponent(row: row)
+            return stringForStartTime(time)
+        case .whole:
+            return valueNumberFormatter.string(from: Double(row))


wholeNumberFormatter?

darinkrauss · 2019-02-28T05:36:54Z

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

+        case .whole:
+            return valueNumberFormatter.string(from: Double(row))
+        case .fractional:
+            return valueNumberFormatter.string(from: Double(row) * minimumRateIncrement)


fractionalNumberFormatter?

darinkrauss · 2019-02-28T05:38:31Z

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

+    }
+
+    func pickerView(_ pickerView: UIPickerView, widthForComponent component: Int) -> CGFloat {
+        let w = pickerView.frame.size.width


w -> width?

darinkrauss · 2019-02-28T05:40:49Z

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

+        case .time:
+            return Int(round((maximumStartTime - minimumStartTime) / pickerInterval) + 1)
+        case .whole:
+            return maximumWholeValue + 1


(maximumWholeValue - minimumWholeValue) + 1?

darinkrauss · 2019-02-28T05:48:46Z

FYI - There are a couple of comments that are stale due to some commits you added recently.

… Type

ps2 · 2019-02-28T06:01:48Z

A number of comments. Overall I like the design. Not sure I agree with the separation of the whole number from fractional number in the picker though. It is very odd to go from 2.00 to 1.90. Was using a single number component too clunky going from 0.00 to 5.00 in 0.05 steps?

The issue is that max basal rate can be 35 on mm pumps, and 30 on omnipod, and MM can do 0.025 rates as well. Though I do think it could be smart to switch to a single spinner column if the user's max basal is low enough to make it work.

mpangburn

Bug: it's possible to add more than the maximum allowed number of rates.
To reproduce:

Add to maximum number of rates. (add button will disable)
Sync with pump. (add button will reenable)
Tap add again, and the max number of rates has been bypassed.

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

mpangburn · 2019-02-28T19:51:53Z

And a UI bug: if there exist basal rates at both the first second possible times, the date picker of the first cell shows no date.

… reuse

darinkrauss · 2019-03-01T18:36:54Z

Going to review it again now. One more minor bug:

Add a couple basal rate schedules.
Edit a basal rate schedule and spin the basal rate spinner.
While it is slowing down, tap the Edit button. The basal rate schedule closes.
Tap the Done button.
Open the basal rate schedule you were editing in Mock managers update #2.
Note that the basal rate spinner has a different value than the basal rate label.

darinkrauss

Thanks for making all of the changes. Looking good!

mpangburn

Looks good!

The potentially misnamed isCellEditingPossible is my only concern, though the controller behavior seems right.. Otherwise, just nits.

mpangburn · 2019-03-02T01:07:56Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+    }
+
+    private var isCellEditingPossible: Bool {
+        return isReadOnly || isSyncInProgress


I think this property returns the negation of its name.

mpangburn · 2019-03-02T01:08:13Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+        tableView.endEditing(false)
+
+        var startTime = TimeInterval(0)
+        var value = allowedBasalRates.count > 0 ? allowedBasalRates[0] : 0


allowedBasalRates.first ?? 0

mpangburn · 2019-03-02T01:09:31Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+        updateSyncButton()
+    }
+
+    var preferredValueFractionDigits: Int = 1


Why a stored property here?

mpangburn · 2019-03-02T01:10:18Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+    }
+
+    public var isScheduleValid: Bool {
+        return scheduleItems.endIndex > 0 &&


!scheduleItems.isEmpty

mpangburn · 2019-03-02T01:11:12Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+
+    public var isScheduleValid: Bool {
+        return scheduleItems.endIndex > 0 &&
+            scheduleItems.endIndex <= maximumScheduleItemCount &&


Nit: This one I think is clearer as scheduleItems.count, since it's being used to refer to the number of items rather than an index.

mpangburn · 2019-03-02T01:12:02Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+    }
+
+    private func updateTimeLimitsFor(itemAt index: Int) {
+        guard index > 0 && index < scheduleItems.endIndex else {


scheduleItems.indices.contains(index)

mpangburn · 2019-03-02T01:13:30Z

LoopKitUI/View Controllers/BasalScheduleTableViewController.swift

+
+            super.tableView(tableView, commit: editingStyle, forRowAt: indexPath)
+
+            if scheduleItems.endIndex == 1 {


Nit: scheduleItems.count

mpangburn · 2019-03-02T01:14:57Z

LoopKitUI/Views/BasalScheduleEntryTableViewCell.swift

+        super.setSelected(selected, animated: animated)
+
+        if selected && !isReadOnly {
+            isPickerHidden = !isPickerHidden


Nit: isPickerHidden.toggle()

Nice! Didn't know about that.

…ct match

darinkrauss · 2019-03-04T17:11:36Z

LoopKit/QuantityFormatter.swift

+            case .short, .medium:
+                return LocalizedString("U/hr", comment: "The short unit display string for international units of insulin per hour")
+            case .long:
+                return LocalizedString("Units/Hour", comment: "The long unit display string for international units of insulin per hour")


I don't think "Hour" should be capitalized.

Do we need to have a pluralized version? So, "1 Unit/hour" and "2 Units/hour"?

…salScheduleEntryDuration

darinkrauss · 2019-03-05T16:51:01Z

This is all good from my perspective. 🚀

* Add unannounced meal func to CarbStoreProtocol * V1 of UAM algo * Fix issues with time intervals not lining up * Fix issue with ICE not being cumulative * Make carb threshold a static const * Handle errors in fetching glucose effects * Add date range unit test * Style improvement * Update build settings so watch can access LoopNotificationUserInfoKey * Add debug info for feature to carbstore * Remove unneeded variable set * Save last UAM notification time in UserDefaults * Add debug info * Update naming to make constant transition easier * Use constants instead of variables * Only restrict notification delivery so we don't detect the same meal twice * Retract UAM notifications after the carbs have expired * Simplify change & meal checks into a single threshold * Fix effects from outside of the search window being included & triggering false-positive UAM detections * Improve function naming * Add enhanced debug info * Move notification logic from LoopKit to Loop * Improve debug logs * Make current date configurable * Add preliminary tests * Add test * Fix carb entries being purged bc of current date not being set during unit testing * Update UAM tests * Update status enum naming for point-of-use clarity * Fix observation start for unit testing * Pull UAM constants into separate struct * Fix unexpected effect not including all needed effects * Fix carb effects not being carb _counteraction_ effects * Update tests for algo changes * Add noisy cgm testcase * Add test for non-unannounced meal with COB * Add realistic unannounced meal test * Improve algo commenting * Update UAM algo to detect more-aggressively * Remove TODO * Update debug information * Schedule missed meal notifications to avoid notification during an microbolus (#2) * Add ability to estimate delivery duration of bolus * Add tests for UAM notification delay * Update `estimatedDuration` function headers in response to PR feedback * Add UAM banner to carb entry screen (#4) * UAM algo updates: only use directly observed carb absorption (#5) * Add ability to calculate the number of carbs in a missed meal (#3) * Plumb a customizable carb amount through UAM notification architecture * Refactor carb effect threshold computation into helper function * Add first draft of dynamic carb selection for meal * Bump max carb autofill limit from 100 -> 150 * Add tests for carb autofill * Bump max carb threshold limit because of dynamic autofill clamping * Lower the UAM max autofill * Require that unannounced meals have ICE onboard that is >= the meal threshold, and only autofill observed ICE * Lower ICE threshold for a UAM notification * Update tests * Improve debug info * Update commenting * Remove unused test * Update test * `dateRange` -> `simulationDateRange` * Create `MealDetectionManager` from old UAM functions in `CarbStore` * Add fixme * Updates based on feedback for UAM PR (#6) * `dateRange` -> `simulationDateRange` * Create `MealDetectionManager` from old UAM functions in `CarbStore` * Add fixme * Move UAM test fixtures from LoopKit to Loop * Unannounced meal / UAM -> missed meal

ps2 added 14 commits February 26, 2019 10:09

New basal schedule editor, supporting increment and max basal rate li…

c08382c

…mits

Add minimumBasalRatePerHour, and maximumScheduleItemCount

c54b5e1

Do not allow sync if schedule is not valid

1a2c1cf

Adjust allowable startTimes for previous and next cells after editing

022a347

Configure basal schedule editor via initializer

e0cb49f

Support read-only mode

b54da10

Use colors from asset bundle

08ad327

avoid recomputing startOfDay

f641d97

Reuse picker labels

ae44f2b

Revert unneeded changes to nib

6f7d1f6

Update time limits of adjacent rows when inserting and moving cells

32dbb33

Add color assets

aebc163

Scale picker with width changes

b8ccb3c

Use UIPickerVIew provided labels, fix auto layout, and increase picke…

cd545be

…r row height

darinkrauss requested changes Feb 28, 2019

View reviewed changes

Switch back to custom views for better alignment, and support Dynamic…

54c8e71

… Type

ps2 added 3 commits February 28, 2019 00:19

validate -> isBasalRateValid

f524300

Use fixed list of supported basal rates to deal with progressive rates

856797d

Off by one error for initial set of x23 example rates

c5c9f85

mpangburn suggested changes Feb 28, 2019

View reviewed changes

ps2 added 4 commits February 28, 2019 17:05

Misc usability fixes from review

b05c47c

Fix crash when deleting last item; and set isPickerHidden during cell…

911de9d

… reuse

Updates from review

afbdf44

Access colors via UIColor extension, and naming tweaks

c56d141

ps2 added 2 commits March 1, 2019 13:21

Remove unneeded min() call

d31849b

Fix spinner/value mismatch, and other small tweaks

c1b5b2a

darinkrauss approved these changes Mar 1, 2019

View reviewed changes

mpangburn approved these changes Mar 2, 2019

View reviewed changes

ps2 added 2 commits March 4, 2019 09:28

Use QuantityFormatter for basal rate display

d952f1e

Updates from reviews, and select next lowest rate in picker if no exa…

6898470

…ct match

darinkrauss reviewed Mar 4, 2019

View reviewed changes

ps2 added 4 commits March 4, 2019 12:05

PumpManager must provide maximumBasalScheduleEntryCount and minimumBa…

353a541

…salScheduleEntryDuration

Support singular unit string

6160c57

PumpManagers must implement BasalScheduleTableViewControllerSyncSource

0ff1607

Back does not cancel when no syncSource

27c3916

ps2 merged commit 35b6897 into dev Mar 5, 2019

ps2 deleted the basal-picker branch March 5, 2019 17:47


		super.tableView(tableView, commit: editingStyle, forRowAt: indexPath)

		if scheduleItems.endIndex == 1 {

Conversation

ps2 commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darinkrauss left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darinkrauss commented Feb 28, 2019

Uh oh!

ps2 commented Feb 28, 2019

Uh oh!

mpangburn 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!

Uh oh!

Uh oh!

mpangburn commented Feb 28, 2019

Uh oh!

darinkrauss commented Mar 1, 2019

Uh oh!

darinkrauss left a comment

Choose a reason for hiding this comment

Uh oh!

mpangburn 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ps2 commented Feb 27, 2019 •

edited

Loading

darinkrauss left a comment •

edited

Loading