nightscout / Trio

MIT License
66 stars 349 forks source link

Utilize max bolus limit in Bolus module #100

Closed MikePlante1 closed 4 months ago

MikePlante1 commented 5 months ago

In the bolus module, if bolus amount is set over the Max Bolus set in Pump Settings, disable the Enact bolus button, and change its text to Max Bolus exceeded! (>X) where X is the value of the Max Bolus setting.

Max Bolus is set to 10U for this test:

Screenshot 2024-04-30 at 8 25 41 PM

Cherry-picked from https://github.com/Artificial-Pancreas/iAPS/commit/581e3e2838f7ee461df8f86c17db06450d49850e and https://github.com/Artificial-Pancreas/iAPS/commit/7c7cafecf2c10b377d329559b851e417c02ebe96 but did not include external insulin parts since that no longer exists here.

This PR replaces #73 which was accidentally closed.

bjornoleh commented 5 months ago

Could we display the actual max bolus limit here? E.g something like “Max bolus (10U) exceeded”

Also, perhaps use lowercase for “bolus”?

I have not looked at the code or built at this time.

MikePlante1 commented 5 months ago

Could we display the actual max bolus limit here? E.g something like “Max bolus (10U) exceeded”

Also, perhaps use lowercase for “bolus”?

I have not looked at the code or built at this time.

We could, but there are already a bunch of localized strings for Max Bolus exceeded!. I suppose Max Bolus exceeded! (>10U) would be doable, but I'm not sure how much value that adds.

The reason Max Bolus is capitalized and exceeded is not is because Max Bolus is the name of the setting.

bjornoleh commented 5 months ago

I think the reason I’d like to see the actual max bolus setting here is that (if I remember correctly, I haven’t built this yet), the blousing is blocked until you enter an amount below that max setting. If you don’t remember what that was, it could take some trial and error to get to the bolusing (if you act wanted to bolus near the limit).

Before these changes, the app would just (secretly) round down to the max bolus setting, and issue it straight away. Not ideal, of course, but the implementation in the PR adds some faff when entering something just above the max limit.

dsnallfot commented 5 months ago

Just want to share a visualization on how I’ve implemented this in my custom branch. Think it adds some UX value to include the actual numbers. Also the same logic for maxcarbs exceeded In meal view

maxbolus maxcarbs
IMG_3605 IMG_3604
dnzxy commented 5 months ago

Guys, sorry to be the one to point this out: this PR is not about UI redesigning or adding new fancier UI elements or warnings, it’s about bringing a limit into the app that is missing, very much needed for safety, and was added between iAPS 2.3.x and 3.x in dev.

Can we please see this through and add all (very good!) ideas around a better UI to the project board?

bjornoleh commented 5 months ago

@dnzxy Agreed regarding bigger changes like the screenshots above. But my initial suggestion was just to add printing of a variable (max bolus) in the one string. I think that’s not really in the fancy UI department 😊

dnzxy commented 5 months ago

If I‘m not mistaken (please correct me @MikePlante1 or Bjørn), this means having to change all the i18n strings to cater for the string literals of the max bolus variable value. Is that worth it for now if we are probably going to redesign most of this stuff anyways and will very likely be needing new translations in the process, too? If it’s easy and quick to do, let’s please add/change it 🤝.

bjornoleh commented 5 months ago

It’s very quick to copy paste in the English/english for all strings, meaning the translation for the string will be lost, but that’s a minor thing once we have a translation project up (probably Crowdin). We can also quickly get the languages that we collectively master correctly translated here before release too.

MikePlante1 commented 5 months ago

@bjornoleh I don't really think it's needed, but I could add (>10U) to the end of 'Max Bolus exceeded!` if you think it'd be beneficial. I'd just rather leave that localized string intact, at least for now.

MikePlante1 commented 5 months ago

^Obviously the 10 would change to whatever is set in ⚙️>Pump Settings>Max Bolus.

bjornoleh commented 4 months ago

I would still like to see the max bolus setting in the string that currently says "Max Bolus exceeded!". But I don't see exactly how to do this myself just yet, and I won't find time to look closer during the next few days.

Great if someone else can do this, but if not, I'm happy to accept the PR as is for now.

MikePlante1 commented 4 months ago

I would still like to see the max bolus setting in the string that currently says "Max Bolus exceeded!". But I don't see exactly how to do this myself just yet, and I won't find time to look closer during the next few days.

Great if someone else can do this, but if not, I'm happy to accept the PR as is for now.

Is Max Bolus Exceeded! (>10U) an acceptable formatting to you? 10 being whatever your set maxBolus is that you’ve exceeded

bjornoleh commented 4 months ago

I would still like to see the max bolus setting in the string that currently says "Max Bolus exceeded!". But I don't see exactly how to do this myself just yet, and I won't find time to look closer during the next few days. Great if someone else can do this, but if not, I'm happy to accept the PR as is for now.

Is Max Bolus Exceeded! (>10U) an acceptable formatting to you? 10 being whatever your set maxBolus is that you’ve exceeded

Yes, I think this would be great!

MikePlante1 commented 4 months ago

@bjornoleh The max bolus string has now been updated. I confirmed localizations still work with it by selecting Deutsch in a simulator. I also deleted the duplicate localization you pointed out.

bjornoleh commented 4 months ago

Thanks @MikePlante1! Merging this with two approvals.