Various Omnipod Active Time display improvements #673
Conversation
itsmojo
commented
Jul 26, 2021
itsmojo
commented
- Refresh the Active Time value on a status reload
- Change activedAt case name to activeTime to match reality
- Document that the displayed Active Time is a calculated value
- Document how to calculate an Active Time which matches the pod
+ Refresh the Active Time value on a status reload + Change activedAt case name to activeTime to match reality + Document that the displayed Active Time is a calculated value + Document how to calculate an Active Time which matches the pod
| let cell = tableView.dequeueReusableCell(withIdentifier: SettingsTableViewCell.className, for: indexPath) | ||
| cell.textLabel?.text = LocalizedString("Active Time", comment: "The title of the cell showing the pod activated at time") | ||
| cell.textLabel?.text = LocalizedString("Active Time", comment: "The title of the cell showing the pod active time") | ||
| // Since the podState doesn't actually keep the pod's active time value reported in each response, |
There was a problem hiding this comment.
I don't think we should be checking in unused code.
Also, the comments are not right; the choice here is Active Time based on the phone's clock, or based on the Pod's clock. It's not a question of what we store. We technically have the ability to show either.
If you really want to show a value based on the pod's active time, which I could see being useful for planning time of expiry, then show a TimeInterval based representation of time to expiry, based on our computed expiresAt. This could be another row, and we can keep the phone-based Active Time.
There was a problem hiding this comment.
The main reason I'm wanting to keep phone based active time is because by saying "Active Time" we're telling the user how much time has elapsed since the pod was activated. The pod's cheap crystals might be way off, and not an actual reflection of the active time.
For the use case of a person knowing when the pod will expire, it's better to show a "Time to expiry" field. If you really want to show the pod's idea of elapsed time, that's ok, but it should be something like "Pod Lifetime Counter" or something, that indicates it's just the pod's (often wrong) idea of time elapsed since activation.
There was a problem hiding this comment.
I was documenting why there is a calculation for Active Time at all instead of simply displaying the active time value returned by the pod as would be expected if this value was stored. Then I was documenting that the calculation being done is based on the phone’s clock (which I euphemistically described as the true actual time) and the alternate calculation which can be done to compute an active time based on the pod’s clock (for anyone trying to understand how to get a more natural and helpful active time display to fix issue #656 IMO). The comments are right.
Showing “a TimeInterval based representation of time of expiry” being the time to the nominal 72-hour pod expiry, it could show inconsistent and confusing values with the current phone’s clock based Active Time with potential pod clock differences after 3 days (e.g., an Active Time of not yet 72 hours but the pod’s nominal expiry has already passed). I think that doing this would only highlight problems with having the Active Time display based on the phone’s clock and would just make matters worse.
While I strongly believe that Loop should be displaying an Active Time based on the pod’s clock for the reasons I have stated before (it would always match the value displayed in Read Pod Status and thus prevent situations like those described in issue #656, it would make sense with the Expire value which is adjusted based on the pod’s active time, it would be natural that this should be based on the pod’s active time status value, etc), in this version I was actually trying to capitulate on this with some comments to try to close issue #656 somehow and to at least land other some Active Time related improvements (renaming this enum value from the totally inappropriate activatedAt and redisplaying the updated Active Time value on refresh).
As a possible alternative approach to playing with the comments surrounding the current Active Time display, what about changing this row to match the Read Pod Status display of the pod’s active time by using the label of “Pod Active” with a time value down to the minute? This new name would suggest that this value is the based on the pod’s clock and displaying the more precise value wouldn’t create any confusing/mismatched values when compared to either the “Expires” time or the “Read Pod Status” output.
|
I think this is a clearer solution: #677, and better for the user, anyway. |
|
Preferring this: #677, as it is unambiguous with respect to pod time vs phone time. |
