Skip to content
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

Fmegen/3.3.0.0 - Adding VeryHigh Overview Display Option #3678

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

vanmegen
Copy link

@vanmegen vanmegen commented Jan 2, 2025

This is analog to the nightscout Very High setting which displays bg values higher than a configurable value in a different color.
I did set the default to 400, but it can be set to 240 to be a similar threshold as nightscout uses.

(in the following screenshot red dots mark values higher than 240)
image

I have added this to the overview settings
image

as well as to the onboarding wizzard
image

@MilosKozak
Copy link
Contributor

MilosKozak commented Jan 2, 2025

I kept it without "very high" to match dexcom setting
But I'm open to discussion about this

@olorinmaia
Copy link
Contributor

I prefer keeping red color for lows.

But, default value is set to 400 mg/dl which translates to about 22 mmol/l so I think this is a win / win for everyone. This new setting can be set so high that it won't have affect on the visuals in most cases as we rarely go above 13 mmol\l. Then I can just set it to e.g. 20 mmol/l and it will display like before. Or if users want it lower they have the freedom to do so.

So I think this will be a good change, regardless if you prefer it like before, or want red color for very high.

Copy link

sonarqubecloud bot commented Jan 3, 2025

@vanmegen
Copy link
Author

vanmegen commented Jan 3, 2025

@olorinmaia

So I think this will be a good change, regardless if you prefer it like before, or want red color for very high.
Thanks for the comments!

@MilosKozak

I kept it without "very high" to match dexcom setting But I'm open to discussion about this
The reason I added this to aaps is that this display format is one of the defaults in nightscout and quite useful to get at-a-glance information about the current range. like "green" == all good, "yellow" == not-great-not-terrible, and "red" == bad.

here is the corresponding nightscout screenshot
image

in nightscout, you can set this via environment variable
image

I agree that this should be the "high" externally (i.e., in the UI), however, internally, the range above 180 is already named "high_value", which is why I used the very_high_value names instead.

should I do any change to the PR?

@Philoul
Copy link
Contributor

Philoul commented Jan 4, 2025

Hum, if we want to be fully consistant, we should also review Watch graph (to include the "Very High color") and watchfaces colors for Very High BG...
So probably a bit more work to do.
I should also have to update a bit Custom Watchface code to keep compatibility with Custom Watchfaces V1 for BG color management

@Philoul
Copy link
Contributor

Philoul commented Jan 4, 2025

If this proposal is merged, maybe it's better to manage Watch update for color consistancy in a different PR 🤔

@koelewij
Copy link

koelewij commented Jan 7, 2025

As @olorinmaia already mentioned, setting the "Very High" threshold to a very high value is basically allowing you to disable it would in my opinion, not bother me to implement this.

@vanmegen
Copy link
Author

vanmegen commented Jan 8, 2025

Hi @Philoul

If this proposal is merged, maybe it's better to manage Watch update for color consistancy in a different PR 🤔
I would suggest so too as I dont have a physical watch for testing,

@vanmegen
Copy link
Author

vanmegen commented Jan 8, 2025

Hi @koelewij,

As @olorinmaia already mentioned, setting the "Very High" threshold to a very high value is basically allowing you to disable it would in my opinion, not bother me to implement this.
thanks you for your feedback

@Philoul
Copy link
Contributor

Philoul commented Jan 8, 2025

@vanmegen I will manage watch update (with Phone app impact for data communication) on my side.

  • I will have to manage compatibility with v1 and v2 custom watchfaces
  • don't know if complications are impacted or not

@vanmegen
Copy link
Author

vanmegen commented Jan 9, 2025

Hi @Philoul

@vanmegen I will manage watch update (with Phone app impact for data communication) on my side.
Awesome! thank you very much

@MilosKozak
what do you think? Is it worth merging?

kind regards,
friedel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants