Skip to content

Denote stale glucose (LOOP-927) #44

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 27, 2020
Merged

Denote stale glucose (LOOP-927) #44

merged 5 commits into from
Jan 27, 2020

Conversation

@ps2
Copy link

@ps2 ps2 commented Jan 14, 2020

Update based on design review of DIY merge. https://tidepool.atlassian.net/browse/LOOP-927

@ps2
Copy link
Author

ps2 commented Jan 16, 2020

  • A single place for specification of glucose age time interval (LoopSettings?). Currently hardcoded at 15 minutes.
  • HUDInterfaceController in WatchApp Extension should not display old bg data (second page of main watch app)
  • Complications updated to show '---' when bg data is missing.
  • Complications should register future timeline entries to allow bg data to go stale without updates from Loop.

@ps2 ps2 force-pushed the missing-glucose-signifier branch 2 times, most recently from c2cb548 to fb87c6c Compare January 17, 2020 22:20
@ps2 ps2 changed the title Use three dashes for missing glucose instead of one (LOOP-927) Denote stale glucose (LOOP-927) Jan 18, 2020
@ps2
Copy link
Author

ps2 commented Jan 18, 2020

Watch app and complications are now also showing Loop completion freshness using color. BG going stale is still denoted with dashes.

@ps2 ps2 force-pushed the missing-glucose-signifier branch from fd0554b to cf77081 Compare January 18, 2020 00:04
@ps2 ps2 requested review from darinkrauss and nhamming January 18, 2020 00:17
nhamming
nhamming approved these changes Jan 20, 2020
View reviewed changes
Copy link

@nhamming nhamming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question about why there seems to be 2 different definitions of stale. Probably me missing something.

WatchApp Extension/Controllers/HUDInterfaceController.swift

glucoseLabel.setHidden(true)
glucoseLabel.setText("---")
glucoseLabel.setHidden(false)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this line, Line 62 isn't needed.

darinkrauss
darinkrauss approved these changes Jan 20, 2020
View reviewed changes
Copy link

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. More only questions were around the use of LoopCompletionFreshness versus RecencyInterval, but @nhamming covered that. 🚀

@ps2 ps2 requested review from darinkrauss and nhamming January 21, 2020 19:48
@ps2
Copy link
Author

ps2 commented Jan 21, 2020

Re-requested reviews, just to make sure my responses address your comments before merging.

darinkrauss
darinkrauss approved these changes Jan 21, 2020
View reviewed changes
Copy link

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

If RecencyInterval is specific to glucose values, then perhaps renaming this to something more specific might clarify the difference.

@ps2
Copy link
Author

ps2 commented Jan 22, 2020

@darinkrauss It's not just for glucose data. It's also for pump data. It's about the age limit of data Loop is willing to use for creating a forecast. Maybe inputDataRecencyInterval?

nhamming
nhamming approved these changes Jan 22, 2020
View reviewed changes
Copy link

@nhamming nhamming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darinkrauss
Copy link

@ps2 Yes, something to distinguish it from the loop completion freshness. But, you can also do this in a later code change.

@ps2 ps2 requested review from darinkrauss and nhamming January 24, 2020 21:10
darinkrauss
darinkrauss approved these changes Jan 25, 2020
View reviewed changes
Copy link

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

@ps2 ps2 merged commit d1764b3 into dev Jan 27, 2020
@ps2 ps2 deleted the missing-glucose-signifier branch February 6, 2020 14:08
@ps2 ps2 mentioned this pull request Feb 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darinkrauss darinkrauss darinkrauss approved these changes

@nhamming nhamming Awaiting requested review from nhamming

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ps2 @darinkrauss @nhamming