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

Fix possible incorrect total basal units in profile editor #421

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

Conversation

robertdejong1
Copy link

Fixes possible incorrect total basal units in profile editor

Steps to reproduce

  • Edit simple basal profile eg. 00:00 1 u/hr, 12:00 0.5 u/hr
    • Total shows 18 u/day, which is correct.
  • Add a new entry and let it start at 10:00 with 0.75 u/hr
    • Total units now shows 21.5 u/day which is incorrect. It should be 17.5 u/day.

Cause
This is caused by the fact that the total is calculated before the entries are sorted.

Effects
It seems to be a visual problem only, saving on pump seems to save the profile correctly. And after reopening the profile editor, the total is calulated correctly aswell.

Priority
Even though this might not be a problem when 1.0 comes with reworked settings, this bug got me scared when editing my basal profile. So I figured this might be worth fixing.

Deduplication of items
Furthermore, upon inspecing the validate() function I noticed that entries will get deduplicated based on the start time. As this method is called .onAppear of the RootView it's possible that while editing an entry it will get depulicated if one selects an already used start time. after that happens, changing the time or rate of this entry will cause the application to crash

I included a fix for that particular situation by making it impossible to select an already used start time, which I feel like is a nice addition from a users perspective aswell.
But this is more of an edge case and this problem also exists in other similar views like the ISF profile editor. So if we think it's worth fixing, I could also fix that in a separate PR and then include the other views aswell.

This is my first PR to this project, so feedback is welcome :)

@dnzxy
Copy link
Contributor

dnzxy commented Sep 28, 2024

Hi @robertdejong1
thank you very much for your contribution.
Good find, and a good solution.

We are not really adding anything on top of 0.2.0 other than critical fixes, because with the (in the works) Trio 1.0.0 the entirety of settings are reworked.

Let me verify that we have addressed this in 1.0-dev ☺️ if not, I‘ll adapt your work to 1.0 with appropriate contribution credit by you 🙏

@Sjoerd-Bo3
Copy link
Contributor

@robertdejong1 are you already on discord? That way it's easier to contribute. We would love fellow dev's / enthusiasts to help.

@robertdejong1
Copy link
Author

Hi @Sjoerd-Bo3
Yes I'm robert0648 on discord. Would love to help out where I can.

dnzxy
dnzxy previously approved these changes Sep 28, 2024
Copy link
Contributor

@dnzxy dnzxy left a comment

Choose a reason for hiding this comment

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

If you change the function signature to func availableTimeIndices(_ itemIndex: Int) -> [Int] it may become a little more readable and you can omit the parameter name when the function is called.

That's about the only thing I could add here. Great contribution.

Looking at the other settings (glucose target, isf, carb ratio), settings there are essentially copy paste, so this is probably also applicable there; doesn't necessarily fall into the scope of this PR though.

LGTM.

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

Successfully merging this pull request may close these issues.

3 participants