-
Notifications
You must be signed in to change notification settings - Fork 195
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
Shortcuts - refactoring and add new functionalities #177
base: dev
Are you sure you want to change the base?
Conversation
Refactoring the shortcuts with adding : - same organisation of files - remove junk code - add internationalization of shortcuts - add AddCarbs shortcuts - add createAndApply temp target - Add override preset selection, application and cancelation - Add Carbs preset list and display all carbs, fat and proteins associated - fix carb list shortcut issue - fix carb added with fat/protein shortcut - add bolus shortcuts with guardrails - harmonise temporary target labels - add in OIAPS a shortcut config allowing to authorize bolus enact and guardrails. - Add localisation for the UI shortcuts config view and shortcuts - minor improvements to avoid warning in build
md/dL instead of mg/L for instead of during # Conflicts: # FreeAPS/Sources/Localizations/ShortcutsDetail/ShortcutsDetail.xcstrings
@avouspierre I made some suggestions to harmonize the texts and also list all shortcuts for the Trio app However the Bolus shortcut does not popup the amount dialog in itself as the Add carbs shortcut does. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see commits I proposed in link above
Q: I see temp override and temp target. I believe currently they're just the temp targets. Are overrides supposed to be what's called a Profile in-app? |
That was one thing I suggested, calling them overrides without temp as they can be also without an end date. |
however the BolusIntent does not popup the amount dialog
thanks @mountrcg ! I updated the PR with your contribution + 1 more fix for wording |
|
@avouspierre the Bolus shortcut when run as shortcut and not integrated as an action does not ask for bolus amount. Instread throws an error. Carbs works fine this way. Could you add that to Bolus as well
|
The fix for this is to not mark Will add these notes in a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested change at line 28 and 50 to ensure a bolus amount is declared in the shortcut prompt (bug identified by @mountrcg).
• Mark bolusQuantity as non-optional • Remove associated guard/else {...} since this is not optional
Committed requested changes to the PR and tested on a physical device. Ready to merge in my opinion. @marionbarker or @bjornoleh or @mountrcg could you give a 👍 and merge if you agree? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn’t look at the localisations. And do the tables work with crowdin?
But found 2 foreign changes that should not be in this PR.
For the rest LGTM
I merged dev into Then I got the following unstaged changes:
So Xcode seems to have added the The app built ok and is running on my phone, but I haven't done any actual testing yet. |
Quick comment: When limiting bolus to insulin recommendation, a more specific info string could be presented in the shortcut. Tested 1 U bolus: Recommended was 0,5 U Response: Suggestion: Add info to indicate that the bolus was limited to xx U due to the "Limit by insulin suggestion estimation". Similarly when issuing a bolus above the max bolus limit, it would be good to get a specific response that tells that the bolus was limited to that value. Another comment: this settings string could possibly be improved/shortened: "Limit by insulin suggestion estimation" |
Regarding the uncommitted changes and sync with dev, I opened this PR: |
So I re-tested all of the above and could not reproduce any of the errors. Could it have been some initial thing as that were the first overrides I activated on a fresh installation? EDIT: sry I forgot to specify that I tested this: #177 (comment) |
Ok, thanks. Can you verify there are no uncommitted changes on build now after the last commit? If so I can give my approval. |
Ref my previous post: Unless there are still tings to do regarding the above. |
I merged this PR into the latest alpha and tested on a simulator... I toggled on In this circumstance, I think the bolus shortcut should fail and no insulin should be delivered, not the maximum allowed amount. For the test above, it could have been someone who meant to type Also, here's a couple wording suggestions: current:
suggestion:
current:
suggestion: I'm a little unsure the best way to phrase the following, but how about current:
suggestion:
current:
suggestion:
current:
suggestion:
|
Are there any blockers for this to go forward? Was testing successful? I see some phrasing recommendations - I recommend those either get fixed (@MikePlante1 maybe add them in directly?) or touch those up after initial release? |
@aug0211 Phrasing suggestions aren’t crucial, but I think the initial part of my comment about delivering the max when requesting above the max should be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a note for @MikePlante1, I think I identified the section of code he is interested in, and I proposed what this could look like.
table: "ShortcutsDetail" | ||
) | ||
case .limitBolusMax: | ||
bolusQuantity = apsManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikePlante1 is this where the change is needed for your recommendation? Is the check here to compare the bolusAmount with maxBolus?
I think it's something like this:
if bolusAmount > maxBolus {
return LocalizedStringResource("Maximum bolus exceeded, no bolus administered.")
}
else {
bolusQuantity = roundBolus(Decimal(bolusAmount))
}
Do this down below for the other version - checking for recommended percentage.
|
Also, Carbs seems to work like Bolus when it comes to exceeding the maximum limit. As in, if I set Max Carbs to 30g and enter 44g in the Carbs shortcut, it prompts to confirm an entry of "30g". I think a better workflow would be for it to just alert you that your entry exceeded the maximum and prompt you to edit your entry. Just like how the Meal and Bolus modules work in the Trio app on the iPhone. |
Another test:
Screen.Recording.2024-07-01.at.3.50.17.PM.mov |
I can reproduce this when running locally on a test phone, in Trio, iAPS 4.X and iAPS 3.X. They all end up having the actual expected action (they enter the amount of carbs entered), except for the 201 case which never logs anything because it keeps asking for an amount. So this issue is a "display/confirmation" issue, living somewhere in the confirmation code. Will try to take a look next time I can get time. |
Couldn't get video to load here but this seems similar to the carb issue - a shortcut confirmation text issue. I think:
I can work on and test this in an in vitro device. |
- Shortcut cancellation confirmation text should display the latest override, not the first override - Addresses a text display bug identified by Mike here: nightscout#177 (comment)
Fixed this one and committed. I kept the originally desired intent of showing the "current" override that was cancelled in the confirmation text. If we want to switch to the simpler "Override cancelled" confirmation I proposed above, that's a simple 1 line change we can make in the future after merging. |
- Error handling for carb/fat/protein exceeding max defined - Show user all values for carb/fat/protein entered in the confirmation - Do not assume 0 for these values, instead - ask the user - This can be refactored later if we can get access to settingsManager.settings.maxCarbs/Fat/Protein: if we get that, drop it into the upper bound in the shortcut and clean up other code - Lastly, we should allow the user to specify the date if they'd like to, in the future
Fixed this and a few other things in this commit.
|
- Block (with an alert) boluses that exceed the user-configured max (whether this is max pump max bolus or recommended amount) - Addresses this request nightscout#177 (comment)
Fixed in this commit. Boluses will now be blocked with an alert if they exceed the maximum configured (whether this is pump max bolus or insulin suggestion). |
- Addresses requested updates in PR feedback for phrasing - See original requests here: nightscout#177 (comment) - Skipped one request based on my judgment - This commit includes comment cleanup on shortcut guardrails, too - No functional change in this commit
Phrasing notes requested in this comment are addressed in this commit. |
Thanks @aug0211, it's certainly improved now, but still a bit more to fix I think. Regarding canceling of overrides, I think it's fine to just say Example 1: Start a new profile override (not a saved override) in the Trio app. Then run the "Cancel Trio Override" shortcut. The shortcut says "No active Override to cancel", even though it actually does cancel the override. Example 2: Start a saved profile override with a set duration. Wait for the override to expire. Run the "Cancel Trio Override" shortcut and it will say Example 3: Create two saved profiles: OA and OB. In the Trio app, start OA and then start OB. Run the "Cancel Trio Override" shortcut and it will say For the "Trio Bolus" shortcut, when Likewise, when I set The check failed, however, when the Insulin recommended equals the Max Bolus. I entered 120g carbs, which caused the Insulin recommended to be 4U. It would have been higher, but it was capped at 4U because the Max Bolus setting is 4U. I then ran the "Trio Bolus" shortcut and requested a 5U bolus. It asked carbs/fat/protein seem to be properly capped at their max limits in settings. I do find it odd that it seems to pad numbers to one decimal place in the prompts, though. I also don't like how it responds when you don't enter a value for any of the prompts, though. It looks like the prompt should default to 0 if you don't enter anything for carbs, fat, or protein and just hit "done" to move to the next macro prompt, but instead the shortcut just kills itself without any feedback. So if you enter 10 for carbs, hit Done, then it prompts for fat but it looks like 0g is already pre-populated, so you hit Done, and it crashes. Maybe there should be two shortcuts: |
Shortcuts Trio
this PR refactors the shortcuts and adds new shortcuts for Trio.
Refactoring
List of shortcuts available
Some examples described in the picture :
When your iPhone alarm clock is ringing a Profile override is started/canceled.
• When you get to your gym a Profile Override is started/canceled.
• When you step inside your car (which has CarPlay) a profile override is started/canceled.
• When you start an exercise a profile override is started.
• When you end an exercise the active profile override is canceled.
• .
New UI for config shortcuts :
Detail of the commit (squash all commits about shortcuts) :
Refactoring the shortcuts with adding :